Closed Bug 492385 Opened 11 years ago Closed 11 years ago

crash freeing named CRL entry on shutdown

Categories

(NSS :: Libraries, defect, P1)

defect

Tracking

(Not tracked)

RESOLVED FIXED
3.12.4

People

(Reporter: blassey, Assigned: hiro)

References

Details

(Keywords: mobile)

Attachments

(2 files, 2 obsolete files)

Steps to reproduce:
1) Launch Fennec with js console
2) Install Add-on
3) Close Fennec Window (don't click the restart button)
4) Using Today Screen's task manager, close the error console

The steps maybe simpler than that, but I've reproduced the same crash twice with these steps.

stack:
 	0x03f9ff64	
>	nssutil3.dll!PORT_ZFree_Util(void* ptr = 0x79734165, unsigned int len = 2768266094) Line: 161, Byte Offsets: 0x28	C
 	nssutil3.dll!SECITEM_ZfreeItem_Util(SECItemStr* zap = 0x68ea9570, int freeit = 1) Line: 278, Byte Offsets: 0x34	C
 	nss3.dll!NamedCRLCacheEntry_Destroy(NamedCRLCacheEntryStr* entry = 0x68eb0370) Line: 1321, Byte Offsets: 0x78	C
 	nss3.dll!FreeNamedEntries(PLHashEntry* he = 0x68ea94e0, int i = 2, void* arg = 0x2fbbed10) Line: 1372, Byte Offsets: 0xbc	C
 	0x010c2b1c	
 	nss3.dll!ShutdownCRLCache(void) Line: 1426, Byte Offsets: 0x118	C
 	nss3.dll!NSS_Shutdown(void) Line: 883, Byte Offsets: 0x64	C
 	xul.dll!nsNSSComponent::ShutdownNSS(void) Line: 1750, Byte Offsets: 0x1c8	C++
 	xul.dll!nsNSSComponent::DoProfileBeforeChange(nsISupports* aSubject = 0x00000000) Line: 2494, Byte Offsets: 0xe0	C++
 	xul.dll!nsNSSComponent::Observe(nsISupports* aSubject = 0x00000000, const char* aTopic = 0xa500636e, const wchar_t* someData = 0x79734165) Line: 2167, Byte Offsets: 0x168	C++
 	xul.dll!nsObserverList::NotifyObservers(nsISupports* aSubject = 0x00000000, const char* aTopic = 0xa500636e, const wchar_t* someData = 0x79734165) Line: 128, Byte Offsets: 0xa8	C++
 	xul.dll!nsObserverService::NotifyObservers(nsISupports* aSubject = 0x00000000, const char* aTopic = 0xa500636e, const wchar_t* someData = 0x79734165) Line: 184, Byte Offsets: 0x10c	C++
 	xul.dll!nsXREDirProvider::DoShutdown(void) Line: 878, Byte Offsets: 0x1c4	C++
 	xul.dll!ScopedXPCOMStartup::~ScopedXPCOMStartup(void) Line: 993, Byte Offsets: 0x48	C++
 	xul.dll!XRE_main(int argc = 2037596517, char** argv = 0x00000000, nsXREAppData* aAppData = 0xa500636e) Line: 3407, Byte Offsets: 0x2604	C++
 	0x00012220	
 	0x00012404	
 	0x0001295c	
 	0x03f67274
Guys, we need to know which NSS CVS tag you're using.
Assignee: nobody → alexei.volkov.bugs
Priority: -- → P1
Summary: crash [@PORT_ZFree_Util] on shutdown → crash freeing named CRL entry on shutdown
Target Milestone: --- → 3.12.4
NSS 3.12.3
This may have already been fixed on the CVS trunk.  
Can you try a test using NSS built from the CVS trunk?

If it's not, then it must wait until the two guys who work on that code 
return, one from vacation and the other from sick leave.
Version: unspecified → 3.12.3
I am back from sick leave.
However, the stack provided does not match NSS 3.12.3 . The NamedCRL code didn't get added until later.
Please provide an actual NSS CVS tag.
The NamedCRL code was added on the trunk for 3.12.4, which hasn't RTM'ed yet. It was added over a weekend a few weeks ago. The last change was on 4/24. The CVS revisions for crl.c are 1.62 through 1.66 . Please indicate which one you have.

Looking at the stack in comment 0, I cannot exactly tell which of the two SECITEM_ZfreeItem you are tripping on. It could be the one freeing entry->crl, or entry->canonicalizedNamed . Can you figure out which one of the two it is ?
A debug build would help a lot if you don't already have one. There are a lot of assertions in the code to detect corruption problems in the hash table. Also, please print out the content of the entry structure when you get this crash.

I just reviewed the code and found a minor problem, but it would only occur if the NSPR hash table calls fail. I don't think it will solve your problem. But I will attach a patch.
Version: 3.12.3 → trunk
Comment on attachment 376829 [details] [diff] [review]
Don't destroy entries if they couldn't be removed from the hash table

Julien, I have a question about this patch.  It does what you said it does,
but I wonder if that is what we want to do.

Q: If an attempt to remove an entry from a hash table fails, 
is the correct interpretation that the entry is still in the table?  
Or is it that the entry was already not in the table?  

Thinking out loud here ... I seem to recall seeing some warnings about
attempts to insert entries into the table that are already in the table.
I wonder if perhaps the code tries to remove an entry as many times as
it has tried to insert it, so that the first attempt to remove succeeds
and the rest fail.  Maybe we need reference counting on these entries...
Nelson,

The old entry was obtained from the table with cert_FindCRLByGeneralName . See http://mxr.mozilla.org/security/source/security/nss/lib/certdb/crl.c#3170 . The table is locked. So, we must assume that the old entry is still in the table. I don't know of any good reason why the NSPR hash table call to remove the old entry would fail. I am just trying to do proper error checking. This is probably a more theoritical case than anything else.

If we insert a new entry in the table with the same index, the old entry is replaced in the table. Unless somebody still has a pointer to that old entry and subsequently frees it, that old entry is leaked.

The comment about the leak is at the time of the old entry removal failure, though the actual leak happens later when a new entry is inserted to replace it.

In summary, yes, I think the patch is doing the right thing.
Attachment #376829 - Flags: review?(nelson) → review+
Comment on attachment 376829 [details] [diff] [review]
Don't destroy entries if they couldn't be removed from the hash table

I'm not wild about this, but I'd rather have it leak than crash.
I confirmed this crash.

The CVS tag is NSS_3_12_4_FIPS1 according to bug 490864.
(In reply to comment #5)
> Looking at the stack in comment 0, I cannot exactly tell which of the two
> SECITEM_ZfreeItem you are tripping on. 

In my case, it was entry->crl. I think the length in PORT_ZFree is incredibly big (it is 2768266094 in comment #1). Is it an ordinary value?
Attached patch Clear entry->crl address with 0 (obsolete) — Splinter Review
entry->crl should be cleared with 0 when CERT_CacheCRL fails in addCRLToCache.
Assignee: alexei.volkov.bugs → ikezoe
Status: NEW → ASSIGNED
Attachment #377095 - Flags: superreview?(nelson)
Attachment #377095 - Flags: review?(julien.pierre.boogz)
Attachment 37705 [details] [diff] fixes this crash in my case.
Comment on attachment 377095 [details] [diff] [review]
Clear entry->crl address with 0

Hiroyuki,

Good catch. The assignment should be of NULL, not 0, however.

The SECItem length field should not be large. It probably indicated a corrupt/previously freed block.

I will need to start running the browser with the current NSS code manually, since unfortunately our QA tests still have zero coverage for this code.
Attachment #377095 - Flags: review?(julien.pierre.boogz) → review+
Attached patch Update (obsolete) — Splinter Review
Use NULL instead of 0.
Thank you, Julien.
Attachment #377095 - Attachment is obsolete: true
Attachment #377142 - Flags: superreview?(nelson)
Attachment #377142 - Flags: review+
Attachment #377095 - Flags: superreview?(nelson)
I don't SR Hg patches. 
Turn this into a CVS patch which works with bugzilla's diff context tool,
and then I'll be happy to SR it.
Nelson, out of curiosity, what's not working for you with bugzilla's diff context tool?
Attached patch CVS diffSplinter Review
I could not find bugzilla's diff context tool so I recreated CVS diff.
Attachment #377142 - Attachment is obsolete: true
Attachment #377270 - Flags: superreview+
Attachment #377270 - Flags: review+
Attachment #377142 - Flags: superreview?(nelson)
Comment on attachment 377270 [details] [diff] [review]
CVS diff

Oops! I am sorry. I checked sr+ by myself.
Attachment #377270 - Flags: superreview+ → superreview?(nelson)
Brad,

What's not working is that :

a) we can't tell what revision of the source the patch is made against
b) we can't extended diffs . When clicking on diff, if typing a larger number of lines, it doesn't work.

