# [PATCH] files_for_each_reflog_ent_reverse(): close stream and free strbuf on error 2 messages from 2017-04-16 to 2017-04-17. Participants: René Scharfe, Michael Haggerty. Thread: https://gitlist.dev/t/45712 ## René Scharfe, 2017-04-16 16:55 Subject: [PATCH] files_for_each_reflog_ent_reverse(): close stream and free strbuf on error Message-ID: URL: https://gitlist.dev/e/a77dfa47-3153-3f8c-3c37-30147f93e61b%40web.de ``` 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 --- 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); 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, 2017-04-17 07:05 Subject: Re: [PATCH] files_for_each_reflog_ent_reverse(): close stream and free strbuf on error Message-ID: URL: https://gitlist.dev/e/d26b6409-988e-386c-1fb3-914319ada8a6%40alum.mit.edu In-Reply-To: ``` 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. > Signed-off-by: Rene Scharfe > --- > 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. > 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 Michael ```