1365193 - Coverity issues introduced in bug 1162897

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Franziskus Kiefer [:franziskus]]

Patches landed for bug 1162897 introduced the following coverity issues.

** CID 1409013:  Resource leaks  (RESOURCE_LEAK)
/lib/dev/devtoken.c: 420 in nssToken_FindObjectsByTemplate()


________________________________________________________________________________________________________
*** CID 1409013:  Resource leaks  (RESOURCE_LEAK)
/lib/dev/devtoken.c: 420 in nssToken_FindObjectsByTemplate()
414                 if (statusOpt)
415                     *statusOpt = status;
416                 return objects;
417             }
418         }
419         /* Either they are not cached, or cache failed; look on token. */
>>>     CID 1409013:  Resource leaks  (RESOURCE_LEAK)
>>>     Overwriting "objects" in "objects = find_objects(token, sessionOpt, obj_template, otsize, maximumOpt, statusOpt)" leaks the storage that "objects" points to.
420         objects = find_objects(token, sessionOpt,
421                                obj_template, otsize,
422                                maximumOpt, statusOpt);
423         return objects;
424     }
425

** CID 1409012:  Null pointer dereferences  (NULL_RETURNS)
/gtests/util_gtest/util_pkcs11uri_unittest.cc: 35 in nss_test::PK11URITest::TestCreateRetrieve(const PK11URIAttributeStr *, unsigned long, const PK11URIAttributeStr *, unsigned long)()