Hiroyuki,

Thanks for attaching a CVS patch. Please note that we never "carry over" r+ or sr+ for NSPR or NSS patches . All new patches need new reviews. Right now, the NSS trunk only needs one review.
Attachment #377142 - Flags: review+
Attachment #377270 - Flags: review+ → review?(julien.pierre.boogz)
Attachment #377270 - Flags: review?(julien.pierre.boogz) → review+
(In reply to comment #20)

> Hiroyuki,
> 
> Thanks for attaching a CVS patch. Please note that we never "carry over" r+ or
> sr+ for NSPR or NSS patches . All new patches need new reviews. Right now, the
> NSS trunk only needs one review.

Thank you for your advices. I will care about these rules next time.
In reply to comment 17 and comment 18,
Go click on the "diff" link for that Hg patch in the bug above.
That will take you to 
    https://bugzilla.mozilla.org/attachment.cgi?id=377142&action=diff
At the top of that page (and any diff page), you will see 
     Context:  (Patch / File / [3  ]) 
That is bugzilla's diff context tool.  It allows you to see the patch with
different amounts of context.  Click on "File", or change "3" to some other
number (I commonly use 32), and press enter.  See what you get. 

Now, try it with the CVS patch, and see how it works with that. 
https://bugzilla.mozilla.org/attachment.cgi?id=377270&action=diff
(Thanks for the CVS patch, Hiroyuki!)

It works with the CVS patch, but not with the Hg patch.  
There is a bugzilla bug against bugzilla about this.

But frankly, given that the CVS repository, and not the Hg repositoriry,
is the upstream repository for NSS and NSPR, patches should be against
the upstream repository, not against some downstream repository.
Comment on attachment 377270 [details] [diff] [review]
CVS diff

sr=nelson, thanks!
Attachment #377270 - Flags: superreview?(nelson) → superreview+
I checked in attachment 376829 [details] [diff] [review] and attachment 377270 [details] [diff] [review] to the trunk.

Checking in crl.c;
/cvsroot/mozilla/security/nss/lib/certdb/crl.c,v  <--  crl.c
new revision: 1.67; previous revision: 1.66
done
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
OS: Windows Mobile 6 Professional → All
Hardware: ARM → All
(In reply to comment #22)
> In reply to comment 17 and comment 18,
> Go click on the "diff" link for that Hg patch in the bug above.
> That will take you to 
>     https://bugzilla.mozilla.org/attachment.cgi?id=377142&action=diff
> At the top of that page (and any diff page), you will see 
>      Context:  (Patch / File / [3  ]) 
> That is bugzilla's diff context tool.  It allows you to see the patch with
> different amounts of context.  Click on "File", or change "3" to some other
> number (I commonly use 32), and press enter.  See what you get. 
>
> Now, try it with the CVS patch, and see how it works with that. 
> https://bugzilla.mozilla.org/attachment.cgi?id=377270&action=diff

I did not aware this. You are very kind!
Duplicate of this bug: 494661
Duplicate of this bug: 500675
Duplicate of this bug: 501322
Duplicate of this bug: 517841
You need to log in before you can comment on or make changes to this bug.