Re: [PATCH 1/4] Check for null pointers before strcmp. (LLVM/Clang)

[Juan Lang]

Hi Lauri,
     ok(pdst != NULL, "inet_ntoa failed %s\n", dst);
-    ok(!strcmp(pdst, addr0_Str),"Address %s != %s\n", pdst, addr0_Str);
+    ok(pdst && !strcmp(pdst, addr0_Str),"Address %s != %s\n", pdst, addr0_Str);

This change doesn't accomplish anything.  In the first place, the
previous ok which you did not change already ensures pdst is not NULL.
 While it's true that this ok could fail, leading to a following crash
in the tests, the fact that the tests pass indicates this isn't
happening.  Second, the ok output message also dereferences pdst by
printing it, so in the case that it is NULL, you haven't fixed
anything.

Really, I suggest you just ignore this warning, it's not worth the
extra baggage.
--Juan

[PATCH 1/4] Check for null pointers before strcmp. (LLVM/Clang)

[Lauri Kenttä]

On Fri, 10 Jun 2011 14:19:43 -0700, Juan Lang wrote:
> This change doesn't accomplish anything.

Yes it does. It makes Clang happy and avoids a null pointer dereference 
in case the tests are failing. (Even if they're passing now, someone 
might break them in future.)
> In the first place, the previous ok which you did not change already 
> ensures pdst is not NULL.

Yes, I'm sure it does, but that has really nothing to do with this. The 
next line is run in either case, and if pdst is null, it will cause a 
segfault.
> Second, the ok output message also dereferences pdst by printing it

That's not true. "%s" formats null pointers as "(null)" instead of 
dereferencing anything. Feel free to try it yourself or see 
ntdll/printf.c:225 for such a check.
> Really, I suggest you just ignore this warning, it's not worth the 
> extra baggage.

It would be a lot easier to find and fix real things if there weren't a 
thousand false ones hanging around. But if you feel it's better to have 
a lot of warnings and some possible bugs than a lot of checks and no 
bugs, then maybe I'll not waste any more time "fixing" them.

[PATCH 1/4] Check for null pointers before strcmp. (LLVM/Clang)

[Juan Lang]

> It would be a lot easier to find and fix real things if there weren't a
> thousand false ones hanging around. But if you feel it's better to have a
> lot of warnings and some possible bugs than a lot of checks and no bugs,
> then maybe I'll not waste any more time "fixing" them.

This is a false dichotomy.  LLVM/Clang uses static analysis, which is
a sound technique, i.e. it produces no false negatives.  The downside
is that it will always produce false positives.  Filtering out false
positives is a necessary component of any bug fixing exercise using
static analysis.  I'd argue that this is one such example, that the
fix isn't worth the noise, as there's no actual bug being fixed.
--Juan

[PATCH 1/4] Check for null pointers before strcmp. (LLVM/Clang)

[David Laight]

On Sat, Jun 11, 2011 at 01:15:42AM +0300, Lauri Kentt? wrote:
> > Second, the ok output message also dereferences pdst by printing it
>
> That's not true. "%s" formats null pointers as "(null)" instead of 
> dereferencing anything.

That isn't a requirement of the C language, there are many implemenations
which will fault.

David