automated cppcheck for gnupg
Hans-Christoph Steiner
hans at guardianproject.info
Wed Apr 16 12:50:53 CEST 2014
On 04/16/2014 04:15 AM, Werner Koch wrote:
> On Tue, 15 Apr 2014 23:44, hans at guardianproject.info said:
>
>> I have seen cppcheck find new issues when fixing seemingly trivial ones like
>> this. Plus it makes automated reporting a lot easier if the trivial errors
>> are fixed.
>
> By definition of C stdio streams are closed at process end (or well
> after returning from main).
>
>>> Just checked it. It is pretty clear the HD is initialzied and has not
>>> been deinitialized. But please prove me wrong.
>>
>> Same as above, making cppcheck happy on things like this helps it find other
>> errors. (That's my experience, I don't have any other specific knowledge).
>
> Sorry, I have no idea on how to do that. This is prety sstandard code
> and I actually fail to see why cppcheck complains about it.
>
>>>> libgcrypt/cipher/md.c:976
>>>
>>>> nullPointer Possible null pointer dereference: r - otherwise it
>>>> is redundant to check if r is null at line 971
>>>
>>> Code not found.
>>
>> This is from LIBGCRYPT-1-6-BRANCH since its the GPG-Android build.
>
> Okay, I was on master but the code is identuical. Let's see
>
> 969: GcryDigestEntry *r = a->ctx->list;
> 970:
> 971: if (r && r->next)
> 972: {
> 973: fips_signal_error ("possible usage error");
> 974: log_error ("WARNING: more than one algorithm in md_get_algo()\n");
> 975: }
> 976: return r ? r->spec->algo : 0;
>
> On 971 I check a condition whether to print a warning.
> On 971 I return the algo or 0.
>
> All fine.
>
>> It was a quick sample, here is the whole list from master on all projects but
>> libgcrypt, where it is on LIBGCRYPT-1-6-BRANCH:
>
> Let's see
>
>> resourceLeak
>> ------------
>> gnupg/tools/symcryptrun.c:380 Resource leak: in
>> gnupg/kbx/keybox-dump.c:679 Resource leak: fp
>> gnupg/tools/symcryptrun.c:420 Resource leak: out
>> libassuan/src/mkheader.c:251 Resource leak: fp
>> libgcrypt/tests/fipsdrv.c:2525 Resource leak: input
>> libgcrypt/tests/rsacvt.c:426 Resource leak: input
>> libgpg-error/src/mkheader.c:426 Resource leak: fp
>
> These are all helper programs and they do not leak anything.
>
>> memleak
>> -------
>> gpgme/src/w32-io.c:797 Memory leak: ctx
>> gpgme/src/w32-io.c:428 Memory leak: ctx
>> gpgme/src/w32-util.c:713 Memory leak: tmpname
>
> I fixed the one at 428.
>
>> gpgme/src/engine-gpg.c:904 Memory leak: fd_data_map
>
> Hard to fix. I added this comment:
>
> /* We leak fd_data_map and the fds. This is not easy
> to avoid and given that we reach this here only
> after a malloc failure for a small object, it is
> probably better not to do anything. */
>
>> gpgme/src/gpgme-tool.c:2621 Memory leak: filename
>> gpgme/src/gpgme-tool.c:2644 Memory leak: filename
>> gpgme/src/gpgme-tool.c:2668 Memory leak: filename
>
> No leaks there. server_parse_fd (ctx, line, &sysfd, &filename)
> guarantees that NULL is stored at FILENAME on error.
>
>> uninitvar
>> ---------
>> gnupg/keyserver/gpgkeys_ldap.c:897
>> Uninitialized variable: keyid
>> gnupg/keyserver/gpgkeys_hkp.c:235
>> Uninitialized variable: keyid
>
> Not sure which version of GnuPG. 2.1 does not use code in keyserver,
> anyway.
>
>> gpgme/src/data.c:255
>> Uninitialized variable: buffer
>
> Sure, BUFFER is initialzied:
>
> char buffer[BUFFER_SIZE];
> char *bufp = buffer;
>
>
>> nullPointer
>> -----------
>> gnupg/tools/sockprox.c:446
>> Possible null pointer dereference: protocol_file
>
> Fixed.
>
>> gnupg/tests/asschk.c:1088
>> Possible null pointer dereference: p - otherwise it is redundant to check if
>> p is null at line 1086
>
> Nope, that is correct:
>
> if (!p)
> die_0 ("incomplete script line");
> *p = 0;
>
> cppcheck needs to know more about the semantics of functions.
>
>> gnupg/sm/server.c:126
>> Possible null pointer dereference: s - otherwise it is redundant to check if
>> s is null at line 124
>
> 124: if (s && s >= skip_options (line))
> 125: return 0;
> 126: return (s && (s == line || spacep (s-1)) && (!s[n] || spacep (s+n)));
>
> Sorry, I can't see a problem.
>
>> gnupg/kbx/keybox-blob.c:950
>> Possible null pointer dereference: blob - otherwise it is redundant to check
>> if blob is null at line 951
>
> Right, the check is redunant. No NULL deref, though.
>
>
>> gnupg/scd/apdu.c:4200
>> Possible null pointer dereference: p
>
> Nope:
>
> *retbuf = p = xtrymalloc (bufsize + 2);
> if (!*retbuf)
> {
> unlock_slot (slot);
> xfree (result_
);
> return SW_HOST_OUT_OF_CORE;
> }
> assert (resultlen < bufsize);
> 4200: memcpy (p, result, resultlen);
>
> P has already been checked above.
>
>> gnupg/g10/server.c:99
>> Possible null pointer dereference: s - otherwise it is redundant to check if
>> s is null at line 97
>
> Nope, same as above in sm.server.c
>
>> gnupg/g10/keylist.c:825
>> Possible null pointer dereference: pk - otherwise it is redundant to check if
>> pk is null at line 886
>> gnupg/g10/keylist.c:837
>> Possible null pointer dereference: pk - otherwise it is redundant to check if
>> pk is null at line 886
>> gnupg/g10/keylist.c:843
>> Possible null pointer dereference: pk - otherwise it is redundant to check if
>> pk is null at line 886
>> gnupg/g10/keylist.c:849
>> Possible null pointer dereference: pk - otherwise it is redundant to check if
>> pk is null at line 886
>
>
> Consider them an assert (pk);
>
>> gnupg/g10/keyid.c:369
>> Possible null pointer dereference: sub_pk - otherwise it is redundant to
>> check if sub_pk is null at line 366
>
> Huh?
>
> keyid_from_pk (main_pk, NULL);
> if (sub_pk)
> keyid_from_pk (sub_pk, NULL);
>
> return keystr_with_sub (main_pk->keyid, sub_pk? sub_pk->keyid:NULL);
>
> That looks pretty right.
>
>
>> gnupg/common/estream.c:3106
>> Possible null pointer dereference: stream - otherwise it is redundant to
>> check if stream is null at line 3104
>
> Okay. I removed the redunant check.
>
>
>> gnupg/agent/command.c:285
>> Possible null pointer dereference: s - otherwise it is redundant to check if
>> s is null at line 282
>
> Nope. Similars in sm/server.c and g10/server.c.
>
>> gnupg/agent/command.c:268
>> Possible null pointer dereference: s - otherwise it is redundant to check if
>> s is null at line 266
>
> Ditto.
>
>
>> libassuan/src/assuan.c:136
>> Possible null pointer dereference: ctx - otherwise it is redundant to check
>> if ctx is null at line 135
>
> Mean while fixed.
>
>> libgcrypt/cipher/md.c:976
>> Possible null pointer dereference: r - otherwise it is redundant to check if
>> r is null at line 971
>
> See above.
>
>> libgcrypt/cipher/md.c:1251
>> Possible null pointer dereference: spec - otherwise it is redundant to check
>> if spec is null at line 1247
>
> Already fixed.
>
>
>> libksba/src/cert.c:447
>> Possible null pointer dereference: cert - otherwise it is redundant to check
>> if cert is null at line 445
>
> Ditto.
>
>
>> libgcrypt/src/hmac256.c:510
>> Deallocating a deallocated pointer: hd
>
> I still can't see the problem.
>
>> gpgme/tests/gpg/t-eventloop.c:82
>> Assigning address of local auto-variable to a function parameter.
>
> See yesterdays comment.
>
>>
>> pinentry/gtk+-2/gtksecentry.h:188
>> number of character ({) when these macros are defined:
>> 'MAKE_EMACS_HAPPY;__cplusplus'.
>
> See yesterdays comment.
>
>
>> The bad news is that cppcheck did not check a lot of files because there were
>> more combinations of #ifdefs than its limit. I'll try upping the limit and
>> running it again. These are the files that were not checked because of
>> "Interrupted checking because of too many #ifdef configurations."
>
> Somehow I doubt that it makes sense to go through a list of so many
> false positives. At least I can't do that in the next days.
>
> Anyway, we found a couple of things and that was worth it.
>
>
> Feel free to forward to gnupg-devel.
The question now is whether you want cppcheck errors to automatically trigger
an email to the committers. That will be a lot easier if cppcheck does not
find things its thinks are errors. That means doing things like:
- char buffer[BUFFER_SIZE];
+ char buffer[BUFFER_SIZE] = ""; // make cppcheck happy
I'm happy to submit these kinds of fixes if you are willing to accept them.
.hc
--
PGP fingerprint: 5E61 C878 0F86 295C E17D 8677 9F0F E587 374B BE81
More information about the Gnupg-devel
mailing list