1558313 - Code bugs found by clang scanners.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

clang generates a lot of noise, particularly in gtests and fipstest, but it did find a few low priority issues that probably should be fixed:

>>> 1. Defect type: CLANG_WARNING
1. nss-3.44/nss/lib/softoken/pkcs11.c:2283:13: warning: Value stored to 'crv' is never read

sftk_PutPubKey() has a switch statement in which crv can fall out with an error. The code in the switch is expecting a crv check out the bottom, but one doesn't exist. The chances of an error is pretty small, but the check should be there anyway.

>>>2. Defect type: CLANG_WARNING
1. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:179:20: warning: Access to field 'arena' results in a dereference of a null pointer (loaded from variable 'epki')

There are a bunch of frees here that need to check the pointer for NULL, The epki arena is just the most obvious. This is a test program, and crashing in an error condition will still trigger a test failure, but it should be cleaned up.

------------------------ full clang log ------------------------------------------------------

>>> 1. Defect type: CLANG_WARNING
1. nss-3.44/nss/lib/softoken/pkcs11.c:2283:13: warning: Value stored to 'crv' is never read
#             crv = sftk_AddAttributeType(publicKey, CKA_EC_POINT,
#             ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
4. nss-3.44/nss/lib/softoken/pkcs11.c:2283:13: note: Value stored to 'crv' is never read
#             crv = sftk_AddAttributeType(publicKey, CKA_EC_POINT,
#             ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
#  2281|               }
#  2282|   
#  2283|->             crv = sftk_AddAttributeType(publicKey, CKA_EC_POINT,
#  2284|                                           sftk_item_expand(&pubKey->u.ec.publicValue));
#  2285|               break;

>>>3. Defect type: CLANG_WARNING
1. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:179:20: warning: Access to field 'arena' results in a dereference of a null pointer (loaded from variable 'epki')
#     PORT_FreeArena(epki->arena, PR_TRUE);
#                    ^
4. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:263:16: note: Assuming 'progName' is null
#     progName = progName ? progName + 1 : argv[0];
#                ^~~~~~~~
7. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:263:16: note: '?' condition is false
8. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:266:9: note: Assuming 'rv' is equal to SECSuccess
#     if (SECSuccess != rv) {
#         ^~~~~~~~~~~~~~~~
11. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:266:5: note: Taking false branch
#     if (SECSuccess != rv) {
#     ^
14. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:272:9: note: Assuming 'rv' is equal to SECSuccess
#     if (rv != SECSuccess) {
#         ^~~~~~~~~~~~~~~~
17. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:272:5: note: Taking false branch
#     if (rv != SECSuccess) {
#     ^
20. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:282:9: note: Assuming the condition is false
#     if (args.options[opt_PWFile].arg) {
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
23. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:282:5: note: Taking false branch
#     if (args.options[opt_PWFile].arg) {
#     ^
26. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:286:9: note: Assuming the condition is false
#     if (args.options[opt_PWString].arg) {
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
29. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:286:5: note: Taking false branch
#     if (args.options[opt_PWString].arg) {
#     ^
32. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:290:9: note: Assuming the condition is false
#     if (args.options[opt_NoRSA].activated) {
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
35. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:290:5: note: Taking false branch
#     if (args.options[opt_NoRSA].activated) {
#     ^
38. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:293:9: note: Assuming the condition is true
#     if (args.options[opt_NoDSA].activated) {
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
41. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:293:5: note: Taking true branch
#     if (args.options[opt_NoDSA].activated) {
#     ^
44. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:296:9: note: Assuming the condition is false
#     if (args.options[opt_NoDH].activated) {
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
47. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:296:5: note: Taking false branch
#     if (args.options[opt_NoDH].activated) {
#     ^
50. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:299:9: note: Assuming the condition is false
#     if (args.options[opt_NoEC].activated) {
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
53. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:299:5: note: Taking false branch
#     if (args.options[opt_NoEC].activated) {
#     ^
56. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:304:9: note: Assuming 'slot' is not equal to NULL
#     if (slot == NULL) {
#         ^~~~~~~~~~~~
59. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:304:5: note: Taking false branch
#     if (slot == NULL) {
#     ^
62. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:309:9: note: Assuming 'rv' is equal to SECSuccess
#     if (rv != SECSuccess) {
#         ^~~~~~~~~~~~~~~~
65. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:309:5: note: Taking false branch
#     if (rv != SECSuccess) {
#     ^
68. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:316:9: note: Assuming the condition is false
#     if (args.options[opt_KeySize].activated &&
#         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
71. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:316:45: note: Left side of '&&' is false
#     if (args.options[opt_KeySize].activated &&
#                                             ^
74. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:321:9: note: Left side of '||' is false
#     if (doDSA || doDH) {
#         ^
77. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:321:5: note: Taking false branch
#     if (doDSA || doDH) {
#     ^
80. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:334:5: note: Taking true branch
#     if (doRSA) {
#     ^
83. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:338:14: note: Calling 'handleEncryptedPrivateImportTest'
#         rv = handleEncryptedPrivateImportTest(progName, slot, "RSA",
#              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
86. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:67:5: note: 'epki' initialized to a null pointer value
#     SECKEYEncryptedPrivateKeyInfo *epki = NULL;
#     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
89. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:77:9: note: Assuming 'privKey' is equal to NULL
#     if (privKey == NULL) {
#         ^~~~~~~~~~~~~~~
92. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:77:5: note: Taking true branch
#     if (privKey == NULL) {
#     ^
95. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:79:9: note: Control jumps to line 174
#         goto cleanup;
#         ^
98. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:174:5: note: Taking false branch
#     if (objs) {
#     ^
101. nss-3.44/nss/cmd/pk11importtest/pk11importtest.c:179:20: note: Access to field 'arena' results in a dereference of a null pointer (loaded from variable 'epki')
#     PORT_FreeArena(epki->arena, PR_TRUE);
#                    ^~~~
#   177|       SECITEM_FreeItem(&pubValue, PR_FALSE);
#   178|       SECITEM_FreeItem(&privID, PR_FALSE);
#   179|->     PORT_FreeArena(epki->arena, PR_TRUE);
#   180|       SECKEY_DestroyPublicKey(pubKey);
#   181|       SECKEY_DestroyPrivateKey(privKey);

