[PATCH] util: remove ATTRIBUTE_NONNULL from virDirClose declaration

Laine Stump posted 1 patch 2 weeks, 2 days ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20201113174227.2544422-1-laine@redhat.com
src/util/virfile.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

[PATCH] util: remove ATTRIBUTE_NONNULL from virDirClose declaration

Posted by Laine Stump 2 weeks, 2 days ago
Before commit 24d8968c, virDirClose took a DIR**, and that was never
NULL, so its declaration included ATTRIBUTE_NONNULL(1). Since that
commit, virDirClose takes a DIR*, and it may be NULL (e.g. if the DIR*
is initialized to NULL and was never closed).

Even though virDirClose() is currently only called implicitly (as the
cleanup for a g_autoptr(DIR)), and (as I've just newly learned) the
autocleanup function g_autoptr will only be called if the pointer in
question is non-null (see the definition of
_GLIB_AUTOPTR_CLEAR_FUNC_NAME in
/usr/include/glib-2.0/glib/gmacros.h), it does still cause Coverity to
complain that it *could* be called with a NULL, and it's also possible
that in the future someone might add code that explicitly calls
virDirClose.

To eliminate the Coverity complaints, and protect against the
hypothetical future where someone both explicitly calls virDirClose()
with a potentially NULL value, *and* re-enables the nonnull directive
when not building with Coverity (disabled by commit eefb881) this
patch removes the ATTRIBUTE_NONNULL(1) from the declaration of
virDirClose().

Fixes: 24d8968cd0a718af4badbbc858b1b449fea7205a
Details-Research-by: Dan Berrange <berrange@redhat.com>
Signed-off-by: Laine Stump <laine@redhat.com>
---
 src/util/virfile.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/util/virfile.h b/src/util/virfile.h
index ba31a78e58..32505826ee 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -269,8 +269,7 @@ int virDirOpenQuiet(DIR **dirp, const char *dirname)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
 int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname)
     ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
-void virDirClose(DIR *dirp)
-    ATTRIBUTE_NONNULL(1);
+void virDirClose(DIR *dirp);
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(DIR, virDirClose);
 
 int virFileMakePath(const char *path) G_GNUC_WARN_UNUSED_RESULT;
-- 
2.28.0

Re: [PATCH] util: remove ATTRIBUTE_NONNULL from virDirClose declaration

Posted by Laine Stump 2 weeks, 2 days ago
After short discussion on IRC, I pushed this as "trivial".

On 11/13/20 12:42 PM, Laine Stump wrote:
> Before commit 24d8968c, virDirClose took a DIR**, and that was never
> NULL, so its declaration included ATTRIBUTE_NONNULL(1). Since that
> commit, virDirClose takes a DIR*, and it may be NULL (e.g. if the DIR*
> is initialized to NULL and was never closed).
>
> Even though virDirClose() is currently only called implicitly (as the
> cleanup for a g_autoptr(DIR)), and (as I've just newly learned) the
> autocleanup function g_autoptr will only be called if the pointer in
> question is non-null (see the definition of
> _GLIB_AUTOPTR_CLEAR_FUNC_NAME in
> /usr/include/glib-2.0/glib/gmacros.h), it does still cause Coverity to
> complain that it *could* be called with a NULL, and it's also possible
> that in the future someone might add code that explicitly calls
> virDirClose.
>
> To eliminate the Coverity complaints, and protect against the
> hypothetical future where someone both explicitly calls virDirClose()
> with a potentially NULL value, *and* re-enables the nonnull directive
> when not building with Coverity (disabled by commit eefb881) this
> patch removes the ATTRIBUTE_NONNULL(1) from the declaration of
> virDirClose().
>
> Fixes: 24d8968cd0a718af4badbbc858b1b449fea7205a
> Details-Research-by: Dan Berrange <berrange@redhat.com>
> Signed-off-by: Laine Stump <laine@redhat.com>
> ---
>   src/util/virfile.h | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/util/virfile.h b/src/util/virfile.h
> index ba31a78e58..32505826ee 100644
> --- a/src/util/virfile.h
> +++ b/src/util/virfile.h
> @@ -269,8 +269,7 @@ int virDirOpenQuiet(DIR **dirp, const char *dirname)
>       ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
>   int virDirRead(DIR *dirp, struct dirent **ent, const char *dirname)
>       ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) G_GNUC_WARN_UNUSED_RESULT;
> -void virDirClose(DIR *dirp)
> -    ATTRIBUTE_NONNULL(1);
> +void virDirClose(DIR *dirp);
>   G_DEFINE_AUTOPTR_CLEANUP_FUNC(DIR, virDirClose);
>   
>   int virFileMakePath(const char *path) G_GNUC_WARN_UNUSED_RESULT;