Re: [PATCH] vasnprintf: silence some clang false positives

[Eric Blake]

On 02/14/2011 07:11 PM, Bruno Haible wrote:
> Hi Eric,
> 
>> Bruno, would you be okay with this patch?
> 
>> +    * lib/vasnprintf.c (VASNPRINTF) [ENSURE_ALLOCATION]: Teach clang
>> +    that ENSURE_ALLOCATION guarantees a non-null result.
> 
>> +    else if (!result)                                                       
>>  \
>> +      abort ()
> 
> No, this patch is wrong. ENSURE_ALLOCATION does not guarantee a non-NULL
> 'result'. In fact, in the invocations at lines 2060, 2094, 2188, 2222, 2316,
> 2350, 2565, 2820, 2880, 3379, 4565, 5321, 5347, 5353, 5358, the 'needed'
> argument may be 0, and when at the same time the 'allocated' variable is
> also 0, the 'result' will be NULL. You patch would add an invocation of
> abort() in these cases.

OK, so the real fix is to add annotations at the three places that clang
flagged as potential NULL dereferences, rather than changing
ENSURE_ALLOCATION [shown here using abort, but see below]:

From 7501dd6520b2fd639286004e63ce1d0f84523798 Mon Sep 17 00:00:00 2001
From: Eric Blake <address@hidden>
Date: Mon, 14 Feb 2011 15:51:58 -0700
Subject: [PATCH] vasnprintf: silence some clang false positives

Clang missed the fact that when ENSURE_ALLOCATION is called with
a guaranteed non-zero value, then result is guaranteed non-NULL
after that point.  Adding some conditionals fix the analysis.

* lib/vasnprintf.c (VASNPRINTF): Teach clang when
ENSURE_ALLOCATION guarantees a non-null result.

Signed-off-by: Eric Blake <address@hidden>
---
 ChangeLog        |    6 ++++++
 lib/vasnprintf.c |    4 ++++
 2 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index 8e14a2b..d639fe2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2011-02-14  Eric Blake  <address@hidden>
+
+       vasnprintf: silence some clang false positives
+       * lib/vasnprintf.c (VASNPRINTF): Teach clang when
+       ENSURE_ALLOCATION guarantees a non-null result.
+
 2011-02-15  Jim Meyering  <address@hidden>

        doc: update users.txt
diff --git a/lib/vasnprintf.c b/lib/vasnprintf.c
index 8f07308..19da825 100644
--- a/lib/vasnprintf.c
+++ b/lib/vasnprintf.c
@@ -1847,6 +1847,8 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,
             size_t augmented_length = xsum (length, n);

             ENSURE_ALLOCATION (augmented_length);
+            if (!result)
+              abort ();
             /* This copies a piece of FCHAR_T[] into a DCHAR_T[].  Here we
                need that the format string contains only ASCII characters
                if FCHAR_T and DCHAR_T are not the same type.  */
@@ -5517,6 +5519,8 @@ VASNPRINTF (DCHAR_T *resultbuf, size_t *lengthp,

     /* Add the final NUL.  */
     ENSURE_ALLOCATION (xsum (length, 1));
+    if (!result)
+      abort ();
     result[length] = '\0';

     if (result != resultbuf && length + 1 < allocated)
---
1.7.4



>   2) There will always be situations where the tool cannot determine the
>      invariants of a program automatically. For these cases, the language
>      needs a way to assert invariants. What is the way to assert invariants
>      that clang understands? Is 'assert(condition)' or
>      'if (!condition) abort();' enough?

Yes, 'assert(cond)' (when NDEBUG is not defined), or 'if (!cond)
abort();', are both equally good at informing any decent static analysis
tool about an invariant.

In fact, here's how the libvirt project does it:

In configure.ac:

# Detect when running under the clang static analyzer's scan-build driver
# or Coverity-prevent's cov-build.  Define STATIC_ANALYSIS accordingly.
test -n "$CCC_ANALYZER_ANALYSIS$COVERITY_BUILD_COMMAND" && t=1 || t=0
AC_DEFINE_UNQUOTED([STATIC_ANALYSIS], [$t],
  [Define to 1 when performing static analysis.])

then in a common header:

# if STATIC_ANALYSIS
#  undef NDEBUG /* Don't let a prior NDEBUG definition cause trouble.  */
#  include <assert.h>
#  define sa_assert(expr) assert (expr)
# else
#  define sa_assert(expr) /* empty */
# endif

Then anywhere that clang or coverity or other tools need help, then
sa_assert(cond) works as a no-op for normal compilation while still
providing the extra info needed to avoid false positives from the static
analysis.

> So, what I'd like to see is a standard way to declare invariants (assertions)
> in a way that clang and other static analysis tools (maybe GCC in the future?)
> could understand.

Should we create a gnulib module that defines sa_assert() automatically?

> Of course, these declarations should have no negative
> impact on the speed of the program. (When you declare types in CL or C#, it
> also never degrades the performance.)

Correct - those assertions are only defined when doing static analysis,
and are not present in normal compilation and therefore cannot degrade
normal performance.

-- 
Eric Blake   address@hidden    +1-801-349-2682
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature