Page MenuHome GnuPG

gnupg coverity static analysis reports
Open, NormalPublic

Description

I have also a couple of static analysis reports for gnupg. Good sign is that most of them are really just a missing free, but there is a lot of them.

Please, let me know if this format is acceptable or I should do something differently

Details

Version
2.3

Revisions and Commits

Event Timeline

gniibe triaged this task as Normal priority.

Thank you.

I review/apply/push-ed half of them today.

Please note that *_error-from_syserror accesses system's errno which may be cleared by xfree.

Thank you. The initial run was against olderer version of gnupg (and had one issue in g10/keyedit.c -- see the new patch with fixup). Now I ran it against the version 2.3 and there are couple of more issues to be fixed (rebased on top of already applied changes and the previous commits).

There is couple of issues that I did not want to propose a patch for, but might require some attention:

Error: IDENTICAL_BRANCHES (CWE-398): [#def28] [important]
gnupg-2.3.0/common/tlv-builder.c:353: identical_branches: The same code is executed regardless of whether "tag < 31" is true, because the 'then' and 'else' branches are identical. Should one of the branches be modified, or the entire 'if' statement replaced?
#  351|     (void)constructed;  /* Not used, but passed for uniformity of such calls.  */
#  352|   
#  353|->   if (tag < 0x1f)
#  354|       {
#  355|         buflen++;

There are also couple of reports about the function default_homedir(), which is supposed to return const char * but in reality, it sometimes allocates memory while callers do not expect it so they do not free:

Error: RESOURCE_LEAK (CWE-772): [#def11]
gnupg-2.2.27/common/homedir.c:477: alloc_fn: Storage is returned from allocation function "default_homedir".
gnupg-2.2.27/common/homedir.c:477: var_assign: Assigning: "newdir" = storage returned from "default_homedir()".
gnupg-2.2.27/common/homedir.c:488: noescape: Resource "newdir" is not freed or pointed-to in "make_absfilename".
gnupg-2.2.27/common/homedir.c:490: leaked_storage: Returning without freeing "newdir" leaks the storage that it points to.
#  488|     the_gnupg_homedir = make_absfilename (newdir, NULL);;
#  489|     xfree (tmp);
#  490|-> }
#  491|   
#  492|   

Error: RESOURCE_LEAK (CWE-772): [#def12]
gnupg-2.2.27/common/homedir.c:533: alloc_fn: Storage is returned from allocation function "default_homedir".
gnupg-2.2.27/common/homedir.c:533: noescape: Resource "default_homedir()" is not freed or pointed-to in "make_absfilename".
gnupg-2.2.27/common/homedir.c:533: leaked_storage: Failing to save or free storage allocated by "default_homedir()" leaks it.
#  531|     /* If a homedir has not been set, set it to the default.  */
#  532|     if (!the_gnupg_homedir)
#  533|->     the_gnupg_homedir = make_absfilename (default_homedir (), NULL);
#  534|     return the_gnupg_homedir;
#  535|   }

Error: RESOURCE_LEAK (CWE-772): [#def108]
gnupg-2.2.27/g10/trustdb.c:442: alloc_fn: Storage is returned from allocation function "default_homedir".
gnupg-2.2.27/g10/trustdb.c:442: noescape: Assuming resource "default_homedir()" is not freed or pointed-to as ellipsis argument to "log_info".
gnupg-2.2.27/g10/trustdb.c:442: leaked_storage: Failing to save or free storage allocated by "default_homedir()" leaks it.
#  440|   
#  441|     log_info (_("You may try to re-create the trustdb using the commands:\n"));
#  442|->   log_info ("  cd %s\n", default_homedir ());
#  443|     log_info ("  %s --export-ownertrust > otrust.tmp\n", GPG_NAME);
#  444|   #ifdef HAVE_W32_SYSTEM

I think this should return allocated string in all cases for consistency.

This is hopefully last patch-set for minor issues that I would like to get fixed:

@gniibe: If you don't mind I would like to steal task this from you. I have noticed a few things which could get a little code refresh in addition to the fixes.

Regarding the identical branches thing: This is on purpose. The function works closely together with another one which will then BUG() out. @Jakuje: If you know some meta comment to attribute this, please let me know.

Regarding the identical branches thing: This is on purpose. The function works closely together with another one which will then BUG() out. @Jakuje: If you know some meta comment to attribute this, please let me know.

The report can be suppressed (marked as intentional) if you put a comment like this on the line proceeding the branch:

diff --git a/common/tlv-builder.c b/common/tlv-builder.c
index 3b644ca24..59e2691e0 100644
--- a/common/tlv-builder.c
+++ b/common/tlv-builder.c
@@ -350,6 +350,7 @@ get_tlv_length (int class, int tag, int constructed, size_t length)
 
   (void)constructed;  /* Not used, but passed for uniformity of such calls.  */
 
+  /* coverity[identical_branches] */
   if (tag < 0x1f)
     {
       buflen++;

but the problem is that this will work only for coverity and potentially other scanners might complain anyway so maybe human-readable description of the reasoning why it is there would be the best. The same code is in the libksba (libksba-1.5.0/src/ber-help.c).