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

Status: RESOLVED FIXED

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

Description

[Bob Clary [:bc] (inactive)]

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?

Comment 1

[Bob Clary [:bc] (inactive)]

#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

Comment 2

[Kaspar]

Attachment: Proposed fix

(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?

Comment 3

[Kai Engert (:KaiE:)]

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)

Comment 4

[Kai Engert (:KaiE:)]

Attachment: Prefered, but bigger Patch v2

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.

Comment 5

[Bob Clary [:bc] (inactive)]

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?

Comment 6

[Kai Engert (:KaiE:)]

(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.

Comment 7

[Kaspar]

(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.

Comment 8

[Kai Engert (:KaiE:)]

Attachment: Minimal Fix

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!

Comment 9

[Reed Loden [:reed]]

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 10

[Kai Engert (:KaiE:)]

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.

Comment 11

[Kaspar]

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).

Comment 12

[Kai Engert (:KaiE:)]

Comment on attachment 285484 [details] [diff] [review]
Minimal Fix

let's get this obvious crash fix in

Comment 13

[Robert Relyea]

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

Comment 14

[Kaspar]

Setting target milestone - should be fixed for M9 in any case (either by the minimal fix or the preferred one).

Comment 15

[Kai Engert (:KaiE:)]

Comment on attachment 284989 [details] [diff] [review]
Prefered, but bigger Patch v2

I'm fine to land either patch.

Comment 16

[Kai Engert (:KaiE:)]

Comment on attachment 285484 [details] [diff] [review]
Minimal Fix

explicitly requesting approval on the minimal fix for M9

Comment 17

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 285484 [details] [diff] [review]
Minimal Fix

a=drivers for M9 landing

Comment 18

[Kai Engert (:KaiE:)]

Minimal fix checked in.

NOT marking as fixed. I'd let to get the better fix in, after we're done with M9 endgame.

Comment 19

[Bob Clary [:bc] (inactive)]

fwiw, I can now use cz on trunk with prompt for cert. thanks!

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 284989 [details] [diff] [review]
Prefered, but bigger Patch v2

a=endgame drivers for after M9 freeze

Comment 21

[Robert Sayre]

Is this fixed?

Comment 22

[Reed Loden [:reed]]

(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).

Comment 23

[Kai Engert (:KaiE:)]

I backed out the minimal fix, and checked in the other, preferred patch.
Marking fixed.