Closed Bug 399706 Opened 12 years ago Closed 12 years ago

glibc detected: free(): invalid pointer attempting to select cert

Categories

(Core :: Security: PSM, defect, P1, critical)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: bc, Assigned: kaie)

References

Details

(Keywords: crash, regression, Whiteboard: [has-patch])

Attachments

(2 files, 1 obsolete file)

Hang with glibc detected: free(): invalid pointer:

steps to reproduce:

1. install signing/encrption cert (I use thawte).
2. enable ask everytime when web site requests cert.
3. install chatzilla.
4. connect to ircs://moznet

I also have a master password set, but that normally isn't requested until after the cert selection dialog appears so I don't think that is required.

expected results:

after connecting to moznet message, dialog appears to select cert

actual results:

hang, console shows glibc detected: free(): invalid pointer:

regressed after 2007-10-12-14 build. bug 392790?
Flags: blocking1.9?
#0  0x00110402 in __kernel_vsyscall ()
#1  0x00b01ba0 in raise () from /lib/libc.so.6
#2  0x00b034b1 in abort () from /lib/libc.so.6
#3  0x00b37dfb in __libc_message () from /lib/libc.so.6
#4  0x00b3fa96 in _int_free () from /lib/libc.so.6
#5  0x00b42fb0 in free () from /lib/libc.so.6
#6  0x003bbb9e in PR_Free (ptr=0xa8b8f90) at /work/mozilla/builds/1.9.0/mozilla/nsprpub/pr/src/malloc/prmem.c:490
#7  0x00350219 in NS_Free_P (ptr=0xa8b8f90) at /work/mozilla/builds/1.9.0/mozilla/xpcom/base/nsMemoryImpl.cpp:303
#8  0x0148a357 in nsMemory::Free (ptr=0xa8b8f90) at ../../../../dist/include/xpcom/nsMemory.h:74
#9  0x014aedd0 in nsNSSCertificate::FormatUIStrings (this=0xa96d858, nickname=@0xb49f1cd4, nickWithSerial=@0xb49f1c40, 
    details=@0xb49f1bac) at /work/mozilla/builds/1.9.0/mozilla/security/manager/ssl/src/nsNSSCertificate.cpp:480
#10 0x0149dc27 in nsNSS_SSLGetClientAuthData (arg=0xb63006d0, socket=0xb630c808, caNames=0xb49f1fe8, pRetCert=0xb6300b38, 
    pRetKey=0xb6300b3c) at /work/mozilla/builds/1.9.0/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp:2191
