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 <ebl...@redhat.com>
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 <ebl...@redhat.com>
---
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 <ebl...@redhat.com>
+
+ 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 <meyer...@redhat.com>
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 ebl...@redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
