[libvirt] [PATCH] util: Remove unnecessary ATTRIBUTE_NONNULL for virCommandAddArg[Pair]
Commit 912c6b22 modified the virCommandAddArg and virCommandAddArgPair to perform NULL argument checking; however, no corresponding change to the prototypes was made to remove the ATTRIBUTE_NONNULL, so the Coverity build failed. Adjust the prototypes accordingly. Signed-off-by: John Ferlan <jferlan@redhat.com> --- It's a build breaker for Coverity or anything that enables STATIC_ANALYSIS so I'll push under that rule. src/util/vircommand.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/util/vircommand.h b/src/util/vircommand.h index dbf5041890..100f7a06e0 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -123,7 +123,7 @@ void virCommandAddEnvPassAllowSUID(virCommandPtr cmd, void virCommandAddEnvPassCommon(virCommandPtr cmd); void virCommandAddArg(virCommandPtr cmd, - const char *val) ATTRIBUTE_NONNULL(2); + const char *val); void virCommandAddArgBuffer(virCommandPtr cmd, virBufferPtr buf); @@ -134,8 +134,7 @@ void virCommandAddArgFormat(virCommandPtr cmd, void virCommandAddArgPair(virCommandPtr cmd, const char *name, - const char *val) - ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + const char *val); void virCommandAddArgSet(virCommandPtr cmd, const char *const*vals) ATTRIBUTE_NONNULL(2); -- 2.17.2
On Tue, Dec 18, 2018 at 10:39:20AM -0500, John Ferlan wrote:
Commit 912c6b22 modified the virCommandAddArg and virCommandAddArgPair to perform NULL argument checking; however, no corresponding change to the prototypes was made to remove the ATTRIBUTE_NONNULL, so the Coverity build failed. Adjust the prototypes accordingly.
This sounds wrong or at least different from the past. We have historically still added nonnull annotations even when having checks for NULL in the impl. We had to explicitly disabble the nonnull annotation when building under GCC to prevent it from optimizing out the NULL checks. We left it enabled for STATIC_ANALYSIS so that coverity would still report if any callers passed a NULL into the methods.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- It's a build breaker for Coverity or anything that enables STATIC_ANALYSIS so I'll push under that rule.
src/util/vircommand.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/util/vircommand.h b/src/util/vircommand.h index dbf5041890..100f7a06e0 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -123,7 +123,7 @@ void virCommandAddEnvPassAllowSUID(virCommandPtr cmd, void virCommandAddEnvPassCommon(virCommandPtr cmd);
void virCommandAddArg(virCommandPtr cmd, - const char *val) ATTRIBUTE_NONNULL(2); + const char *val);
void virCommandAddArgBuffer(virCommandPtr cmd, virBufferPtr buf); @@ -134,8 +134,7 @@ void virCommandAddArgFormat(virCommandPtr cmd,
void virCommandAddArgPair(virCommandPtr cmd, const char *name, - const char *val) - ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + const char *val);
void virCommandAddArgSet(virCommandPtr cmd, const char *const*vals) ATTRIBUTE_NONNULL(2);
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 12/18/18 10:47 AM, Daniel P. Berrangé wrote:
On Tue, Dec 18, 2018 at 10:39:20AM -0500, John Ferlan wrote:
Commit 912c6b22 modified the virCommandAddArg and virCommandAddArgPair to perform NULL argument checking; however, no corresponding change to the prototypes was made to remove the ATTRIBUTE_NONNULL, so the Coverity build failed. Adjust the prototypes accordingly.
This sounds wrong or at least different from the past.
I didn't push yet... The last time this happened is/was commit 425aac3abf. Still, before this patch if you build with STATIC_ANALYSIS set you'd see the same phenomena. If I modify config.h to have STATIC_ANALYSIS be 1 instead of 0, I get: util/vircommand.c: In function 'virCommandAddArg': util/vircommand.c:1501:8: error: nonnull argument 'val' compared to NULL [-Werror=nonnull-compare] if (val == NULL) { ^ util/vircommand.c: In function 'virCommandAddArgPair': util/vircommand.c:1604:14: error: nonnull argument 'name' compared to NULL [-Werror=nonnull-compare] if (name == NULL || val == NULL) { ^ util/vircommand.c:1604:29: error: nonnull argument 'val' compared to NULL [-Werror=nonnull-compare] if (name == NULL || val == NULL) { ^ Is there a different way you'd prefer this resolved? I could leave it as a my coverity environment only, but I would assume clang would be impacted too, although I don't use clang.
We have historically still added nonnull annotations even when having checks for NULL in the impl. We had to explicitly disabble the nonnull annotation when building under GCC to prevent it from optimizing out the NULL checks. We left it enabled for STATIC_ANALYSIS so that coverity would still report if any callers passed a NULL into the methods.
That wasn't my recollection. Adding ATTRIBUTE_NONNULL has been "less important" because we found that it really only helps if someone calls some API with NULL and it does not help if the argument itself is NULL. My recollection is always towards removing it once the attribute itself was checked in the API. I also have a faint recollection of a discussion we've had before on this with the thought towards removing all the ATTRIBUTE_NONNULL's and replacing with real argument == NULL checks, but that got to be too many patches and checks in too many places. John
Signed-off-by: John Ferlan <jferlan@redhat.com> --- It's a build breaker for Coverity or anything that enables STATIC_ANALYSIS so I'll push under that rule.
src/util/vircommand.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/util/vircommand.h b/src/util/vircommand.h index dbf5041890..100f7a06e0 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -123,7 +123,7 @@ void virCommandAddEnvPassAllowSUID(virCommandPtr cmd, void virCommandAddEnvPassCommon(virCommandPtr cmd);
void virCommandAddArg(virCommandPtr cmd, - const char *val) ATTRIBUTE_NONNULL(2); + const char *val);
void virCommandAddArgBuffer(virCommandPtr cmd, virBufferPtr buf); @@ -134,8 +134,7 @@ void virCommandAddArgFormat(virCommandPtr cmd,
void virCommandAddArgPair(virCommandPtr cmd, const char *name, - const char *val) - ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); + const char *val);
void virCommandAddArgSet(virCommandPtr cmd, const char *const*vals) ATTRIBUTE_NONNULL(2);
Regards, Daniel
On Tue, Dec 18, 2018 at 11:10:34AM -0500, John Ferlan wrote:
On 12/18/18 10:47 AM, Daniel P. Berrangé wrote:
On Tue, Dec 18, 2018 at 10:39:20AM -0500, John Ferlan wrote:
Commit 912c6b22 modified the virCommandAddArg and virCommandAddArgPair to perform NULL argument checking; however, no corresponding change to the prototypes was made to remove the ATTRIBUTE_NONNULL, so the Coverity build failed. Adjust the prototypes accordingly.
This sounds wrong or at least different from the past.
I didn't push yet...
The last time this happened is/was commit 425aac3abf.
Still, before this patch if you build with STATIC_ANALYSIS set you'd see the same phenomena. If I modify config.h to have STATIC_ANALYSIS be 1 instead of 0, I get:
util/vircommand.c: In function 'virCommandAddArg': util/vircommand.c:1501:8: error: nonnull argument 'val' compared to NULL [-Werror=nonnull-compare] if (val == NULL) { ^ util/vircommand.c: In function 'virCommandAddArgPair': util/vircommand.c:1604:14: error: nonnull argument 'name' compared to NULL [-Werror=nonnull-compare] if (name == NULL || val == NULL) { ^ util/vircommand.c:1604:29: error: nonnull argument 'val' compared to NULL [-Werror=nonnull-compare] if (name == NULL || val == NULL) { ^ Is there a different way you'd prefer this resolved? I could leave it as a my coverity environment only, but I would assume clang would be impacted too, although I don't use clang.
We have historically still added nonnull annotations even when having checks for NULL in the impl. We had to explicitly disabble the nonnull annotation when building under GCC to prevent it from optimizing out the NULL checks. We left it enabled for STATIC_ANALYSIS so that coverity would still report if any callers passed a NULL into the methods.
That wasn't my recollection. Adding ATTRIBUTE_NONNULL has been "less important" because we found that it really only helps if someone calls some API with NULL and it does not help if the argument itself is NULL. My recollection is always towards removing it once the attribute itself was checked in the API. I also have a faint recollection of a discussion we've had before on this with the thought towards removing all the ATTRIBUTE_NONNULL's and replacing with real argument == NULL checks, but that got to be too many patches and checks in too many places.
This goes way back to this commit: commit eefb881d4683d50882b43e5b28b0e94657cd0c9c Author: Laine Stump <laine@laine.org> Date: Wed Apr 25 16:10:01 2012 -0400 build: make ATTRIBUTE_NONNULL() a NOP unless STATIC_ANALYSIS is on The ATTRIBUTE_NONNULL(m) macro normally resolves to the gcc builtin __attribute__((__nonnull__(m))). The effect of this in gcc is unfortunately only to make gcc believe that "m" can never possibly be NULL, *not* to add in any checks to guarantee that it isn't ever NULL (i.e. it is an optimization aid, *not* something to verify code correctness.) - see the following gcc bug report for more details: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 Static source analyzers such as clang and coverity apparently can use ATTRIBUTE_NONNULL(), though, to detect dead code (in the case that the arg really is guaranteed non-NULL), as well as situations where an obviously NULL arg is given to the function. https://bugzilla.redhat.com/show_bug.cgi?id=815270 is a good example of a bug caused by erroneous application of ATTRIBUTE_NONNULL(). Several people spent a long time staring at this code and not finding the problem, because the problem wasn't in the function itself, but in the prototype that specified ATTRIBUTE_NONNULL() for an arg that actually *wasn't* always non-NULL, and caused a segv when dereferenced (even though the code that dereferenced the pointer was inside an if() that checked for a NULL pointer, that code was optimized out by gcc). There may be some very small gain to be had from the optimizations that can be inferred from ATTRIBUTE_NONNULL(), but it seems safer to err on the side of generating code that behaves as expected, while turning on the attribute for static analyzers. We had methods doing "if (foo == NULL) return 0" and had code that was (mistakenly) passing in NULL which would have been safe, except gcc had deleted the "if (foo == NULL)" check. Rather than removing all attribute(nonnull) checks, we merely disabling it under gcc. We left it under STATIC_ANALYSIS so that coverity could still report on places which passed an explicit NULL into a method annotated nonnull. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 12/18/18 11:16 AM, Daniel P. Berrangé wrote:
On Tue, Dec 18, 2018 at 11:10:34AM -0500, John Ferlan wrote:
On 12/18/18 10:47 AM, Daniel P. Berrangé wrote:
On Tue, Dec 18, 2018 at 10:39:20AM -0500, John Ferlan wrote:
Commit 912c6b22 modified the virCommandAddArg and virCommandAddArgPair to perform NULL argument checking; however, no corresponding change to the prototypes was made to remove the ATTRIBUTE_NONNULL, so the Coverity build failed. Adjust the prototypes accordingly.
This sounds wrong or at least different from the past.
I didn't push yet...
The last time this happened is/was commit 425aac3abf.
Still, before this patch if you build with STATIC_ANALYSIS set you'd see the same phenomena. If I modify config.h to have STATIC_ANALYSIS be 1 instead of 0, I get:
util/vircommand.c: In function 'virCommandAddArg': util/vircommand.c:1501:8: error: nonnull argument 'val' compared to NULL [-Werror=nonnull-compare] if (val == NULL) { ^ util/vircommand.c: In function 'virCommandAddArgPair': util/vircommand.c:1604:14: error: nonnull argument 'name' compared to NULL [-Werror=nonnull-compare] if (name == NULL || val == NULL) { ^ util/vircommand.c:1604:29: error: nonnull argument 'val' compared to NULL [-Werror=nonnull-compare] if (name == NULL || val == NULL) { ^ Is there a different way you'd prefer this resolved? I could leave it as a my coverity environment only, but I would assume clang would be impacted too, although I don't use clang.
We have historically still added nonnull annotations even when having checks for NULL in the impl. We had to explicitly disabble the nonnull annotation when building under GCC to prevent it from optimizing out the NULL checks. We left it enabled for STATIC_ANALYSIS so that coverity would still report if any callers passed a NULL into the methods.
That wasn't my recollection. Adding ATTRIBUTE_NONNULL has been "less important" because we found that it really only helps if someone calls some API with NULL and it does not help if the argument itself is NULL. My recollection is always towards removing it once the attribute itself was checked in the API. I also have a faint recollection of a discussion we've had before on this with the thought towards removing all the ATTRIBUTE_NONNULL's and replacing with real argument == NULL checks, but that got to be too many patches and checks in too many places.
This goes way back to this commit:
commit eefb881d4683d50882b43e5b28b0e94657cd0c9c Author: Laine Stump <laine@laine.org> Date: Wed Apr 25 16:10:01 2012 -0400
build: make ATTRIBUTE_NONNULL() a NOP unless STATIC_ANALYSIS is on
The ATTRIBUTE_NONNULL(m) macro normally resolves to the gcc builtin __attribute__((__nonnull__(m))). The effect of this in gcc is unfortunately only to make gcc believe that "m" can never possibly be NULL, *not* to add in any checks to guarantee that it isn't ever NULL (i.e. it is an optimization aid, *not* something to verify code correctness.) - see the following gcc bug report for more details:
And Eric's comment from Apr 2012 eventually answered in Sept 2015 would seem to indicate to me the exact thing that happened in this code and why removing the ATTRIBUTE_NONNULL from the header file would be necessary. Maybe I'm reading it wrong, but it appears to say that if ATTRIBUTE_NONNULL was provided in the prototype and the code did a comparison of the variable to NULL, then some sort of warning would be issued. And that's what I see in the code provided. I really only skimmed and didn't spend too many cycles thinking about it though.
Static source analyzers such as clang and coverity apparently can use ATTRIBUTE_NONNULL(), though, to detect dead code (in the case that the arg really is guaranteed non-NULL), as well as situations where an obviously NULL arg is given to the function.
https://bugzilla.redhat.com/show_bug.cgi?id=815270 is a good example of a bug caused by erroneous application of ATTRIBUTE_NONNULL(). Several people spent a long time staring at this code and not finding the problem, because the problem wasn't in the function itself, but in the prototype that specified ATTRIBUTE_NONNULL() for an arg that actually *wasn't* always non-NULL, and caused a segv when dereferenced (even though the code that dereferenced the pointer was inside an if() that checked for a NULL pointer, that code was optimized out by gcc).
There may be some very small gain to be had from the optimizations that can be inferred from ATTRIBUTE_NONNULL(), but it seems safer to err on the side of generating code that behaves as expected, while turning on the attribute for static analyzers.
We had methods doing "if (foo == NULL) return 0" and had code that was (mistakenly) passing in NULL which would have been safe, except gcc had deleted the "if (foo == NULL)" check. Rather than removing all attribute(nonnull) checks, we merely disabling it under gcc. We left it under STATIC_ANALYSIS so that coverity could still report on places which passed an explicit NULL into a method annotated nonnull.
Um, OK. Well, I've been removing ATTRIBUTE_NONNULL when encountering this same error in the past. It just seemed to be the path of least resistance. After all now regardless of whether its "bar(NULL)" or "bar(foo)" where foo==NULL, if the code in bar does "if (arg1 == NULL) xxx;" then we're covered. In any case, I'll just leave this change in my local Coverity tree and drop this patch. John
On Tue, Dec 18, 2018 at 02:31:27PM -0500, John Ferlan wrote:
On 12/18/18 11:16 AM, Daniel P. Berrangé wrote:
On Tue, Dec 18, 2018 at 11:10:34AM -0500, John Ferlan wrote:
On 12/18/18 10:47 AM, Daniel P. Berrangé wrote:
On Tue, Dec 18, 2018 at 10:39:20AM -0500, John Ferlan wrote:
Commit 912c6b22 modified the virCommandAddArg and virCommandAddArgPair to perform NULL argument checking; however, no corresponding change to the prototypes was made to remove the ATTRIBUTE_NONNULL, so the Coverity build failed. Adjust the prototypes accordingly.
This sounds wrong or at least different from the past.
I didn't push yet...
The last time this happened is/was commit 425aac3abf.
Still, before this patch if you build with STATIC_ANALYSIS set you'd see the same phenomena. If I modify config.h to have STATIC_ANALYSIS be 1 instead of 0, I get:
util/vircommand.c: In function 'virCommandAddArg': util/vircommand.c:1501:8: error: nonnull argument 'val' compared to NULL [-Werror=nonnull-compare] if (val == NULL) { ^ util/vircommand.c: In function 'virCommandAddArgPair': util/vircommand.c:1604:14: error: nonnull argument 'name' compared to NULL [-Werror=nonnull-compare] if (name == NULL || val == NULL) { ^ util/vircommand.c:1604:29: error: nonnull argument 'val' compared to NULL [-Werror=nonnull-compare] if (name == NULL || val == NULL) { ^ Is there a different way you'd prefer this resolved? I could leave it as a my coverity environment only, but I would assume clang would be impacted too, although I don't use clang.
We have historically still added nonnull annotations even when having checks for NULL in the impl. We had to explicitly disabble the nonnull annotation when building under GCC to prevent it from optimizing out the NULL checks. We left it enabled for STATIC_ANALYSIS so that coverity would still report if any callers passed a NULL into the methods.
That wasn't my recollection. Adding ATTRIBUTE_NONNULL has been "less important" because we found that it really only helps if someone calls some API with NULL and it does not help if the argument itself is NULL. My recollection is always towards removing it once the attribute itself was checked in the API. I also have a faint recollection of a discussion we've had before on this with the thought towards removing all the ATTRIBUTE_NONNULL's and replacing with real argument == NULL checks, but that got to be too many patches and checks in too many places.
This goes way back to this commit:
commit eefb881d4683d50882b43e5b28b0e94657cd0c9c Author: Laine Stump <laine@laine.org> Date: Wed Apr 25 16:10:01 2012 -0400
build: make ATTRIBUTE_NONNULL() a NOP unless STATIC_ANALYSIS is on
The ATTRIBUTE_NONNULL(m) macro normally resolves to the gcc builtin __attribute__((__nonnull__(m))). The effect of this in gcc is unfortunately only to make gcc believe that "m" can never possibly be NULL, *not* to add in any checks to guarantee that it isn't ever NULL (i.e. it is an optimization aid, *not* something to verify code correctness.) - see the following gcc bug report for more details:
And Eric's comment from Apr 2012 eventually answered in Sept 2015 would seem to indicate to me the exact thing that happened in this code and why removing the ATTRIBUTE_NONNULL from the header file would be necessary.
Maybe I'm reading it wrong, but it appears to say that if ATTRIBUTE_NONNULL was provided in the prototype and the code did a comparison of the variable to NULL, then some sort of warning would be issued. And that's what I see in the code provided. I really only skimmed and didn't spend too many cycles thinking about it though.
Static source analyzers such as clang and coverity apparently can use ATTRIBUTE_NONNULL(), though, to detect dead code (in the case that the arg really is guaranteed non-NULL), as well as situations where an obviously NULL arg is given to the function.
https://bugzilla.redhat.com/show_bug.cgi?id=815270 is a good example of a bug caused by erroneous application of ATTRIBUTE_NONNULL(). Several people spent a long time staring at this code and not finding the problem, because the problem wasn't in the function itself, but in the prototype that specified ATTRIBUTE_NONNULL() for an arg that actually *wasn't* always non-NULL, and caused a segv when dereferenced (even though the code that dereferenced the pointer was inside an if() that checked for a NULL pointer, that code was optimized out by gcc).
There may be some very small gain to be had from the optimizations that can be inferred from ATTRIBUTE_NONNULL(), but it seems safer to err on the side of generating code that behaves as expected, while turning on the attribute for static analyzers.
We had methods doing "if (foo == NULL) return 0" and had code that was (mistakenly) passing in NULL which would have been safe, except gcc had deleted the "if (foo == NULL)" check. Rather than removing all attribute(nonnull) checks, we merely disabling it under gcc. We left it under STATIC_ANALYSIS so that coverity could still report on places which passed an explicit NULL into a method annotated nonnull.
Um, OK. Well, I've been removing ATTRIBUTE_NONNULL when encountering this same error in the past. It just seemed to be the path of least resistance. After all now regardless of whether its "bar(NULL)" or "bar(foo)" where foo==NULL, if the code in bar does "if (arg1 == NULL) xxx;" then we're covered.
In any case, I'll just leave this change in my local Coverity tree and drop this patch.
What if we also disable the nonnull-compare in case of STATIC_ANALYSIS? That would make perfect sense if what Daniel said is the desired thing to do. However maybe Daniel misread what is written in the commit message. The fact that these functions now _do_ handle NULL, the argument _can_ be NULL, so the attributes should _not_ be there. The scenario only gets weird because the way the NULL is handled here is a call to abort(), which should not be in our code given the way we are handling errors usually. Maybe instead the cmd->error should be set instead. Maybe it should not be there. Maybe I'm completely off track.
John
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Adding Rich, since he recently raised the topic on the libguestfs list and pointed to libvirt as precedence: On 12/18/18 9:47 AM, Daniel P. Berrangé wrote:
On Tue, Dec 18, 2018 at 10:39:20AM -0500, John Ferlan wrote:
Commit 912c6b22 modified the virCommandAddArg and virCommandAddArgPair to perform NULL argument checking; however, no corresponding change to the prototypes was made to remove the ATTRIBUTE_NONNULL, so the Coverity build failed. Adjust the prototypes accordingly.
This sounds wrong or at least different from the past.
We have historically still added nonnull annotations even when having checks for NULL in the impl. We had to explicitly disabble the nonnull annotation when building under GCC to prevent it from optimizing out the NULL checks. We left it enabled for STATIC_ANALYSIS so that coverity would still report if any callers passed a NULL into the methods.
Looking at the gcc bug and following some of the links mentioned therein, https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01103.html It looks like modern gcc has added -Wnonnull-compare that noisily informs us any time we have an ATTRIBUTE_NONNULL mismatch with the body of a function comparing that parameter against NULL after all. And gnulib's manywarnings module turns that on automatically (at least for gcc 8.2.1 on Fedora 29). That is sufficient to fix the bug that led us to historically disable ATTRIBUTE_NONNULL under gcc (commit eefb881), so maybe it's time to just blindly enable ATTRIBUTE_NONNULL everywhere on the grounds that we have enough developers and CI coverage to now immediately catch inconsistent attributes rather than risking silently mis-compiled code that omits the branch after a NULL comparison as dead code. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On 1/2/19 2:37 PM, Eric Blake wrote:
We have historically still added nonnull annotations even when having checks for NULL in the impl. We had to explicitly disabble the nonnull annotation when building under GCC to prevent it from optimizing out the NULL checks. We left it enabled for STATIC_ANALYSIS so that coverity would still report if any callers passed a NULL into the methods.
Looking at the gcc bug and following some of the links mentioned therein,
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01103.html
It looks like modern gcc has added -Wnonnull-compare that noisily informs us any time we have an ATTRIBUTE_NONNULL mismatch with the body of a function comparing that parameter against NULL after all. And gnulib's manywarnings module turns that on automatically (at least for gcc 8.2.1 on Fedora 29).
That is sufficient to fix the bug that led us to historically disable ATTRIBUTE_NONNULL under gcc (commit eefb881), so maybe it's time to just blindly enable ATTRIBUTE_NONNULL everywhere on the grounds that we have enough developers and CI coverage to now immediately catch inconsistent attributes rather than risking silently mis-compiled code that omits the branch after a NULL comparison as dead code.
Also worth considering: https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/... mentions that we can use -fsanitize=nonnull -fsanitize=returns-nonnull (both parts of the larger -fsanitize=undefined) for runtime checking of cases that sneak into the code in spite of the compile-time checking. gnulib's manywarnings module does not currently enable -fsanitize automatically, so we'd have to do a bit more work if we want to take advantage of that (there's also a question of how much runtime slowdown happens any time we run with -fsanitize= turned on, to the point where it may be something useful during CI testing but not in general) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Wed, Jan 02, 2019 at 02:37:00PM -0600, Eric Blake wrote:
Adding Rich, since he recently raised the topic on the libguestfs list and pointed to libvirt as precedence:
On 12/18/18 9:47 AM, Daniel P. Berrangé wrote:
On Tue, Dec 18, 2018 at 10:39:20AM -0500, John Ferlan wrote:
Commit 912c6b22 modified the virCommandAddArg and virCommandAddArgPair to perform NULL argument checking; however, no corresponding change to the prototypes was made to remove the ATTRIBUTE_NONNULL, so the Coverity build failed. Adjust the prototypes accordingly.
This sounds wrong or at least different from the past.
We have historically still added nonnull annotations even when having checks for NULL in the impl. We had to explicitly disabble the nonnull annotation when building under GCC to prevent it from optimizing out the NULL checks. We left it enabled for STATIC_ANALYSIS so that coverity would still report if any callers passed a NULL into the methods.
Looking at the gcc bug and following some of the links mentioned therein,
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01103.html
It looks like modern gcc has added -Wnonnull-compare that noisily informs us any time we have an ATTRIBUTE_NONNULL mismatch with the body of a function comparing that parameter against NULL after all. And gnulib's manywarnings module turns that on automatically (at least for gcc 8.2.1 on Fedora 29).
That is sufficient to fix the bug that led us to historically disable ATTRIBUTE_NONNULL under gcc (commit eefb881), so maybe it's time to just blindly enable ATTRIBUTE_NONNULL everywhere on the grounds that we have enough developers and CI coverage to now immediately catch inconsistent attributes rather than risking silently mis-compiled code that omits the branch after a NULL comparison as dead code.
Yes & no. Originally the desire was that we would have functions which were marked NONNULL and *also* have "if (foo == NULL) ... return..' as a second safety net. IOW, we were aiming to get *both* compile time and runtime checking for NULL, since compile time checks were unsufficiently safe on their own. The compiler was "clever" and optimized out the runtime checks, leaving only the incomplete compile time checks. We compromised and only set __attribute__(nonnull) under coverity. The idea was that coverity would get us the static checks as a substiute for the compile time checks, while we still have the runtime checks. Using -Wnonnull-compare would detect when gcc did this dangerous optimization, but it would still force us to either remove either the compile time check, or the runtime check. I do wonder if we can enable __attribute__(nonnull) under GCC and then set '-fdelete-null-pointer-checks' to stop it deleting our runtime NULL checks, so we get both compile time & runtime protection. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 1/10/19 5:23 AM, Daniel P. Berrangé wrote:
We have historically still added nonnull annotations even when having checks for NULL in the impl. We had to explicitly disabble the nonnull annotation when building under GCC to prevent it from optimizing out the NULL checks. We left it enabled for STATIC_ANALYSIS so that coverity would still report if any callers passed a NULL into the methods.
Looking at the gcc bug and following some of the links mentioned therein,
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=17308 https://gcc.gnu.org/ml/gcc-patches/2016-02/msg01103.html
It looks like modern gcc has added -Wnonnull-compare that noisily informs us any time we have an ATTRIBUTE_NONNULL mismatch with the body of a function comparing that parameter against NULL after all. And gnulib's manywarnings module turns that on automatically (at least for gcc 8.2.1 on Fedora 29).
That is sufficient to fix the bug that led us to historically disable ATTRIBUTE_NONNULL under gcc (commit eefb881), so maybe it's time to just blindly enable ATTRIBUTE_NONNULL everywhere on the grounds that we have enough developers and CI coverage to now immediately catch inconsistent attributes rather than risking silently mis-compiled code that omits the branch after a NULL comparison as dead code.
Yes & no.
Originally the desire was that we would have functions which were marked NONNULL and *also* have "if (foo == NULL) ... return..' as a second safety net.
No, I think that any case where we had 'if (foo == NULL)' but didn't change the attribute was not a safety net, but a mismatch in declared intentions because of the gcc bug not informing us about the mismatch.
IOW, we were aiming to get *both* compile time and runtime checking for NULL, since compile time checks were unsufficiently safe on their own.
The compiler was "clever" and optimized out the runtime checks, leaving only the incomplete compile time checks.
The compiler's bug was not that it optimized out the safety checks because we had a buggy attribute, but that it did not warn us that it was optimizing out the safety checks so that we could fix our buggy attribute. But that bug has been fixed.
We compromised and only set __attribute__(nonnull) under coverity. The idea was that coverity would get us the static checks as a substiute for the compile time checks, while we still have the runtime checks.
Coverity was able to warn about attribute mismatch even where older gcc wasn't able to, so having the attribute set under coverity let us flag our bugs where we had attribute mismatch to fix them, while avoiding the mis-compiled code where gcc optimized without warning. But now that gcc no longer optimizes without warning, we can rely on gcc instead of coverity to flag the mismatch, which is a much lower barrier of entry.
Using -Wnonnull-compare would detect when gcc did this dangerous optimization, but it would still force us to either remove either the compile time check, or the runtime check.
Which is correct. Either the compile-time check is correct (and we don't need the runtime check - if the programmer passes in NULL in spite of the attribute, they deserve the resulting crash), or the attribute is wrong (if we are doing a runtime check, then the function should NOT have the attribute).
I do wonder if we can enable __attribute__(nonnull) under GCC and then set '-fdelete-null-pointer-checks' to stop it deleting our runtime NULL checks, so we get both compile time & runtime protection.
I think that leads to the more confusing situation where we are documenting something that is not true. If we're going to handle NULL, then there's no point in telling users not to rely on that. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Thu, Jan 10, 2019 at 09:23:06AM -0600, Eric Blake wrote:
I do wonder if we can enable __attribute__(nonnull) under GCC and then set '-fdelete-null-pointer-checks' to stop it deleting our runtime NULL checks, so we get both compile time & runtime protection.
I think that leads to the more confusing situation where we are documenting something that is not true. If we're going to handle NULL, then there's no point in telling users not to rely on that.
For any pointer parameter there's then two possibilities. Either NULL is a valid value and results in some useful behaviour, or NULL is not valid and libvirt will catch that and report an error. The latter are essentially all programmer bugs, but we wanted to be robust and catch them at runtime & report graceful error instead of referencing NULL or assert()ing. We added ATTRIBUTE_NONNULL widely to try to catch the latter at compile time too. This also served as documentation in the header files to indicate that you shouldn't be passing NULL for that parameter. We later discovered our usage of ATTRIBUTE_NONNULL was based on a mis-understanding of the functional impact of that annotation, and so #defined it to a no-op, so it merely served as documentation that you should not pass NULL to a method. It is still desirable for us to find a way to do all three tasks 1. document that NULL should not be passed for a param 2. detect mistakes passing NULL via static analysis 3. retain runtime graceful NULL param checking. AFAICT we have a choice either a. Replace ATTRIBUTE_NONNULL to just a plain comment /* nonnull */ against each parameter. This covers items 1 & 2, mostly as we do today, while stopping coverity getting upset b. Re-enable ATTRIBUTE_NONNULL to expand to attribute(nonnull) but only under modern GCC, and add -fdelete-null-pointer-checks This covers items 1, 2 & 3, under GCC, while stopping coverity getting upset Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 1/10/19 9:39 AM, Daniel P. Berrangé wrote:
For any pointer parameter there's then two possibilities. Either NULL is a valid value and results in some useful behaviour, or NULL is not valid and libvirt will catch that and report an error.
But within the latter are two cases: libvirt catches the error gracefully, or libvirt crashes.
The latter are essentially all programmer bugs, but we wanted to be robust and catch them at runtime & report graceful error instead of referencing NULL or assert()ing. We added ATTRIBUTE_NONNULL widely to try to catch the latter at compile time too. This also served as documentation in the header files to indicate that you shouldn't be passing NULL for that parameter.
We later discovered our usage of ATTRIBUTE_NONNULL was based on a mis-understanding of the functional impact of that annotation, and so #defined it to a no-op, so it merely served as documentation that you should not pass NULL to a method.
It is still desirable for us to find a way to do all three tasks
1. document that NULL should not be passed for a param 2. detect mistakes passing NULL via static analysis 3. retain runtime graceful NULL param checking.
AFAICT we have a choice either
a. Replace ATTRIBUTE_NONNULL to just a plain comment /* nonnull */ against each parameter. This covers items 1 & 2, mostly as we do today, while stopping coverity getting upset
b. Re-enable ATTRIBUTE_NONNULL to expand to attribute(nonnull) but only under modern GCC, and add -fdelete-null-pointer-checks This covers items 1, 2 & 3, under GCC, while stopping coverity getting upset
Or go with two separate macros: One for use on internal files, where when we mark something non-NULL, it is a programming bug in libvirt for the caller for passing NULL, or a programming bug in the implementation for using the attribute and then checking for NULL after all. Let gcc/coverity always flag these violations as bugs, and don't bother worrying about safety nets because we trust our callers of internal interfaces for knowing the semantics. The other for use in public headers, where the public gets good documentation that passing NULL is nonsense, and even gets the attribute enabled if they compile with gcc in order to help them avoid bad code. But when compiling internally, we disable the macro (similarly to how our headers use VIR_ENUM_SENTINELS to control whether the VIR_*_LAST enum members are present for internal use but absent for the public); that way, our implementations don't see the attribute and can code in the safety-net runtime check to handle bad users that disobeyed the warning, without crashing and without our NULL checks being optimized out. I'm still not sold that we want -fdelete-null-pointer-checks, but on the other hand, how much overhead does it add? -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
On Thu, Jan 10, 2019 at 10:23:13AM -0600, Eric Blake wrote:
On 1/10/19 9:39 AM, Daniel P. Berrangé wrote:
For any pointer parameter there's then two possibilities. Either NULL is a valid value and results in some useful behaviour, or NULL is not valid and libvirt will catch that and report an error.
But within the latter are two cases: libvirt catches the error gracefully, or libvirt crashes.
The latter are essentially all programmer bugs, but we wanted to be robust and catch them at runtime & report graceful error instead of referencing NULL or assert()ing. We added ATTRIBUTE_NONNULL widely to try to catch the latter at compile time too. This also served as documentation in the header files to indicate that you shouldn't be passing NULL for that parameter.
We later discovered our usage of ATTRIBUTE_NONNULL was based on a mis-understanding of the functional impact of that annotation, and so #defined it to a no-op, so it merely served as documentation that you should not pass NULL to a method.
It is still desirable for us to find a way to do all three tasks
1. document that NULL should not be passed for a param 2. detect mistakes passing NULL via static analysis 3. retain runtime graceful NULL param checking.
AFAICT we have a choice either
a. Replace ATTRIBUTE_NONNULL to just a plain comment /* nonnull */ against each parameter. This covers items 1 & 2, mostly as we do today, while stopping coverity getting upset
b. Re-enable ATTRIBUTE_NONNULL to expand to attribute(nonnull) but only under modern GCC, and add -fdelete-null-pointer-checks This covers items 1, 2 & 3, under GCC, while stopping coverity getting upset
Or go with two separate macros: One for use on internal files, where when we mark something non-NULL, it is a programming bug in libvirt for the caller for passing NULL, or a programming bug in the implementation for using the attribute and then checking for NULL after all. Let gcc/coverity always flag these violations as bugs, and don't bother worrying about safety nets because we trust our callers of internal interfaces for knowing the semantics.
I think this is the key point. I do *not* trust us to know the semantics of our internal interfaces. Developers are fallible, so while we can expect we'll get it right 95% of the time, we're inevitably going to pass NULL to methods which don't want it. We must have the runtime safety net to catch this gracefully, and ideally have static checks to catch some of it at build time (though the latter is never going to be perfect as it needs non-trivial control-flow data analysis).
I'm still not sold that we want -fdelete-null-pointer-checks, but on the other hand, how much overhead does it add?
It disables constant folding optimizations, and stops gcc from deleting "useless" null pointer checks. It isn't doing instrumentation like the -fsanitize args do, so I don't think it should have significant overhead. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (4)
-
Daniel P. Berrangé -
Eric Blake -
John Ferlan -
Martin Kletzander