1612583 - Crash in [@ arena_t::DallocSmall | je_free | PORT_FreeArena_Util | nsNSSCertificate::GetSubjectAltNames]

Status: RESOLVED DUPLICATE of bug 1685552

(NSS :: Libraries, defect, P3)

Description

[Randell Jesup [:jesup] (needinfo me)]

This bug is for crash report bp-5edc7aaf-cb99-4e5f-8ca7-166060200120.
Note: double-free, which one should assume may be a security issue

Top 10 frames of crashing thread:

0 mozglue.dll arena_t::DallocSmall memory/build/mozjemalloc.cpp:3251
1 mozglue.dll je_free memory/build/malloc_decls.h:54
2 nss3.dll PORT_FreeArena_Util security/nss/lib/util/secport.c:380
3 xul.dll void nsNSSCertificate::GetSubjectAltNames security/manager/ssl/nsNSSCertificate.cpp:597
4 xul.dll bool nsNSSCertificate::InitFromDER security/manager/ssl/nsNSSCertificate.cpp:126
5 xul.dll nsresult nsNSSCertificate::Read security/manager/ssl/nsNSSCertificate.cpp:1206
6 xul.dll nsBinaryInputStream::ReadObject xpcom/io/nsBinaryStream.cpp:941
7 xul.dll mozilla::psm::TransportSecurityInfo::Read security/manager/ssl/TransportSecurityInfo.cpp:573
8 xul.dll nsBinaryInputStream::ReadObject xpcom/io/nsBinaryStream.cpp:941
9 xul.dll NS_DeserializeObject netwerk/base/nsSerializationHelper.cpp:40

Comment 1

[J.C. Jones [:jcj] (he/they)]

Ben, can you give us an initial impact analysis?

Comment 2

[Daniel Veditz [:dveditz]]

This still happens on recent builds, but is (maybe?) more likely on ESR. In the last month 30/36 crashes, and 75/120 in the last three months. It's possible ESR crashes are processed at a higher rate, like beta and nightly.

Comment 3

[Daniel Veditz [:dveditz]]

Pretty consistently crashing on mainline through 71. In the last 3 months there was one 74rc1 crash. Maybe we can look at the files involved and see if there were changes that might have fixed this. We'd be looking for changes after 2019-10-21 when 71 merged to beta.

in mozjemalloc.cpp this looks worth checking out (though it's a little late):
Bug 1610720 - Change moz_dispose_arena to allow to free empty arenas. r=erahm

in nsNSSCertificate.cpp there were a couple of changes to certList around that time. I don't know if that's directly involved in this crash but could maybe come from the same arenas that we're freeing here? Bug 1592355 and bug 1580304

Since it's fixed on trunk do we need to fix it on ESR? The cases that do crash aren't a worry -- we have ourselves detected the bad situation and die. If we can show the double-free is happening within this stack then we don't have to worry about abuse. If it's a double-free due to a thread race or some other issue then we have to worry about the times it isn't crashing.

(of course crashing isn't great for stability, but this is pretty low volume and I doubt we'd worry about it on ESR)

Comment 4

[J.C. Jones [:jcj] (he/they)]

Ben, please mark this stalled and give a writeup of where you landed, what you learned.

Comment 5

[Benjamin Beurdouche [:beurdouche]]

Going through the old code (ESR 68, affected) and the current code (75, unaffected) multiple times
didn't allow me to find the problem in the ESR version. Getting the mini-dump for a crash didn't help
either, I went through the code with the visual studio debugger with the symbols and the source but
even that wasn't enough since the SubjectAltName value was apparently stripped out during data collection,
hence this led to yet another manual review, which was not enough for me to understand the issue.

Even though I am kind of disappointed, I think a fair amount of time has been spent here.
Since this is fixed in trunk and general consensus seems that it is low volume enough let's mark it stalled.

Comment 6

[J.C. Jones [:jcj] (he/they)]

Marking stalled, let's stop work unless we see something happening on newer branches again.

Comment 7

[Dana Keeler (she/her) [:keeler]]

nsNSSCertificate::GetSubjectAltNames was removed in bug 1685552, so this shouldn't be an issue.

Comment 8

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

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit auto_nag documentation.