[PATCH] files_for_each_reflog_ent_reverse(): close stream and free strbuf on error

René Scharfe· Apr 16, 2017, 16:55 UTC · lore

Exit the loop orderly through the cleanup code, instead of dashing out with logfp still open and sb leaking.

Found with Cppcheck.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 refs/files-backend.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

Show changes to refs/files-backend.c +12 −8

diff --git a/refs/files-backend.c b/refs/files-backend.c
index 50188e92f9..2889f21568 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -3159,8 +3159,8 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
 
 	/* Jump to the end */
 	if (fseek(logfp, 0, SEEK_END) < 0)
-		return error("cannot seek back reflog for %s: %s",
-			     refname, strerror(errno));
+		ret = error("cannot seek back reflog for %s: %s",
+			    refname, strerror(errno));
 	pos = ftell(logfp);
 	while (!ret && 0 < pos) {
 		int cnt;
@@ -3170,13 +3170,17 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
 
 		/* Fill next block from the end */
 		cnt = (sizeof(buf) < pos) ? sizeof(buf) : pos;
-		if (fseek(logfp, pos - cnt, SEEK_SET))
-			return error("cannot seek back reflog for %s: %s",
-				     refname, strerror(errno));
+		if (fseek(logfp, pos - cnt, SEEK_SET)) {
+			ret = error("cannot seek back reflog for %s: %s",
+				    refname, strerror(errno));
+			break;
+		}
 		nread = fread(buf, cnt, 1, logfp);
-		if (nread != 1)
-			return error("cannot read %d bytes from reflog for %s: %s",
-				     cnt, refname, strerror(errno));
+		if (nread != 1) {
+			ret = error("cannot read %d bytes from reflog for %s: %s",
+				    cnt, refname, strerror(errno));
+			break;
+		}
 		pos -= cnt;
 
 		scanp = endp = buf + cnt;

-- 
2.12.2

Michael Haggerty· Apr 17, 2017, 07:05 UTC · re: René Scharfe · lore

Re: [PATCH] files_for_each_reflog_ent_reverse(): close stream and free strbuf on error

On 04/16/2017 06:55 PM, René Scharfe wrote:

> Exit the loop orderly through the cleanup code, instead of dashing out
> with logfp still open and sb leaking.
> 
> Found with Cppcheck.

Nice catch.

Show 18 quoted lines

> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  refs/files-backend.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 50188e92f9..2889f21568 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -3159,8 +3159,8 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
>  
>  	/* Jump to the end */
>  	if (fseek(logfp, 0, SEEK_END) < 0)
> -		return error("cannot seek back reflog for %s: %s",
> -			     refname, strerror(errno));
> +		ret = error("cannot seek back reflog for %s: %s",
> +			    refname, strerror(errno));
>  	pos = ftell(logfp);

It seems odd to call `ftell()` in the case that `fseek()` has failed, but it should be harmless.

Show 27 quoted lines

>  	while (!ret && 0 < pos) {
>  		int cnt;
> @@ -3170,13 +3170,17 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
>  
>  		/* Fill next block from the end */
>  		cnt = (sizeof(buf) < pos) ? sizeof(buf) : pos;
> -		if (fseek(logfp, pos - cnt, SEEK_SET))
> -			return error("cannot seek back reflog for %s: %s",
> -				     refname, strerror(errno));
> +		if (fseek(logfp, pos - cnt, SEEK_SET)) {
> +			ret = error("cannot seek back reflog for %s: %s",
> +				    refname, strerror(errno));
> +			break;
> +		}
>  		nread = fread(buf, cnt, 1, logfp);
> -		if (nread != 1)
> -			return error("cannot read %d bytes from reflog for %s: %s",
> -				     cnt, refname, strerror(errno));
> +		if (nread != 1) {
> +			ret = error("cannot read %d bytes from reflog for %s: %s",
> +				    cnt, refname, strerror(errno));
> +			break;
> +		}
>  		pos -= cnt;
>  
>  		scanp = endp = buf + cnt;
>

Reviewed-by: Michael Haggerty <mhagger@alum.mit.edu>

Michael

patchfiles_for_each_reflog_ent_reverse(): close stream and free strbuf on error