Comment 1

[Jenine (she/her)]

Attachment: Bug 1558313 - NULL check for epki pointer

Comment 2

[Jenine (she/her)]

(In reply to Robert Relyea from comment #0)

I'm caveating my attempt with how I'm new to the codebase and I haven't run the appropriate tests yet but I wanted to confirm:

• This bug is about checks against pointers which could be NULL (found by Clang warnings)
• This bug is not about editing the switch statement in question (but highlighting how from that switch statement, the line PORT_FreeArena(epki->arena, PR_TRUE); can be called as a result from the default case

Thanks

Comment 3

[Marcus Burghardt]

(In reply to Jenine from comment #2)

(In reply to Robert Relyea from comment #0)

I'm caveating my attempt with how I'm new to the codebase and I haven't run the appropriate tests yet but I wanted to confirm:

• This bug is about checks against pointers which could be NULL (found by Clang warnings)

Yes.

• This bug is not about editing the switch statement in question (but highlighting how from that switch statement, the line PORT_FreeArena(epki->arena, PR_TRUE); can be called as a result from the default case

The crv var is set twice in those switches, but the last one is not read (if). We have to add another test of crv.

Thanks

Comment 4

[Jenine (she/her)]

Attachment: Bug 1558313 - Fix clang warnings in pk11importtest.c and pkcs11.c

Depends on D40573

Comment 5

[Jenine (she/her)]

Sorry for the delayed reply! I've commented on D41486 with an update (I'm having trouble rebasing the patch)

Comment 6

[Jenine (she/her)]

Additionally, my last comment had default status changes ready to be saved - sorry, I'm unsure what the status should be. Apologies if I'm incorrectly labelling the bug something it shouldn't be?

Comment 7

[J.C. Jones [:jcj] (he/him)]

https://hg.mozilla.org/projects/nss/rev/4569b745f74e3ec1d1ea8840a7165627f2a34db7