1766978 - Crash in [@ nssCKFWObject_GetAttribute | NSSCKFWC_GetAttributeValue | PK11_ReadAttribute]

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This got more frequent since bug 1747959 landed. ~900 crashes for Firefox 99.0 and 99.0.1.

Crash report: https://crash-stats.mozilla.org/report/index/75d7d63d-c996-4f7c-b24f-211100220429

Reason: EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 10 frames of crashing thread:

0 libnssckbi.dylib nssCKFWObject_GetAttribute security/nss/lib/ckfw/object.c:649
1 libnssckbi.dylib NSSCKFWC_GetAttributeValue security/nss/lib/ckfw/wrap.c:2280
2 libnss3.dylib PK11_ReadAttribute security/nss/lib/pk11wrap/pk11obj.c:121
3 libnss3.dylib PK11_FindRawCertsWithSubject security/nss/lib/pk11wrap/pk11obj.c:1992
4 XUL mozilla::psm::NSSCertDBTrustDomain::FindIssuer security/certverifier/NSSCertDBTrustDomain.cpp:249
5 XUL mozilla::pkix::BuildForward security/nss/lib/mozpkix/lib/pkixbuild.cpp:365
6 XUL mozilla::pkix::PathBuildingStep::Check security/nss/lib/mozpkix/lib/pkixbuild.cpp:211
7 XUL mozilla::psm::CheckCandidates security/certverifier/NSSCertDBTrustDomain.cpp:188
8 XUL mozilla::psm::NSSCertDBTrustDomain::FindIssuer security/certverifier/NSSCertDBTrustDomain.cpp:324
9 XUL mozilla::pkix::BuildForward security/nss/lib/mozpkix/lib/pkixbuild.cpp:365

Comment 1

[John Schanck [:jschanck]]

Looked at this today and I have a rough idea of what's wrong. I'll need to spend some more time with it to confirm.

Comment 2

[John Schanck [:jschanck]]

The stack trace shows a crash in nssCKFWObject_GetAttribute while accessing fwObject->mutex. This suggests that nssCKFWObject_Destroy was called on fwObject while the crashing thread was in nssCKFWObject_GetAttribute.

A review of nssCKFWObject_Destroy call sites shows the following in NSSCKFWC_FindObjects.

        phObject[i] = nssCKFWInstance_FindObjectHandle(fwInstance, fwObject);
        if ((CK_OBJECT_HANDLE)0 == phObject[i]) {
            phObject[i] = nssCKFWInstance_CreateObjectHandle(fwInstance, fwObject, &error);
        }
        if ((CK_OBJECT_HANDLE)0 == phObject[i]) {
            /* This isn't right either, is it? */
            nssCKFWObject_Destroy(fwObject);
            goto loser;
        }

upshot: the comment's author was correct.

The call to nssCKFWInstance_FindObjectHandle returns null if fwObject does not yet have a handle.

The call to nssCKFWInstance_CreateObjectHandle typically creates a handle for fwObject, but it returns null and sets error = CKR_GENERAL_ERROR if fwObject already has a handle.

Both functions are thread safe, but in a multithreaded environment the caller must view a null return from nssCKFWInstance_CreateObjectHandle as a potential success: Another thread may have created a handle for fwObject after the call to nssCKFWInstance_FindObjectHandle.

I believe this explains the crash in nssCKFWObject_GetAttribute, since nssCKFWObject_GetAttribute is frequently called after NSSCKFWC_FindObjects.

I'm also adding the signature [@ je_free | nssCKFWObject_Destroy | NSSCKFWC_FindObjects | builtinsC_FindObjects ]. This would happen if three threads got simultaneous null returns from nssCKFWInstance_FindObjectHandle. One would set the handle, another would destroy the object, and the third would crash in destroy.

Comment 3

[John Schanck [:jschanck]]

Attachment: Bug 1766978 - improve error handling after nssCKFWInstance_CreateObjectHandle. r=djackson

Comment 4

[John Schanck [:jschanck]]

https://hg.mozilla.org/projects/nss/rev/2efccbd85918ac0a404340ece219b6f2824ff87a

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Since we're creating an NSS 3.78.1 release anyway, can we include this in it?

Comment 6

[John Schanck [:jschanck]]

Yes, I discussed with Dennis and we decided that it's low risk and worth including in 3.78.1.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Copying crash signatures from duplicate bugs.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

Copying crash signatures from duplicate bugs.