Closed Bug 1601389 Opened 7 months ago Closed 7 months ago

Crash in [@ SearchExtRunnable::~SearchExtRunnable]

Categories

(MailNews Core :: LDAP Integration, defect)

Unspecified
All
defect
Not set
critical

Tracking

(thunderbird_esr68 verified, thunderbird72 fixed, thunderbird73 fixed)

RESOLVED FIXED
Thunderbird 73.0
Tracking Status
thunderbird_esr68 --- verified
thunderbird72 --- fixed
thunderbird73 --- fixed

People

(Reporter: wsmwk, Assigned: benc)

References

(Regression)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 2 obsolete files)

This crash is new to 68.3.0. Also seen in 71 and 72 beta bp-81cb72dd-2e00-4936-8949-e666d0191204

This bug is for crash report bp-7005e466-331d-4f2a-97df-3fd3e0191204.

Top 10 frames of crashing thread:

0 XUL SearchExtRunnable::~SearchExtRunnable comm/ldap/xpcom/src/nsLDAPOperation.cpp:481
1 XUL SearchExtRunnable::~SearchExtRunnable comm/ldap/xpcom/src/nsLDAPOperation.cpp:475
2 XUL mozilla::Runnable::Release xpcom/threads/nsThreadUtils.cpp:51
3 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1222
4 XUL <name omitted> xpcom/threads/ThreadEventTarget.cpp:163
5 XUL NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
6 XUL mozilla::net::nsSocketTransportService::Run netwerk/base/nsSocketTransportService2.cpp:1013
7 XUL XUL@0x50b0cf 
8 XUL non-virtual thunk to mozilla::net::nsSocketTransportService::Run netwerk/base/nsSocketTransportService2.cpp
9 XUL nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1175

Flags: needinfo?(benc)

Oop, that code has my fingerprints all over it!
I think this patch should fix it...

Assignee: nobody → benc
Flags: needinfo?(benc)
Attachment #9113643 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9113643 [details] [diff] [review]
1601389-fix-invalid-memory-access-in-ldap-search-1.patch

Review of attachment 9113643 [details] [diff] [review]:
-----------------------------------------------------------------

::: ldap/xpcom/src/nsLDAPOperation.cpp
@@ +560,5 @@
>    // Convert our comma separated string to one that the C-SDK will like, i.e.
>    // convert to a char array and add a last NULL element.
>    nsTArray<nsCString> attrArray;
>    ParseString(aAttributes, ',', attrArray);
>    char **attrs = nullptr;

you can move this down two lines to where it's first used now

@@ +563,5 @@
>    ParseString(aAttributes, ',', attrArray);
>    char **attrs = nullptr;
>    uint32_t origLength = attrArray.Length();
> +  attrs = static_cast<char **>(moz_xmalloc((origLength + 1) * sizeof(char *)));
> +  if (!attrs) return NS_ERROR_OUT_OF_MEMORY;

the out of memory can go, no? Isn't moz_xmalloc guanarteed to work (or by it self, cause the program to abort)?
Attachment #9113643 - Flags: review?(mkmelin+mozilla) → review+
Status: NEW → ASSIGNED
Component: General → LDAP Integration
Product: Thunderbird → MailNews Core
Comment on attachment 9113643 [details] [diff] [review]
1601389-fix-invalid-memory-access-in-ldap-search-1.patch

Review of attachment 9113643 [details] [diff] [review]:
-----------------------------------------------------------------

::: ldap/xpcom/src/nsLDAPOperation.cpp
@@ +563,5 @@
>    ParseString(aAttributes, ',', attrArray);
>    char **attrs = nullptr;
>    uint32_t origLength = attrArray.Length();
> +  attrs = static_cast<char **>(moz_xmalloc((origLength + 1) * sizeof(char *)));
> +  if (!attrs) return NS_ERROR_OUT_OF_MEMORY;

Hmm, `new` is infallible in Mozilla, I'm not sure about moz_xmalloc. We'd have to check.

Huh. Turns out that moz_xmalloc() is infallible after all. It invokes MOZ_CRASH() in OOM situations and never returns NULL.
Not what I expected, but nice to know!
Patch tweaked accordingly.
(also changed the terminating pointer array entry from 0 to nullptr while I was there).

Attachment #9113643 - Attachment is obsolete: true
Attachment #9113988 - Flags: review+

What is the regressing bug? (I've lost track of it)

Severity: normal → critical
Flags: needinfo?(benc)
Flags: needinfo?(benc)
Regressed by: 1576364
Version: unspecified → 72
Attachment #9113988 - Flags: approval-comm-esr68?
Attachment #9113988 - Flags: approval-comm-beta+

Looking at this: Why don't you just add if (!mAttrs) return; in the DTOR and leave the rest as it is?

I did consider that, but felt it left the code nicer if I removed the special case.

But why go though the hassle of allocating a one-entry array (which is null anyway) just do de-allocate it later?

(In reply to Jorg K (GMT+1) (PTO to 15th Dec 2019, sporadically reading bugmail) from comment #8)

But why go though the hassle of allocating a one-entry array (which is null anyway) just do de-allocate it later?

Because it removes two special-case conditional paths (one at the allocation, and one in the dtor).
I agree that it's ugly to allocate emptiness, but simplicity over efficiency seemed like a good tradeoff (there's always some idiot who's going to forget to check for null in the dtor ;- )

Anyway - alternate oneliner patch attached, if you prefer it. I'm happy either way as long as my original cock-up gets fixed!

Attachment #9113988 - Flags: approval-comm-esr68?
Attachment #9113988 - Flags: approval-comm-beta+
Comment on attachment 9114443 [details] [diff] [review]
1601389-add-dtor-nullcheck-1.patch

I don't think two additional checks are going to cause a mental heap overflow of any programmer ;-)
Attachment #9114443 - Flags: review+
Attachment #9114443 - Flags: approval-comm-esr68+
Attachment #9114443 - Flags: approval-comm-beta+
Attachment #9113988 - Attachment is obsolete: true

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/bdb4d4a5b9fd
Fix invalid-memory access in LDAP search. r=jorgk

Status: ASSIGNED → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 73.0

Uff, I should have commented on the commit message :-(

Target Milestone: Thunderbird 73.0 → Thunderbird 72.0

Is this the same issue?
nsLDAPSSLClose bp-1b1dcb21-920c-48f5-8696-9bc730191210

Only seen in 68.3.0

Flags: needinfo?(benc)

It could definitely be something I screwed up, but I think it's a different issue. Open a new bug for it?

Flags: needinfo?(benc)
Target Milestone: Thunderbird 72.0 → Thunderbird 73.0
You need to log in before you can comment on or make changes to this bug.