#11 0x063273f6 in ssl3_HandleCertificateRequest (ss=0xb6300758, b=0xb6307fa9 "\016", length=0) at ssl3con.c:5149
#12 0x0632ca61 in ssl3_HandleHandshakeMessage (ss=0xb6300758, b=0xb6307fa4 "\002\001\002", length=5) at ssl3con.c:7798
#13 0x0632cdf2 in ssl3_HandleHandshake (ss=0xb6300758, origBuf=0xb63009b4) at ssl3con.c:7898
#14 0x0632d7da in ssl3_HandleRecord (ss=0xb6300758, cText=0xb49f21d4, databuf=0xb63009b4) at ssl3con.c:8161
#15 0x0632e915 in ssl3_GatherCompleteHandshake (ss=0xb6300758, flags=0) at ssl3gthr.c:206
#16 0x06331562 in ssl_GatherRecord1stHandshake (ss=0xb6300758) at sslcon.c:1258
Blocks: 392790
Attached patch Proposed fix (obsolete) — Splinter Review
(In reply to comment #0)
> regressed after 2007-10-12-14 build. bug 392790?

Yes, introduced by bug 392790, unfortunately - thanks for testing/reporting!

For obvious reasons, we have to remember the pointer to the emailAddr list, in order to free it when we're done. The code from attachment 284277 [details] [diff] [review], however, increments emailAddr continuously, until it's pointing at an invalid location (which, when supplied to free(), will cause the hang experienced by the reporter - other platforms will just be silent, that's why I didn't notice this before).

Kai, I suggest we change the code to use for() loops and free the email addresses separately, in a loop at the end (this also makes the code more readable, I think). Can you please review?
Assignee: kengert → mozbugzilla
Status: NEW → ASSIGNED
Attachment #284820 - Flags: review?(kengert)
So, the real problem is, you must free the pointer originally retried from GetEmailAddresses, but you loose it while you iterate. Sorry that I didn't catch that during review.

You're now proposing a big patch, but the solution to your problem is much simpler. No need for a second loop, no need for converting to [] operators. Just keep the original pointer and use a different pointer for iterating.

@@ -443,8 +443,9 @@ nsNSSCertificate::FormatUIStrings(const
     }

     PRUint32 num;
-    PRUnichar **emailAddr = NULL;
+    PRUnichar **emailArray = NULL;
     if (NS_SUCCEEDED(GetEmailAddresses(&num, &emailAddr)) && num > 0) {
+      PRUnichar **emailAddr = emailArray;
       details.AppendLiteral("  ");
       if (NS_SUCCEEDED(nssComponent->GetPIPNSSBundleString("CertInfoEmail", info))) {
         details.Append(info);

(and free emailArray, not emailAddr)
Actually, I would like to fix this differently.

When I first reviewed this, I assumed it would be necessary to make use of function GetEmailAddresses, but we don't have to, it's part of the same class! Let's avoid all memory allocation and dealing with pointers and iterate the email addresses directly.
Attachment #284820 - Attachment is obsolete: true
Attachment #284989 - Flags: review?(rrelyea)
Attachment #284820 - Flags: review?(kengert)
This is a pita in that I can't use chatzilla any more on the trunk and can no longer dogfood minefield. Can we back out the original patch while we are waiting for the new approach to go in?
(In reply to comment #5)
> This is a pita in that I can't use chatzilla any more on the trunk and can no
> longer dogfood minefield. Can we back out the original patch while we are
> waiting for the new approach to go in?

I'm open to all ideas, but the tree is closed.
(In reply to comment #5)
> This is a pita in that I can't use chatzilla any more on the trunk and can no
> longer dogfood minefield.

As a temporary workaround, you could change the "When a web site requires a certificate" setting to "Select one automatically", this should at least help with recent trunk builds.

But in any case, it would be good to get r+ from Bob [Relyea] as soon as possible, since it still requires approval afterwards.
Attached patch Minimal FixSplinter Review
Reviewers are swamped.

Let's use the minimal fix that I describe in comment 3. Basically, the existing code gets a pointer, then uses ++ on that pointer, and tries to free the result. With this patch, we keep the original pointer around and free that.

People are eagerly waiting for the crash fix, so I'll be happy with the first review I get. Thanks!
Attachment #285484 - Flags: superreview?(mozbugzilla)
Attachment #285484 - Flags: review?(nelson)
Attachment #284989 - Attachment description: Patch v2 → Prefered, but bigger Patch v2
Uh, kaie?

kengert: review? (nelson)
kengert: superreview? (mozbugzilla)

Kaspar isn't a super-reviewer, nor is he really a fake one for PSM. Swap the reviewers? :)
Comment on attachment 285484 [details] [diff] [review]
Minimal Fix

Kaspar was the original author of the patch. I think it should be ok to ask him for a simple review for an obvious crash fix.
Attachment #285484 - Flags: superreview?(mozbugzilla) → review?(mozbugzilla)
Comment on attachment 285484 [details] [diff] [review]
Minimal Fix

r+. Actually, I had something similar as a first fix, but then changed to using an index for accessing the array elements... but anyway, I'm fine with this for the time being. (To be more consistent, NULL in the first assignment might be changed to nsnull).
Attachment #285484 - Flags: review?(mozbugzilla) → review+
Comment on attachment 285484 [details] [diff] [review]
Minimal Fix

let's get this obvious crash fix in
Attachment #285484 - Flags: review?(nelson) → approval1.9?
Keywords: crash
Comment on attachment 284989 [details] [diff] [review]
Prefered, but bigger Patch v2

Looks correct to me.

indent/new line style wise the for loop looks funky (we would adjust it to meet NSS styles if it were part of NSS), but if that's the standard style in mozilla, I'm ok with it.

bob
Attachment #284989 - Flags: review?(rrelyea) → review+
Setting target milestone - should be fixed for M9 in any case (either by the minimal fix or the preferred one).
Target Milestone: --- → mozilla1.9 M9
Comment on attachment 284989 [details] [diff] [review]
Prefered, but bigger Patch v2

I'm fine to land either patch.
Attachment #284989 - Flags: approvalM9?
Comment on attachment 285484 [details] [diff] [review]
Minimal Fix

explicitly requesting approval on the minimal fix for M9
Attachment #285484 - Flags: approvalM9?
Comment on attachment 285484 [details] [diff] [review]
Minimal Fix

a=drivers for M9 landing
Attachment #285484 - Flags: approvalM9?
Attachment #285484 - Flags: approvalM9+
Attachment #285484 - Flags: approval1.9?
Attachment #285484 - Flags: approval1.9+
Assignee: mozbugzilla → kengert
Status: ASSIGNED → NEW
Minimal fix checked in.

NOT marking as fixed. I'd let to get the better fix in, after we're done with M9 endgame.
fwiw, I can now use cz on trunk with prompt for cert. thanks!
Comment on attachment 284989 [details] [diff] [review]
Prefered, but bigger Patch v2

a=endgame drivers for after M9 freeze
Attachment #284989 - Flags: approvalM9?
Attachment #284989 - Flags: approvalM9-
Attachment #284989 - Flags: approval1.9+
Flags: blocking1.9? → blocking1.9+
Target Milestone: mozilla1.9 M9 → mozilla1.9 M10
Is this fixed?
Priority: -- → P1
(In reply to comment #21)
> Is this fixed?

No, a minimal fix was taken for M9. A better fix is going to be taken for M10 (see the two patches).
Whiteboard: [has-patch]
I backed out the minimal fix, and checked in the other, preferred patch.
Marking fixed.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.