{"thread":{"id":"35891","subject":"[PATCH 0/5] Miscellaneous fixes from static analysis","startedAt":"2014-02-16T16:06:01Z","lastAt":"2014-02-19T00:02:20Z","messageCount":13,"participants":["John Keeping","David Kastrup","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"234892","messageId":"cover.1392565571.git.john@keeping.me.uk","threadId":"35891","inReplyTo":null,"subject":"[PATCH 0/5] Miscellaneous fixes from static analysis","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-02-16T16:06:01Z","receivedAt":"2014-02-16T16:06:01Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"The first two of these fix real bugs, the rest just clean up some of the\nless obviously \"not actually a problem\" issues identified by Clang's\nstatic analyzer [1] and stack[2].\n\nStack is interesting in that it is designed to detect potentially\nundesirable optimizations where undefined behaviour may be being invoked\nunwittingly. It only detected two error's in git.git, the first of\nwhich is fixed by the final patch. The second it describes as:\n\n\tbug: anti-algebra\n\tmodel: |\n\t %11 = icmp ult i8* %extra_args, %7, !dbg !342\n\t --> %10 = icmp slt i64 %9, 0, !dbg !342\n\t ************************************************************\n\t %extra_args \n\t --> (-1 * (sext i32 %buflen to i64)) \n---\n notes-utils.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/notes-utils.c b/notes-utils.c\nindex 2975dcd..4aa7023 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -75,7 +75,7 @@ static int notes_rewrite_config(const char *k, const char *v, void *cb)\n \t\treturn 0;\n \t} else if (!c->mode_from_env && !strcmp(k, \"notes.rewritemode\")) {\n \t\tif (!v)\n-\t\t\tconfig_error_nonbool(k);\n+\t\t\treturn config_error_nonbool(k);\n \t\tc->combine = parse_combine_notes_fn(v);\n \t\tif (!c->combine) {\n \t\t\terror(_(\"Bad notes.rewriteMode value: '%s'\"), v);\n-- \n1.9.rc0.187.g6292fff\n"},{"id":"234893","messageId":"53061a27144d1cbafd509c3b5a13e642a9ea400a.1392565571.git.john@keeping.me.uk","threadId":"35891","inReplyTo":"cover.1392565571.git.john@keeping.me.uk","subject":"[PATCH 2/5] utf8: fix iconv error detection","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-02-16T16:06:03Z","receivedAt":"2014-02-16T16:06:03Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"iconv(3) returns \"(size_t) -1\" on error. Make sure that we cast the\n\"-1\" properly when checking for this.\n\nSigned-off-by: John Keeping \n---\n utf8.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/utf8.c b/utf8.c\nindex 0d20e0a..24c3c5c 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -529,7 +529,7 @@ char *reencode_string_iconv(const char *in, size_t insz, iconv_t conv, int *outs\n \twhile (1) {\n \t\tsize_t cnt = iconv(conv, &cp, &insz, &outpos, &outsz);\n \n-\t\tif (cnt == -1) {\n+\t\tif (cnt == (size_t) -1) {\n \t\t\tsize_t sofar;\n \t\t\tif (errno != E2BIG) {\n \t\t\t\tfree(out);\n-- \n1.9.rc0.187.g6292fff\n"},{"id":"234895","messageId":"2af4618fe70a02ec92c625eb7f53485ba37827f3.1392565571.git.john@keeping.me.uk","threadId":"35891","inReplyTo":"cover.1392565571.git.john@keeping.me.uk","subject":"[PATCH 3/5] utf8: use correct type for values in interval table","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-02-16T16:06:04Z","receivedAt":"2014-02-16T16:06:04Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"We treat these as unsigned everywhere and compare against unsigned\nvalues, so declare them using the typedef we already have for this.\n\nWhile we're here, fix the indentation as well.\n\nSigned-off-by: John Keeping \n---\n utf8.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/utf8.c b/utf8.c\nindex 24c3c5c..a831d50 100644\n--- a/utf8.c\n+++ b/utf8.c\n@@ -5,8 +5,8 @@\n /* This code is originally from http://www.cl.cam.ac.uk/~mgk25/ucs/ */\n \n struct interval {\n- int first;\n- int last;\n+\tucs_char_t first;\n+\tucs_char_t last;\n };\n \n size_t display_mode_esc_sequence_len(const char *s)\n-- \n1.9.rc0.187.g6292fff\n"},{"id":"234898","messageId":"4df594e05741b2bec1201556d34eb22bab2c235a.1392565571.git.john@keeping.me.uk","threadId":"35891","inReplyTo":"cover.1392565571.git.john@keeping.me.uk","subject":"[PATCH 4/5] builtin/mv: don't use memory after free","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-02-16T16:06:05Z","receivedAt":"2014-02-16T16:06:05Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"If 'src' already ends with a slash, then add_slash() will just return\nit, meaning that 'free(src_with_slash)' is actually 'free(src)'. Since\nwe use 'src' later, this will result in use-after-free.\n\nIn fact, this cannot happen because 'src' comes from\ninternal_copy_pathspec() without the KEEP_TRAILING_SLASH flag, so any\ntrailing '/' will have been stripped; but static analysis tools are not\nclever enough to realise this and so warn that 'src' could be used after\nhaving been free'd. Fix this by checking that 'src_w_slash' is indeed\nnewly allocated memory.\n\nSigned-off-by: John Keeping \n---\n builtin/mv.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 21c46d1..7e26eb5 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -162,7 +162,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tif (strncmp(path, src_w_slash, len_w_slash))\n \t\t\t\t\t\tbreak;\n \t\t\t\t}\n-\t\t\t\tfree((char *)src_w_slash);\n+\t\t\t\tif (src_w_slash != src)\n+\t\t\t\t\tfree((char *)src_w_slash);\n \n \t\t\t\tif (last - first < 1)\n \t\t\t\t\tbad = _(\"source directory is empty\");\n-- \n1.9.rc0.187.g6292fff\n"},{"id":"234894","messageId":"0120f99a7cccc118782cda63f478d7d3f528ee3f.1392565571.git.john@keeping.me.uk","threadId":"35891","inReplyTo":"cover.1392565571.git.john@keeping.me.uk","subject":"[PATCH 5/5] streaming: simplify attaching a filter","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2014-02-16T16:06:06Z","receivedAt":"2014-02-16T16:06:06Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"We are guaranteed that 'nst' is non-null because it is allocated with\nxmalloc(), and in fact we rely on this three lines later by\nunconditionally dereferencing it.\n\nSigned-off-by: John Keeping \n---\n streaming.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/streaming.c b/streaming.c\nindex d7c9f32..8a7135d 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -151,10 +151,7 @@ struct git_istream *open_istream(const unsigned char *sha1,\n \t}\n \tif (filter) {\n \t\t/* Add \"&& !is_null_stream_filter(filter)\" for performance */\n-\t\tstruct git_istream *nst = attach_stream_filter(st, filter);\n-\t\tif (!nst)\n-\t\t\tclose_istream(st);\n-\t\tst = nst;\n+\t\tst = attach_stream_filter(st, filter);\n \t}\n \n \t*size = st->size;\n-- \n1.9.rc0.187.g6292fff\n"},{"id":"234900","messageId":"87txbzvxgq.fsf@fencepost.gnu.org","threadId":"35891","inReplyTo":"be9b384ec77fc39b939b8c5505862a6e1c641faa.1392565571.git.john@keeping.me.uk","subject":"Re: [PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-02-16T16:22:45Z","receivedAt":"2014-02-16T16:22:45Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"John Keeping writes:\n\n> If we carry on after outputting config_error_nonbool then we're\n> guaranteed to dereference a null pointer.\n\nNot really relevant to this patch, but looking at the output of\n\ngit grep config_error_nonbool\n\nseems like a serious amount of ridiculousness going on. The header\nshows\n\ncache.h:extern int config_error_nonbool(const char *);\ncache.h:#define config_error_nonbool(s) (config_error_nonbool(s), -1)\n\nand the implementation\n\nconfig.c:#undef config_error_nonbool\nconfig.c:int config_error_nonbool(const char *var)\n\nPresumably this was done so that the uses of config_error_nonbool can be\nrecognized as returning -1 unconditionally.\n\nBut is that worth the obfuscation? Why not let config_error_nonbool\nreturn -1 in the first place? It does not appear like any caller would\ncall the function rather than the macro, so why declare the function as\nreturning an int at all? And why give it the same name as the macro\n(risking human/computer confusion and requiring an explicit #undef for\nthe definition or was that declaration?) instead of\nconfig_error_nonbool_internal or whatever else?\n\n-- \nDavid Kastrup\n"},{"id":"234943","messageId":"20140218074632.GA29804@sigill.intra.peff.net","threadId":"35891","inReplyTo":"87txbzvxgq.fsf@fencepost.gnu.org","subject":"Re: [PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-18T07:46:32Z","receivedAt":"2014-02-18T07:46:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 16, 2014 at 05:22:45PM +0100, David Kastrup wrote:\n\n> Not really relevant to this patch, but looking at the output of\n> \n> git grep config_error_nonbool\n> \n> seems like a serious amount of ridiculousness going on. The header\n> shows\n> \n> cache.h:extern int config_error_nonbool(const char *);\n> cache.h:#define config_error_nonbool(s) (config_error_nonbool(s), -1)\n> \n> and the implementation\n> \n> config.c:#undef config_error_nonbool\n> config.c:int config_error_nonbool(const char *var)\n\nYou could always look in the commit history:\n\n $ git log -S'#define config_error_nonbool' cache.h\n\nor search the mailing list:\n\n http://thread.gmane.org/gmane.comp.version-control.git/211505\n\n> Presumably this was done so that the uses of config_error_nonbool can be\n> recognized as returning -1 unconditionally.\n\nYes, it helps prevent false positives in gcc's flow analysis\n(specifically, -Wuninitialized warnings).\n\n> But is that worth the obfuscation?\n\nYes?\n\n> Why not let config_error_nonbool return -1 in the first place?\n\nIt originally did, but callers do not get to see the static return, so\nthey miss some analysis opportunities.\n\n> It does not appear like any caller would call the function rather than\n> the macro, so why declare the function as returning an int at all?\n\nBecause we don't use these macros on non-gnu compilers; they actually\nsee the real function return.\n\n> And why give it the same name as the macro (risking human/computer\n> confusion and requiring an explicit #undef for the definition or was\n> that declaration?) instead of config_error_nonbool_internal or\n> whatever else?\n\nThis particular case is simple, but see error() for another use of the\nsame technique which requires variadic macros, which are not available\neverywhere. Callers want to say just \"error(...)\", so we have to name\nthe macro that. If we call the matching function \"error_internal\", then\nnon-gnu compilers would need a macro to convert \"error(...)\" to\n\"error_internal(...)\". But they can't do so, because it would be a\nvariadic macro.\n\nSo you could adjust config_error_nonbool(), but not error(). But that\nintroduces its own confusion, because the technique is not applied\nconsistently.\n\nI agree the #define is ugly. But it's confined to a small area, and it\ndoes solve an actual problem.\n\n-Peff\n"},{"id":"234947","messageId":"87vbwcstgw.fsf@fencepost.gnu.org","threadId":"35891","inReplyTo":"20140218074632.GA29804@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-02-18T08:41:51Z","receivedAt":"2014-02-18T08:41:51Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Jeff King writes:\n\n> On Sun, Feb 16, 2014 at 05:22:45PM +0100, David Kastrup wrote:\n>> \n>> config.c:#undef config_error_nonbool\n>> config.c:int config_error_nonbool(const char *var)\n>\n> You could always look in the commit history:\n>\n> $ git log -S'#define config_error_nonbool' cache.h\n>\n> or search the mailing list:\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/211505\n>\n>> Presumably this was done so that the uses of config_error_nonbool can be\n>> recognized as returning -1 unconditionally.\n>\n> Yes, it helps prevent false positives in gcc's flow analysis\n> (specifically, -Wuninitialized warnings).\n>\n>> But is that worth the obfuscation?\n>\n> Yes?\n\ngcc's flow analysis works with the same data as humans reading the\ncode. If there is no information content in the function call, it makes\nmore sense to either making it void.\n\nOne can always explicitly write\n\n (config_error_nonbool(\"panic-when-assailed\"), -1)\n\nfor shortcircuit use inside of an expression even when\nconfig_error_nonbool is a function returning a void expression: comma\noperators do not try type coercion on their first argument.\n\nShrug. This one has likely been discussed to death already. Sometimes\nit's more convenient to avoid getting a question asked in the first\nplace rather than having a stock answer for it.\n\n-- \nDavid Kastrup\n"},{"id":"234949","messageId":"20140218090156.GC2692@sigill.intra.peff.net","threadId":"35891","inReplyTo":"87vbwcstgw.fsf@fencepost.gnu.org","subject":"Re: [PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-02-18T09:01:56Z","receivedAt":"2014-02-18T09:01:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 18, 2014 at 09:41:51AM +0100, David Kastrup wrote:\n\n> gcc's flow analysis works with the same data as humans reading the\n> code. If there is no information content in the function call, it makes\n> more sense to either making it void.\n\nThe point of error() returning a constant -1 is to use this idiom:\n\n if (something_failed)\n return error(\"this will get printed, and we get a -1 return\");\n\n>From a code perspective it's pointless. You could \"just\" write:\n\n if (something_failed) {\n error(...);\n return -1;\n }\n\nwhich is equivalent. But the point is that the former is shorter and a\nlittle more readable, assuming you are familiar with the idiom.\n\n> One can always explicitly write\n> \n> (config_error_nonbool(\"panic-when-assailed\"), -1)\n\nYes, but again, the point is readability. Doing that at each callsite is\nugly and annoying.\n\n> Shrug. This one has likely been discussed to death already. Sometimes\n> it's more convenient to avoid getting a question asked in the first\n> place rather than having a stock answer for it.\n\nYou are the first person to ask about it, so there is no stock answer.\nHowever, everything I told you was in the commit messages and the list\narchive already. We can also avoid questions being asked by using those\ntools.\n\n-Peff\n"},{"id":"234956","messageId":"87mwhosqyg.fsf@fencepost.gnu.org","threadId":"35891","inReplyTo":"20140218090156.GC2692@sigill.intra.peff.net","subject":"Re: [PATCH 1/5] notes-utils: handle boolean notes.rewritemode correctly","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-02-18T09:36:07Z","receivedAt":"2014-02-18T09:36:07Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 18, 2014 at 09:41:51AM +0100, David Kastrup wrote:\n>\n>> gcc's flow analysis works with the same data as humans reading the\n>> code. If there is no information content in the function call, it makes\n>> more sense to either making it void.\n>\n> The point of error() returning a constant -1 is to use this idiom:\n>\n> if (something_failed)\n> return error(\"this will get printed, and we get a -1 return\");\n\n if (something_failed)\n return error(\"this will get printed\"), -1;\n\nNot much of an idiom, no insider logic hidden to both compiler and\nreader.\n\n>>From a code perspective it's pointless. You could \"just\" write:\n>\n> if (something_failed) {\n> error(...);\n> return -1;\n> }\n>\n> which is equivalent. But the point is that the former is shorter and a\n> little more readable, assuming you are familiar with the idiom.\n\nAssuming that.\n\n>> One can always explicitly write\n>> \n>> (config_error_nonbool(\"panic-when-assailed\"), -1)\n>\n> Yes, but again, the point is readability. Doing that at each callsite is\n> ugly and annoying.\n\nI consider insider semantics ugly and annoying. To each his own.\n\n> You are the first person to ask about it, so there is no stock answer.\n> However, everything I told you was in the commit messages and the list\n> archive already. We can also avoid questions being asked by using\n> those tools.\n\nIt's further raising the barriers for contributors if they are expected\nto have studied the last few years in the mailing archive. And the\nskills needed for that kind of study are mostly unrelated to good\nprogramming skills.\n\nSo I am less than convinced that this is among the coding practices that\ncan be expected to attract/scare away potential contributors in\nproportion to their expected capability of advancing/hindering the\nproject.\n\nIt's not me running the shop, so it's nothing more than an opinion.\n\n-- \nDavid Kastrup\n"},{"id":"235039","messageId":"xmqqd2ikasbe.fsf@gitster.dls.corp.google.com","threadId":"35891","inReplyTo":"0120f99a7cccc118782cda63f478d7d3f528ee3f.1392565571.git.john@keeping.me.uk","subject":"Re: [PATCH 5/5] streaming: simplify attaching a filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-18T23:56:21Z","receivedAt":"2014-02-18T23:56:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Keeping writes:\n\n> We are guaranteed that 'nst' is non-null because it is allocated with\n> xmalloc(), and in fact we rely on this three lines later by\n> unconditionally dereferencing it.\n\nThe intent of the original code is for attach_stream_filter() to\ndetect an error condition and return NULL, in which case it closes\nthe istream it allocated and signal error to the caller, I think,\nand falling thru to use st->anything and return st when that happens\nis *not* a guarantee that a-s-f will not detect an error ever, but\nrather is a bug in the error codepath.\n\n>\n> Signed-off-by: John Keeping \n> ---\n> streaming.c | 5 +----\n> 1 file changed, 1 insertion(+), 4 deletions(-)\n>\n> diff --git a/streaming.c b/streaming.c\n> index d7c9f32..8a7135d 100644\n> --- a/streaming.c\n> +++ b/streaming.c\n> @@ -151,10 +151,7 @@ struct git_istream *open_istream(const unsigned char *sha1,\n> \t}\n> \tif (filter) {\n> \t\t/* Add \"&& !is_null_stream_filter(filter)\" for performance */\n> -\t\tstruct git_istream *nst = attach_stream_filter(st, filter);\n> -\t\tif (!nst)\n> -\t\t\tclose_istream(st);\n> -\t\tst = nst;\n> +\t\tst = attach_stream_filter(st, filter);\n> \t}\n> \n> \t*size = st->size;\n"},{"id":"235040","messageId":"xmqq8ut8as1f.fsf@gitster.dls.corp.google.com","threadId":"35891","inReplyTo":"xmqqd2ikasbe.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 5/5] streaming: simplify attaching a filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-19T00:02:20Z","receivedAt":"2014-02-19T00:02:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> John Keeping writes:\n>\n>> We are guaranteed that 'nst' is non-null because it is allocated with\n>> xmalloc(), and in fact we rely on this three lines later by\n>> unconditionally dereferencing it.\n>\n> The intent of the original code is for attach_stream_filter() to\n> detect an error condition and return NULL, in which case it closes\n> the istream it allocated and signal error to the caller, I think,\n> and falling thru to use st->anything and return st when that happens\n> is *not* a guarantee that a-s-f will not detect an error ever, but\n> rather is a bug in the error codepath.\n\nIn other words, something like this instead.\n\n-- >8 --\nSubject: [PATCH] open_istream(): do not dereference NULL in the error case\n\nWhen stream-filter cannot be attached, it is expected to return NULL,\nand we should close the stream we opened and signal an error by\nreturning NULL ourselves from this function.\n\nHowever, we attempted to dereference that NULL pointer between the\npoint we detected the error and returned from the function.\n\nBrought-to-attention-by: John Keeping \nSigned-off-by: Junio C Hamano \n---\n streaming.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/streaming.c b/streaming.c\nindex d7c9f32..2ff036a 100644\n--- a/streaming.c\n+++ b/streaming.c\n@@ -152,8 +152,10 @@ struct git_istream *open_istream(const unsigned char *sha1,\n \tif (filter) {\n \t\t/* Add \"&& !is_null_stream_filter(filter)\" for performance */\n \t\tstruct git_istream *nst = attach_stream_filter(st, filter);\n-\t\tif (!nst)\n+\t\tif (!nst) {\n \t\t\tclose_istream(st);\n+\t\t\treturn NULL;\n+\t\t}\n \t\tst = nst;\n \t}\n \n-- \n1.9.0-279-gdc9e3eb\n"}]}