{"thread":{"id":"21874","subject":"clang static analyzer","startedAt":"2009-12-06T06:11:24Z","lastAt":"2009-12-07T00:26:33Z","messageCount":7,"participants":["Tomas Carnecky","Jeff King","Nicolas Pitre","Nicolas Sebrecht","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"129312","messageId":"33ABC714-2BCC-4910-BCAE-D331AAF2A724@dbservice.com","threadId":"21874","inReplyTo":null,"subject":"clang static analyzer","fromName":"Tomas Carnecky","fromEmail":"tom@dbservice.com","sentAt":"2009-12-06T06:11:24Z","receivedAt":"2009-12-06T06:11:24Z","isPatch":false,"sender":{"key":"tom@dbservice.com","avatar":"https://gravatar.com/avatar/900a300bdd1a8bbe086008ad78210bbee2ad2803b7d50a5cba04c1e9404bd6d2?d=mp&s=160"},"body":"There have been several attempts at running the clang static analyzer on the git source code, some even resulted in patches. I tried it, too, and among the many false positives I think clang found a few real issues. The results can be seen at [1].\n\nClang again found many dead assignments/increments, but in the earlier discussions you concluded that you want to keep those around. So I focussed on another class of potential bugs: Argument with 'nonnull' attribute passed null. There were a total of seven such issues. I then tried to look through the code and see if they are valid or false positives:\n\nxdiff-interface.c:xdiff_set_find_func() - When 'value' is a string with no newline character in it, the loop at line 291 sets 'value' to NULL on its first iteration and then passes 'value' to strchr() in the second iteration.\n\nutf8.c:utf8_strwidth() - 'string' may be set to NULL in utf8_width() which makes this one a false positive.\n\npretty.c:get_header() - if 'line' doesn't contain a newline character, line is set to NULL on first iteration and then passed to strchr() in the second itration.\n\nattr.c:prepare_attr_stack() - bootstrap_attr_stack() sets attr_stack so this one is a false positive as well.\n\ntest-parse-options.c:length_callback() - if arg == NULL and unset == 0 then the function passes NULL to strlen().\n\nbuiltin-pack-objects.c:check_pbase_path() - false positive, if done_pbase_paths == NULL then also done_pbase_paths_alloc == 0 and so step 4 can't take the false branch.\n\nbuiltin-ls-files.c:verify_pathspec() - false positive, pathspec is not NULL when the function is called.\n\n\n- Some of the issues might be purely hypothetical, for example I don't know if it's possible that get_header() can be passed a string with no newlines, maybe this is prevented earlier in the code path.\n- Some of the false positives (such as the last one) could be avoided by giving clang a hint that a certain variable can't be NULL (by using assert() or if (!foo) return).\n\n\ntom\n\n\n[1] http://78.46.209.101/stuff/clang-static-analyzer/git/v1.6.6-rc1-32-g97f3d79/\n"},{"id":"129331","messageId":"20091206145744.GA6725@coredump.intra.peff.net","threadId":"21874","inReplyTo":"33ABC714-2BCC-4910-BCAE-D331AAF2A724@dbservice.com","subject":"Re: clang static analyzer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-06T14:57:44Z","receivedAt":"2009-12-06T14:57:44Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 06, 2009 at 07:11:24AM +0100, Tomas Carnecky wrote:\n\n> Clang again found many dead assignments/increments, but in the earlier\n> discussions you concluded that you want to keep those around. So I\n> focussed on another class of potential bugs: Argument with 'nonnull'\n> attribute passed null. There were a total of seven such issues. I then\n> tried to look through the code and see if they are valid or false\n> positives:\n\nThanks, I think you are moving in the right direction to manually\ninvestigate the output of clang, since it obviously does generate some\nfalse positives.\n\nI think the next step for each site you found would be:\n\n 1. If it really is a problem, then it should be easy to show a simple\n case that can trigger the issue. Submit a patch fixing that site,\n either describing the test case in the commit message, or adding a\n case to the test suite.\n\n 2. If it is a false positive, see what it would take to silence clang\n and submit a patch. I don't think we are opposed to annotations\n that help analysis tools as long as those annotations aren't too\n intrusive or make the code less readable.\n\n-Peff\n"},{"id":"129332","messageId":"alpine.LFD.2.00.0912061032380.31174@xanadu.home","threadId":"21874","inReplyTo":"20091206145744.GA6725@coredump.intra.peff.net","subject":"Re: clang static analyzer","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2009-12-06T15:39:56Z","receivedAt":"2009-12-06T15:39:56Z","isPatch":false,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 6 Dec 2009, Jeff King wrote:\n\n> On Sun, Dec 06, 2009 at 07:11:24AM +0100, Tomas Carnecky wrote:\n> \n> > Clang again found many dead assignments/increments, but in the earlier\n> > discussions you concluded that you want to keep those around. So I\n> > focussed on another class of potential bugs: Argument with 'nonnull'\n> > attribute passed null. There were a total of seven such issues. I then\n> > tried to look through the code and see if they are valid or false\n> > positives:\n> \n> Thanks, I think you are moving in the right direction to manually\n> investigate the output of clang, since it obviously does generate some\n> false positives.\n> \n> I think the next step for each site you found would be:\n> \n> 1. If it really is a problem, then it should be easy to show a simple\n> case that can trigger the issue. Submit a patch fixing that site,\n> either describing the test case in the commit message, or adding a\n> case to the test suite.\n> \n> 2. If it is a false positive, see what it would take to silence clang\n> and submit a patch. I don't think we are opposed to annotations\n> that help analysis tools as long as those annotations aren't too\n> intrusive or make the code less readable.\n\nI'm a bit skeptical here. Going down that route might mean that we'll \neventually have to add all sort of crap to accommodate everyone's \npreferred static analysis tool of the day. Would be far nicer to try to \nmake those tools more intelligent instead, or at least make them \nunderstand an out-of-line annotation format that does not clutter the \ncode itself.\n\n\nNicolas\n"},{"id":"129334","messageId":"20091206160436.GA7140@coredump.intra.peff.net","threadId":"21874","inReplyTo":"alpine.LFD.2.00.0912061032380.31174@xanadu.home","subject":"Re: clang static analyzer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-12-06T16:04:36Z","receivedAt":"2009-12-06T16:04:36Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 06, 2009 at 10:39:56AM -0500, Nicolas Pitre wrote:\n\n> > 2. If it is a false positive, see what it would take to silence clang\n> > and submit a patch. I don't think we are opposed to annotations\n> > that help analysis tools as long as those annotations aren't too\n> > intrusive or make the code less readable.\n> \n> I'm a bit skeptical here. Going down that route might mean that we'll \n> eventually have to add all sort of crap to accommodate everyone's \n> preferred static analysis tool of the day. Would be far nicer to try to \n> make those tools more intelligent instead, or at least make them \n> understand an out-of-line annotation format that does not clutter the \n> code itself.\n\nTo be clear, I am a bit skeptical, too. I would really prefer an\nout-of-line annotation if one is available. But I am trying to encourage\nthe OP to actually make a patch for one instance so we can see just what\nit would look like. Then we actually have a data point to discuss.\n\n-Peff\n"},{"id":"129361","messageId":"20091206234902.GA12932@vidovic","threadId":"21874","inReplyTo":"20091206160436.GA7140@coredump.intra.peff.net","subject":"Re: clang static analyzer","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmx.fr","sentAt":"2009-12-06T23:49:02Z","receivedAt":"2009-12-06T23:49:02Z","isPatch":false,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"The 06/12/09, Jeff King wrote:\n> On Sun, Dec 06, 2009 at 10:39:56AM -0500, Nicolas Pitre wrote:\n> \n> > > 2. If it is a false positive, see what it would take to silence clang\n> > > and submit a patch. I don't think we are opposed to annotations\n> > > that help analysis tools as long as those annotations aren't too\n> > > intrusive or make the code less readable.\n> > \n> > I'm a bit skeptical here. Going down that route might mean that we'll \n> > eventually have to add all sort of crap to accommodate everyone's \n> > preferred static analysis tool of the day. Would be far nicer to try to \n> > make those tools more intelligent instead, or at least make them \n> > understand an out-of-line annotation format that does not clutter the \n> > code itself.\n> \n> To be clear, I am a bit skeptical, too. \n\nMe too. I have no idea about how clang works but if there are enough\nwork done to support such a tool in the code itself, it would be sad to\nnot share and promote this work. If Junio cares enough himself, he could\nset up a public dedicated branch. Now if he doesn't, it's not necessary\nat all. Tomas or anyone else with enough time and motivation can fork\nthe repository for this purpose.\n\nThe latter option would be good and appreciated by everybody here, I\nthink.\n\n-- \nNicolas Sebrecht\n"},{"id":"129363","messageId":"7vtyw3d4cn.fsf@alter.siamese.dyndns.org","threadId":"21874","inReplyTo":"33ABC714-2BCC-4910-BCAE-D331AAF2A724@dbservice.com","subject":"Re: clang static analyzer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-07T00:18:00Z","receivedAt":"2009-12-07T00:18:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tomas Carnecky writes:\n\n> pretty.c:get_header() - if 'line' doesn't contain a newline character,\n> line is set to NULL on first iteration and then passed to strchr() in\n> the second itration.\n\nThanks.\n\nIn practice, we will always have a newline, as we are reading from a valid\ncommit object in this codepath.\n\n\n pretty.c | 5 +++--\n 1 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/pretty.c b/pretty.c\nindex 8f5bd1a..7eb5384 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -245,11 +245,11 @@ static char *get_header(const struct commit *commit, const char *key)\n \tint key_len = strlen(key);\n \tconst char *line = commit->buffer;\n \n-\tfor (;;) {\n+\twhile (line) {\n \t\tconst char *eol = strchr(line, '\\n'), *next;\n \n \t\tif (line == eol)\n-\t\t\treturn NULL;\n+\t\t\tbreak;\n \t\tif (!eol) {\n \t\t\teol = line + strlen(line);\n \t\t\tnext = NULL;\n@@ -262,6 +262,7 @@ static char *get_header(const struct commit *commit, const char *key)\n \t\t}\n \t\tline = next;\n \t}\n+\treturn NULL;\n }\n \n static char *replace_encoding_header(char *buf, const char *encoding)\n"},{"id":"129364","messageId":"7vpr6rd3ye.fsf@alter.siamese.dyndns.org","threadId":"21874","inReplyTo":"33ABC714-2BCC-4910-BCAE-D331AAF2A724@dbservice.com","subject":"Re: clang static analyzer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-12-07T00:26:33Z","receivedAt":"2009-12-07T00:26:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tomas Carnecky writes:\n\n> xdiff-interface.c:xdiff_set_find_func() - When 'value' is a string with\n> no newline character in it, the loop at line 291 sets 'value' to NULL on\n> its first iteration and then passes 'value' to strchr() in the second\n> iteration.\n\nThanks, but I am confused with your analysis.\n\nIf value doesn't have '\\n', then regs->nr is 1 when you go into the loop\nat ll. 291-, because we counted the number of LF in the first loop in the\nfunction.\n\nThe first iteration of the second loop sets ep to NULL, expression is set\nto value, then we run regcomp on the expression. Then at the end of the\nloop we do set value to a bogus \"(char*)1\". But incrementing i makes it\ngo over regs->nr and satisfy the termination condition of the loop; we\nhappily exit the loop before we use the now bogus \"value\".\n"}]}