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.

In T5393#145158, @werner wrote:

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).

@werner No problem. Just go ahead.

FWIW:

In T5393#145098, @gniibe wrote:

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

Actually free (and also xfree) may not modify ERRNO for a a valid pointer or if NULL is passed. If it is an invalid pointer all bets are anyway off. See https://www.austingroupbugs.net/view.php?id=385. POSIX 1.2008 already says

No function in this volume of POSIX.1-2008 shall set errno to 0. The setting of errno after a successful call to a   function is unspecified unless the description of that function specifies that errno shall not be modified.

which can be interpreted to the same effect as the new spec. In particular our xfree implementation gcry_free even has a safeguard for this. However gpgrt_free does not have this but relies on proper semantics of realloc.

But well, I also tend to protect errno over free calls.

The first two patch sets are now applied with the exception of
the gpgsplit fix; I did not applied that patch to add a free() in case of write errors.

I picked the important patches and applied them to 2.2, most are in rGb677e2ec989c4e1d31efba074419c94f8c7c942f

Thank you. I checked what was missing and all looks good. But do not understand why the last gpgsplit xfree was not applied. We are leaving a block where this variable is dynamically allocated so even without error we need to free it.

We ran the coverity again with the new 2.3.1 release and there are couple of new stuff that I probably missed in the initial review.

Most of them are obvious to fix so I am attaching a patch-set, but some of them are not clear to me how they should be handled such as these:

1. gnupg-2.3.1/scd/app-p15.c:685: var_decl: Declaring variable "sw" without initializer.
6. gnupg-2.3.1/scd/app-p15.c:698: uninit_use: Using uninitialized value "sw".
#   696|                        efid_desc, efid, gpg_strerror (err));
#   697|             if (r_sw)
#   698|->             *r_sw = sw;
#   699|             return err;
#   700|           }

This sets the r_sw to uninitialized value of sw, because the failing function does not return the original sw. The question is whether to modify the function to return sw or make up some sw for this use.

I applied most of gnupg-coverity.patch.

• Part 1 is not applied; It should be handled later.
• Part 2: applied
• Part 3: applied
• Part 4: applied, but spell fixes not require ChangeLog entry
• Part 5
  • ecdh part is fixed differently
  • export.c part is not applied for now, because of semantics/interpretation of strncmp; POSIX says differently although it says it's ISO C standard which defines. https://pubs.opengroup.org/onlinepubs/9699919799/functions/strncmp.html
• Part 6: applied
• Part 7: applied, but empty initializer is GNU extension (or the way of C++), so first 0
• Part 8: applied
• Part 9: applied, but one more fix

Under C11, it seems OK (strncmp).
https://stackoverflow.com/questions/38878195/does-this-usage-of-strncmp-contain-an-out-of-bounds-read

We know that problematic strncmp implementation: T5443
So, I don't blame Coverity. But I think that it's better to fix strncmp implementation.

Or, we can use memcmp to avoid arguing semantics of strncmp, and make it a bit cleaner to avoid calling strlen multple times by put_membuf_str.

diff --git a/g10/export.c b/g10/export.c
index 98c4623cf..c7cfcfaa4 100644
--- a/g10/export.c
+++ b/g10/export.c
@@ -2133,14 +2133,15 @@ key_to_sshblob (membuf_t *mb, const char *identifier, ...)
   size_t buflen;
   gcry_mpi_t a;
 
-  ulongtobuf (nbuf, (ulong)strlen (identifier));
+  buflen = strlen (identifier);
+  ulongtobuf (nbuf, (ulong)buflen);
   put_membuf (mb, nbuf, 4);
-  put_membuf_str (mb, identifier);
-  if (!strncmp (identifier, "ecdsa-sha2-", 11))
+  put_membuf (mb, identifier, buflen);
+  if (buflen > 11 && !memcmp (identifier, "ecdsa-sha2-", 11))
     {
-      ulongtobuf (nbuf, (ulong)strlen (identifier+11));
+      ulongtobuf (nbuf, (ulong)(buflen - 11));
       put_membuf (mb, nbuf, 4);
-      put_membuf_str (mb, identifier+11);
+      put_membuf (mb, identifier+11, buflen - 11);
     }
   va_start (arg_ptr, identifier);
   while ((a = va_arg (arg_ptr, gcry_mpi_t)))

I am fine with either way. The memcmp variant is probably cleaner to make sure all works as expected in all cases.

I recently ran a coverity on latest 2.3.3 version which showed couple of new problems I am attaching here. They are just memory leaks or minor issues, but would be great to have them in.

(forgot to upload the patch to the last comment)

Thank you.

all applied.