________________________________________________________________________________________________________
*** CID 1409012:  Null pointer dereferences  (NULL_RETURNS)
/gtests/util_gtest/util_pkcs11uri_unittest.cc: 35 in nss_test::PK11URITest::TestCreateRetrieve(const PK11URIAttributeStr *, unsigned long, const PK11URIAttributeStr *, unsigned long)()
29         ASSERT_TRUE(tmp);
30
31         size_t i;
32         for (i = 0; i < num_pattrs; i++) {
33           const char *value = PK11URI_GetPathAttribute(tmp.get(), pattrs[i].name);
34           ASSERT_TRUE(value);
>>>     CID 1409012:  Null pointer dereferences  (NULL_RETURNS)
>>>     Dereferencing a pointer that might be null "value" when calling "basic_string".
35           ASSERT_EQ(std::string(value), std::string(pattrs[i].value));
36         }
37         for (i = 0; i < num_qattrs; i++) {
38           const char *value = PK11URI_GetQueryAttribute(tmp.get(), qattrs[i].name);
39           ASSERT_TRUE(value);
40           ASSERT_EQ(std::string(value), std::string(qattrs[i].value));

** CID 1409011:  Null pointer dereferences  (NULL_RETURNS)
/gtests/util_gtest/util_pkcs11uri_unittest.cc: 71 in nss_test::PK11URITest::TestParseRetrieve(const std::basic_string<char, std::char_traits<char>, std::allocator<char>>&, const PK11URIAttributeStr *, unsigned long, const PK11URIAttributeStr *, unsigned long)()


________________________________________________________________________________________________________
*** CID 1409011:  Null pointer dereferences  (NULL_RETURNS)
/gtests/util_gtest/util_pkcs11uri_unittest.cc: 71 in nss_test::PK11URITest::TestParseRetrieve(const std::basic_string<char, std::char_traits<char>, std::allocator<char>>&, const PK11URIAttributeStr *, unsigned long, const PK11URIAttributeStr *, unsigned long)()
65         ASSERT_TRUE(tmp);
66
67         size_t i;
68         for (i = 0; i < num_pattrs; i++) {
69           const char *value = PK11URI_GetPathAttribute(tmp.get(), pattrs[i].name);
70           ASSERT_TRUE(value);
>>>     CID 1409011:  Null pointer dereferences  (NULL_RETURNS)
>>>     Dereferencing a pointer that might be null "value" when calling "basic_string".
71           ASSERT_EQ(std::string(value), std::string(pattrs[i].value));
72         }
73         for (i = 0; i < num_qattrs; i++) {
74           const char *value = PK11URI_GetQueryAttribute(tmp.get(), qattrs[i].name);
75           ASSERT_TRUE(value);
76           ASSERT_EQ(std::string(value), std::string(qattrs[i].value));

** CID 1409010:  Null pointer dereferences  (FORWARD_NULL)
/lib/util/pkcs11uri.c: 683 in PK11URI_ParseURI()


________________________________________________________________________________________________________
*** CID 1409010:  Null pointer dereferences  (FORWARD_NULL)
/lib/util/pkcs11uri.c: 683 in PK11URI_ParseURI()
677         if (strncmp("pkcs11:", p, 7) != 0) {
678             return NULL;
679         }
680         p += 7;
681
682         result = pk11uri_AllocURI();
>>>     CID 1409010:  Null pointer dereferences  (FORWARD_NULL)
>>>     Comparing "result" to null implies that "result" might be null.
683         if (result == NULL) {
684             goto fail;
685         }
686
687         /* Parse the path component and its attributes. */
688         ret = pk11uri_ParseAttributes(&p, "?", ';', PK11URI_PCHAR,

** CID 1078807:    (RESOURCE_LEAK)
/cmd/crmftest/testcrmf.c: 1272 in DoChallengeResponse()
/cmd/crmftest/testcrmf.c: 1278 in DoChallengeResponse()


________________________________________________________________________________________________________
*** CID 1078807:    (RESOURCE_LEAK)
/cmd/crmftest/testcrmf.c: 1272 in DoChallengeResponse()
1266                 return 909;
1267             }
1268             foundPrivKey = PK11_FindKeyByKeyID(privKey->pkcs11Slot, keyID, &pwdata);
1269             if (foundPrivKey == NULL) {
1270                 printf("Could not find the private key corresponding to the public"
1271                        " value.\n");
>>>     CID 1078807:    (RESOURCE_LEAK)
>>>     Variable "keyID" going out of scope leaks the storage it points to.
1272                 return 910;
1273             }
1274             rv = CMMF_POPODecKeyChallContDecryptChallenge(chalContent, i,
1275                                                           foundPrivKey);
1276             if (rv != SECSuccess) {
1277                 printf("Could not decrypt the challenge at index %d\n", i);
/cmd/crmftest/testcrmf.c: 1278 in DoChallengeResponse()
1272                 return 910;
1273             }
1274             rv = CMMF_POPODecKeyChallContDecryptChallenge(chalContent, i,
1275                                                           foundPrivKey);
1276             if (rv != SECSuccess) {
1277                 printf("Could not decrypt the challenge at index %d\n", i);
>>>     CID 1078807:    (RESOURCE_LEAK)
>>>     Variable "keyID" going out of scope leaks the storage it points to.
1278                 return 911;
1279             }
1280             rv = CMMF_POPODecKeyChallContentGetRandomNumber(chalContent, i,
1281                                                             &retrieved);
1282             if (rv != SECSuccess) {
1283                 printf("Could not get the random number from the challenge at "

** CID 1078806:    (RESOURCE_LEAK)
/cmd/crmftest/testcrmf.c: 1266 in DoChallengeResponse()
/cmd/crmftest/testcrmf.c: 1272 in DoChallengeResponse()
/cmd/crmftest/testcrmf.c: 1278 in DoChallengeResponse()


________________________________________________________________________________________________________
*** CID 1078806:    (RESOURCE_LEAK)
/cmd/crmftest/testcrmf.c: 1266 in DoChallengeResponse()
1260                        i);
1261                 return 908;
1262             }
1263             keyID = PK11_MakeIDFromPubKey(publicValue);
1264             if (keyID == NULL) {
1265                 printf("Could not make the keyID from the public value\n");
>>>     CID 1078806:    (RESOURCE_LEAK)
>>>     Variable "publicValue" going out of scope leaks the storage it points to.
1266                 return 909;
1267             }
1268             foundPrivKey = PK11_FindKeyByKeyID(privKey->pkcs11Slot, keyID, &pwdata);
1269             if (foundPrivKey == NULL) {
1270                 printf("Could not find the private key corresponding to the public"
1271                        " value.\n");
/cmd/crmftest/testcrmf.c: 1272 in DoChallengeResponse()
1266                 return 909;
1267             }
1268             foundPrivKey = PK11_FindKeyByKeyID(privKey->pkcs11Slot, keyID, &pwdata);
1269             if (foundPrivKey == NULL) {
1270                 printf("Could not find the private key corresponding to the public"
1271                        " value.\n");
>>>     CID 1078806:    (RESOURCE_LEAK)
>>>     Variable "publicValue" going out of scope leaks the storage it points to.
1272                 return 910;
1273             }
1274             rv = CMMF_POPODecKeyChallContDecryptChallenge(chalContent, i,
1275                                                           foundPrivKey);
1276             if (rv != SECSuccess) {
1277                 printf("Could not decrypt the challenge at index %d\n", i);
/cmd/crmftest/testcrmf.c: 1278 in DoChallengeResponse()
1272                 return 910;
1273             }
1274             rv = CMMF_POPODecKeyChallContDecryptChallenge(chalContent, i,
1275                                                           foundPrivKey);
1276             if (rv != SECSuccess) {
1277                 printf("Could not decrypt the challenge at index %d\n", i);
>>>     CID 1078806:    (RESOURCE_LEAK)
>>>     Variable "publicValue" going out of scope leaks the storage it points to.
1278                 return 911;
1279             }
1280             rv = CMMF_POPODecKeyChallContentGetRandomNumber(chalContent, i,
1281                                                             &retrieved);
1282             if (rv != SECSuccess) {
1283                 printf("Could not get the random number from the challenge at "

Comment 1

[Daiki Ueno [:ueno]]

Attachment: nss-pkcs11uri-coverity.patch

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #0)

> *** CID 1409010:  Null pointer dereferences  (FORWARD_NULL)
> /lib/util/pkcs11uri.c: 683 in PK11URI_ParseURI()
> 677         if (strncmp("pkcs11:", p, 7) != 0) {
> 678             return NULL;
> 679         }
> 680         p += 7;
> 681
> 682         result = pk11uri_AllocURI();
> >>>     CID 1409010:  Null pointer dereferences  (FORWARD_NULL)
> >>>     Comparing "result" to null implies that "result" might be null.
> 683         if (result == NULL) {
> 684             goto fail;
> 685         }

Though I haven't figured out a fix for other issues yet, this is obviously a bug; sorry about that.  I am attaching a patch for this.

Comment 2

[Daiki Ueno [:ueno]]

Attachment: nss-pkcs11uri-gtests-coverity.patch

This patch suppresses the coverity error (actually a false-positive) in util_pkcs11uri_unittest.cc.  Also it now uses EXPECT_*() instead of ASSERT_*() as appropriate.

Comment 3

[Daiki Ueno [:ueno]]

Attachment: nss-crmftest-coverity.patch

This patch fixes RESOURCE_LEAK error at:
/cmd/crmftest/testcrmf.c: 1272 in DoChallengeResponse().

Comment 4

[Daiki Ueno [:ueno]]

I went through the errors and attached possible fixes.

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #0)
> Patches landed for bug 1162897 introduced the following coverity issues.
> 
> ** CID 1409013:  Resource leaks  (RESOURCE_LEAK)
> /lib/dev/devtoken.c: 420 in nssToken_FindObjectsByTemplate()

I think this is a false-positive.  "objects" should be NULL at line 420, when the above nssTokenObjectCache_FindObjectsByTemplate() call returned PR_FAILURE.

> ** CID 1409012:  Null pointer dereferences  (NULL_RETURNS)
> /gtests/util_gtest/util_pkcs11uri_unittest.cc: 35 in
> nss_test::PK11URITest::TestCreateRetrieve(const PK11URIAttributeStr *,
> unsigned long, const PK11URIAttributeStr *, unsigned long)()

This should be fixed with attachment 8868495 [details] [diff] [review].

> ** CID 1409011:  Null pointer dereferences  (NULL_RETURNS)
> /gtests/util_gtest/util_pkcs11uri_unittest.cc: 71 in
> nss_test::PK11URITest::TestParseRetrieve(const std::basic_string<char,
> std::char_traits<char>, std::allocator<char>>&, const PK11URIAttributeStr *,
> unsigned long, const PK11URIAttributeStr *, unsigned long)()

Same as the above.

> ** CID 1409010:  Null pointer dereferences  (FORWARD_NULL)
> /lib/util/pkcs11uri.c: 683 in PK11URI_ParseURI()

This should be fixed with attachment 8868068 [details] [diff] [review].

> ** CID 1078807:    (RESOURCE_LEAK)
> /cmd/crmftest/testcrmf.c: 1272 in DoChallengeResponse()
> /cmd/crmftest/testcrmf.c: 1278 in DoChallengeResponse()

This should be fixed with attachment 8868497 [details] [diff] [review].  It doesn't seem to be related to the PKCS#11 URI patches, but it might be triggered by the code reorganization.

> ** CID 1078806:    (RESOURCE_LEAK)
> /cmd/crmftest/testcrmf.c: 1266 in DoChallengeResponse()
> /cmd/crmftest/testcrmf.c: 1272 in DoChallengeResponse()
> /cmd/crmftest/testcrmf.c: 1278 in DoChallengeResponse()

Same as the above.

Comment 5

[Kai Engert (:KaiE:)]

(In reply to Daiki Ueno [:ueno] from comment #4)
> > 
> > ** CID 1409013:  Resource leaks  (RESOURCE_LEAK)
> > /lib/dev/devtoken.c: 420 in nssToken_FindObjectsByTemplate()
> 
> I think this is a false-positive.  "objects" should be NULL at line 420,
> when the above nssTokenObjectCache_FindObjectsByTemplate() call returned
> PR_FAILURE.

After the call to nssTokenObjectCache_FindObjectsByTemplate,
could it happen that: status != PR_SUCCESS, and object != NULL ?

This is what coverity seems to consider as possible.

Looking at nssTokenObjectCache_FindObjectsByTemplate, I agree this isn't possible with the current code.

I don't know if it's necessary to supress false positive warnings, or if we're required to make the code more obvious.

Would it help with coverity, to extend this code:

        if (status == PR_SUCCESS) {
            if (statusOpt)
                *statusOpt = status;
            return objects;

with:
        } else {
            assert objects == NULL
        }
?

Comment 6

[Kai Engert (:KaiE:)]

https://hg.mozilla.org/projects/nss/rev/4d6a660783c69e124d23b2323ebfcb9122664d29
https://hg.mozilla.org/projects/nss/rev/009ef22070ef34992c153404ad19da8ce2494afc
https://hg.mozilla.org/projects/nss/rev/37915eb6796baba07a4ad831549656c0439c49f7