{"thread":{"id":"45712","subject":"[PATCH] files_for_each_reflog_ent_reverse(): close stream and free strbuf on error","startedAt":"2017-04-16T16:56:00Z","lastAt":"2017-04-17T07:05:11Z","messageCount":2,"participants":["René Scharfe","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"316952","messageId":"a77dfa47-3153-3f8c-3c37-30147f93e61b@web.de","threadId":"45712","inReplyTo":null,"subject":"[PATCH] files_for_each_reflog_ent_reverse(): close stream and free strbuf on error","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-04-16T16:55:46Z","receivedAt":"2017-04-16T16:56:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Exit the loop orderly through the cleanup code, instead of dashing out\nwith logfp still open and sb leaking.\n\nFound with Cppcheck.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n refs/files-backend.c | 20 ++++++++++++--------\n 1 file changed, 12 insertions(+), 8 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 50188e92f9..2889f21568 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3159,8 +3159,8 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n \n \t/* Jump to the end */\n \tif (fseek(logfp, 0, SEEK_END) < 0)\n-\t\treturn error(\"cannot seek back reflog for %s: %s\",\n-\t\t\t     refname, strerror(errno));\n+\t\tret = error(\"cannot seek back reflog for %s: %s\",\n+\t\t\t    refname, strerror(errno));\n \tpos = ftell(logfp);\n \twhile (!ret && 0 < pos) {\n \t\tint cnt;\n@@ -3170,13 +3170,17 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n \n \t\t/* Fill next block from the end */\n \t\tcnt = (sizeof(buf) < pos) ? sizeof(buf) : pos;\n-\t\tif (fseek(logfp, pos - cnt, SEEK_SET))\n-\t\t\treturn error(\"cannot seek back reflog for %s: %s\",\n-\t\t\t\t     refname, strerror(errno));\n+\t\tif (fseek(logfp, pos - cnt, SEEK_SET)) {\n+\t\t\tret = error(\"cannot seek back reflog for %s: %s\",\n+\t\t\t\t    refname, strerror(errno));\n+\t\t\tbreak;\n+\t\t}\n \t\tnread = fread(buf, cnt, 1, logfp);\n-\t\tif (nread != 1)\n-\t\t\treturn error(\"cannot read %d bytes from reflog for %s: %s\",\n-\t\t\t\t     cnt, refname, strerror(errno));\n+\t\tif (nread != 1) {\n+\t\t\tret = error(\"cannot read %d bytes from reflog for %s: %s\",\n+\t\t\t\t    cnt, refname, strerror(errno));\n+\t\t\tbreak;\n+\t\t}\n \t\tpos -= cnt;\n \n \t\tscanp = endp = buf + cnt;\n-- \n2.12.2\n\n"},{"id":"316990","messageId":"d26b6409-988e-386c-1fb3-914319ada8a6@alum.mit.edu","threadId":"45712","inReplyTo":"a77dfa47-3153-3f8c-3c37-30147f93e61b@web.de","subject":"Re: [PATCH] files_for_each_reflog_ent_reverse(): close stream and free strbuf on error","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-04-17T07:05:00Z","receivedAt":"2017-04-17T07:05:11Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/16/2017 06:55 PM, René Scharfe wrote:\n> Exit the loop orderly through the cleanup code, instead of dashing out\n> with logfp still open and sb leaking.\n> \n> Found with Cppcheck.\n\nNice catch.\n\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  refs/files-backend.c | 20 ++++++++++++--------\n>  1 file changed, 12 insertions(+), 8 deletions(-)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 50188e92f9..2889f21568 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -3159,8 +3159,8 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n>  \n>  \t/* Jump to the end */\n>  \tif (fseek(logfp, 0, SEEK_END) < 0)\n> -\t\treturn error(\"cannot seek back reflog for %s: %s\",\n> -\t\t\t     refname, strerror(errno));\n> +\t\tret = error(\"cannot seek back reflog for %s: %s\",\n> +\t\t\t    refname, strerror(errno));\n>  \tpos = ftell(logfp);\n\nIt seems odd to call `ftell()` in the case that `fseek()` has failed,\nbut it should be harmless.\n\n>  \twhile (!ret && 0 < pos) {\n>  \t\tint cnt;\n> @@ -3170,13 +3170,17 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n>  \n>  \t\t/* Fill next block from the end */\n>  \t\tcnt = (sizeof(buf) < pos) ? sizeof(buf) : pos;\n> -\t\tif (fseek(logfp, pos - cnt, SEEK_SET))\n> -\t\t\treturn error(\"cannot seek back reflog for %s: %s\",\n> -\t\t\t\t     refname, strerror(errno));\n> +\t\tif (fseek(logfp, pos - cnt, SEEK_SET)) {\n> +\t\t\tret = error(\"cannot seek back reflog for %s: %s\",\n> +\t\t\t\t    refname, strerror(errno));\n> +\t\t\tbreak;\n> +\t\t}\n>  \t\tnread = fread(buf, cnt, 1, logfp);\n> -\t\tif (nread != 1)\n> -\t\t\treturn error(\"cannot read %d bytes from reflog for %s: %s\",\n> -\t\t\t\t     cnt, refname, strerror(errno));\n> +\t\tif (nread != 1) {\n> +\t\t\tret = error(\"cannot read %d bytes from reflog for %s: %s\",\n> +\t\t\t\t    cnt, refname, strerror(errno));\n> +\t\t\tbreak;\n> +\t\t}\n>  \t\tpos -= cnt;\n>  \n>  \t\tscanp = endp = buf + cnt;\n> \n\nReviewed-by: Michael Haggerty <mhagger@alum.mit.edu>\n\nMichael\n\n"}]}