Closed Bug 529485 Opened 10 years ago Closed 9 years ago
PSM crashes [@ Process
Auth Key Id ] when CERT _Decode Auth Key ID(arena, ext Data) fails
28.68 KB, patch
|Details | Diff | Splinter Review|
9.70 KB, patch
|Details | Diff | Splinter Review|
884 bytes, patch
|Details | Diff | Splinter Review|
We've got three reports of this in Camino now, and four in Firefox: http://crash-stats.mozilla.com/report/list?range_value=2&range_unit=weeks&signature=ProcessAuthKeyId&version=Camino%3A2.0 (Camino, all on Mac OS X 10.6.2) http://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=exact&query=ProcessAuthKeyId&date=&range_value=4&range_unit=weeks&do_query=1&signature=ProcessAuthKeyId (Firefox, two on OS X 10.6.2 with 3.6b, and two on XP SP2 with 3.0.x) The ones in Camino, at least, appear to have been triggered by local imports of certificates (2) and by adding a certificate exception (1). It looks to me from basic code inspection around the crash: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp&rev=1.38&mark=1258#1258 that if CERT_DecodeAuthKeyID (arena, extData) fails for whatever reason (line 1256), |ret| could be null, causing a crash on line 1258. Kai, is there any way that could fail (e.g., with self-signed certificates or any other sorts of non-kosher things that someone might be doing locally) and be causing this?
Summary: CERT_DecodeAuthKeyID (arena, extData) can potentially fail, causing a crash → CERT_DecodeAuthKeyID (arena, extData) can potentially fail, causing a crash [@ProcessAuthKeyId ]
I think I see reports of the same problem in firefox 3.6b3 data with comments 20091119-crashdata.csv:ProcessAuthKeyId 3.6b3 I tried to view the self-signed certificate used for internal web server and signed by local CA. 20091119-crashdata.csv:ProcessAuthKeyId 3.6b3 Tried to view just imported self-signed certificate. not sure how much kai is able to look at bugs these days. is there someone else that can take a look? might be a good test case to add. I spotted a report on windows as well but most of these seem to be mac.
Signature ProcessAuthKeyId UUID c7faa097-3192-4a5b-8d36-642942091110 Time 2009-11-10 04:51:31.201930 Uptime 107 Product Camino Version 2.0 Build ID 2009102617 Branch 1.9.0 OS Mac OS X OS Version 10.6.2 10C540 CPU x86 CPU Info GenuineIntel family 6 model 14 stepping 8 Crash Reason EXC_BAD_ACCESS / KERN_PROTECTION_FAILURE Crash Address 0x8 User Comments Importing certificate lead to the crash. Processor Notes WARNING: Json file missing Add-ons Related Bugs Crashing Thread Frame Module Signature [Expand] Source 0 Camino ProcessAuthKeyId mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp:1258 1 Camino ProcessExtensionData mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp:1649 2 Camino nsNSSCertificate::CreateTBSCertificateASN1Struct mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp:1713 3 Camino nsNSSCertificate::CreateASN1Struct mozilla/security/manager/ssl/src/nsNSSCertHelper.cpp:2164 4 Camino nsNSSCertificate::GetASN1Structure mozilla/security/manager/ssl/src/nsNSSCertificate.cpp:1450 5 Camino -[CertificateItem ensureASN1Info] mozilla/camino/src/security/CertificateItem.mm:747 6 Camino -[CertificateItem ASN1PropertyWithKeyPath:] mozilla/camino/src/security/CertificateItem.mm:761 7 Camino -[CertificateItem version] mozilla/camino/src/security/CertificateItem.mm:322 8 Camino -[CertificateView rebuildDetails] mozilla/camino/src/security/CertificateView.mm:618 9 Camino +[ViewCertificateDialogController showCertificateWindowWithCertificateItem:certTypeForTrustSettings:] mozilla/camino/src/security/ViewCertificateDialogController.mm:86 10 Camino -[CertificatesWindowController viewSelectedCertificates:] mozilla/camino/src/security/CertificatesWindowController.mm:468
Depends on: 259031
Summary: CERT_DecodeAuthKeyID (arena, extData) can potentially fail, causing a crash [@ProcessAuthKeyId ] → CERT_DecodeAuthKeyID (arena, extData) can potentially fail, causing a crash [@ ProcessAuthKeyId ]
this is a changeset sequence generated with hg export. ideally importing it should result in 3 changesets....
Assignee: kaie → timeless
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attachment #413904 - Flags: review?(kaie)
Comment on attachment 413904 [details] [diff] [review] patch v1, with whitespace changes and renamed variables I suppose that somewhere inside this patch there's a change that addresses this bug, but it is completely obscured by many lines that change nothing but whitespace. :( If bugzilla's patch viewer gave us an option to ignore whitespace (bug 285814) we could easily ignore all that, but as things are, please attach a patch which ignores whitespace changes. Thanks.
or, just break the patch up into something that fixes the bug, and another patch that cleans up the code.
chofmann: i did. there are three patches, the patch nelson wants is: Node ID a016173e025e277f07f10dbb710fc864aabcd0e5
What's the bugzilla attachment number of that patch?
nelson, it's the section after https://bugzilla.mozilla.org/attachment.cgi?id=413904&action=diff#a/security/manager/ssl/src/nsNSSCertHelper.cpp_sec38 -- bugzilla's attachment viewer is buggy, it can't link to it.
The patch file originally attached to this bug was actually 3 separate patches, all made to the same file. Small wonder that bugzilla couldn't create links to the second and third ones, since it assumes that each patch file contains at most one patch to any source files. I split that patch into three separate patches, which I will attach here. One of them clearly addresses the crash reported in this bug. The other two may serve some other purpose.
Attachment #494370 - Flags: review?
Attachment #494370 - Flags: review? → review?(kaie)
This is the patch that addresses the crash. One of the people who works on PSM should review it. But it's so small that maybe I will do that.
This is the last of Timeless's 3 original patches to this file.
Attachment #494370 - Attachment description: Large white-space only patch by Timeless → Large white-space and variable renaming patch by Timeless
Comment on attachment 494372 [details] [diff] [review] fix crash, written by Timeless, depends on Timeless's previous variable renaming patch. If this patch did not depend on the other, much larger patch attached to this bug, which changes whitespace and renames variables, I would be inclined to recommend that it get a positive review.
Attachment #494372 - Attachment description: patch to fix crash, written by timeless → fix crash, written by Timeless, depends on Timeless's previous variable renaming patch.
Summary: CERT_DecodeAuthKeyID (arena, extData) can potentially fail, causing a crash [@ ProcessAuthKeyId ] → PSM crashes [@ ProcessAuthKeyId ] when CERT_DecodeAuthKeyID(arena, extData) fails
I also have trouble to read patches where the meat is hidden. I applied the patches, did a -w diff, revert the original patch, applied the -w patch, reverted the key/ret rename, etc. The result is this new patch, which I believe contains all the effective changes from the original patch.
Attachment #494370 - Attachment is obsolete: true
Attachment #494372 - Attachment is obsolete: true
Attachment #494373 - Attachment is obsolete: true
Attachment #499301 - Flags: review?(kaie)
Attachment #494370 - Flags: review?(kaie)
Attachment #494372 - Flags: review?(kaie)
If you had attached a patch like attachment 494372 [details] [diff] [review] right away, which seems to be sufficient to fix the crash, you would have gotten an r+ much earlier. I'm sorry that we're slow and have too much work, but that's the way it is. Doing minimal patches helps to keep turnaround times small. Now, I have reviewed attachment 499306 [details] [diff] [review], and I'm OK with it. I'm also OK with whitespace changes and I don't mind renaming that variable, so I'll r+ the original patch.
Comment on attachment 413904 [details] [diff] [review] patch v1, with whitespace changes and renamed variables I have reviewed attachment 499306 [details] [diff] [review], and this patch is functionally equivalent to it. r=kaie
Attachment #413904 - Flags: review+
FWIW, alternatively, this is the minimal patch sufficient to fix the crash, not depending on anything else, applies to latest trunk.
Comment on attachment 413904 [details] [diff] [review] patch v1, with whitespace changes and renamed variables I'll approve the minimal patch.
Comment on attachment 499309 [details] [diff] [review] alternative, minimal patch (meat without cleanup) a=LegNeato for 188.8.131.52 and 184.108.40.206
I'll wait for trunk approval before landing any of these.
Comment on attachment 499309 [details] [diff] [review] alternative, minimal patch (meat without cleanup) Missed non-blocker code-freeze for 220.127.116.11 and 18.104.22.168 -- rescinding approval, you can try again next time.
Why are FF 4 drivers not interested in a crash fix?
This is a crash with a patch that has been r+d and so we should just consider it a blocker and/or give explicit 2.0 approval. The patch simply prevents a null-pointer dereference so there's no chance of regression.
Whiteboard: [psm-fatal] → [psm-fatal][hardblocker]
It seems this bug exists in Firefox 3.6, so nearly by definition it's not a blocker (soft or hard) at this point. [Alas triage may have been slow here if no one was looking for requests here.] But I'll go ahead and a+ the simplified version since it's simple and comment 24 says it's low risk.
blocking2.0: ? → -
Whiteboard: [psm-fatal][hardblocker] → [psm-fatal]
Brian or Honza, could you please review this obvious minimal patch? Drivers don't want to take the large patch for 2.0 Thanks
Attachment #499309 - Flags: review?(bsmith) → review+
http://hg.mozilla.org/mozilla-central/rev/d0798ddf795c Minimal patch landed. Given how much time we spent on this, we should land the full patch later. Let's hope we won't forget.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Comment on attachment 499309 [details] [diff] [review] alternative, minimal patch (meat without cleanup) now that trunk is fixed, rerequesting approval for stable branches
(In reply to comment #28) > Comment on attachment 499309 [details] [diff] [review] > alternative, minimal patch (meat without cleanup) > > now that trunk is fixed, rerequesting approval for stable branches Was this landed on the branches? I don't see the request flag anymore.
Comment on attachment 499309 [details] [diff] [review] alternative, minimal patch (meat without cleanup) Approved for 22.214.171.124 and 126.96.36.199, a=dveditz for release-drivers
Whiteboard: [psm-fatal] → [psm-fatal][cn: 188.8.131.52, 184.108.40.206]
Can't find recent reports for this crash -- marking VERIFIED.
Status: RESOLVED → VERIFIED
this was pushed to all branches, removing checkin-needed request, correct me if I'm wrong please.
Whiteboard: [psm-fatal][cn: 220.127.116.11, 18.104.22.168] → [psm-fatal]
The "3.6.15" we're releasing today does not fix this bug, the release containing this bug fix has been renamed to "3.6.16" and the bugzilla flags will be updated to reflect that soon. Today's release is a re-release of 3.6.14 plus a fix for a bug that prevented many Java applets from starting up.
Are there any STR for 1.9.1 or 1.9.2 for this? I don't see any way to verify the fix here.
Whiteboard: [psm-fatal] → [psm-fatal] [qa-examined-191] [qa-examined-192] [qa-needs-STR]
You need to log in before you can comment on or make changes to this bug.