1601389 - Crash in [@ SearchExtRunnable::~SearchExtRunnable]

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Wayne Mery (:wsmwk)]

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

Comment 1

[Ben Campbell]

Attachment: 1601389-fix-invalid-memory-access-in-ldap-search-1.patch

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

Comment 2

[Magnus Melin [:mkmelin]]

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

Comment 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[Ben Campbell]

Attachment: 1601389-fix-invalid-memory-access-in-ldap-search-2.patch

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

Comment 5

[Wayne Mery (:wsmwk)]

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

Comment 6

[Jorg K (CEST = GMT+2)]

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

Comment 7

[Ben Campbell]

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

Comment 8

[Jorg K (CEST = GMT+2)]

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

Comment 9

[Ben Campbell]

Attachment: 1601389-add-dtor-nullcheck-1.patch

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

Comment 10

[Jorg K (CEST = GMT+2)]

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bdb4d4a5b9fd
Fix invalid-memory access in LDAP search. r=jorgk

Comment 12

[Jorg K (CEST = GMT+2)]

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

Comment 13

[Geoff Lankow (:darktrojan)]

72 beta 2:
https://hg.mozilla.org/releases/comm-beta/rev/366d146a024904deb432103a766a1a06adf59aef

Comment 14

[Geoff Lankow (:darktrojan)]

68.3.1:
https://hg.mozilla.org/releases/comm-esr68/rev/57955f07b015186d453f6fa1155f9305cd564a22

Comment 15

[Wayne Mery (:wsmwk)]

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

Only seen in 68.3.0

Comment 16

[Ben Campbell]

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