1273711 - Crash in OOM | large | mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | NS_strdup | nsSecretDecoderRing::encode

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Nicholas Nethercote [inactive]]

This bug was filed from the Socorro interface and is 
report bp-c16e6165-1858-4b10-b121-224652160507.
=============================================================

This crash was an OOM where we used NS_strdup(), which is infallible, to duplicate a string of size 330,805,469 bytes.

The relevant code is this:

> char* result = PL_Base64Encode((const char*)data, dataLen, nullptr);
> if (!result) {
>   return NS_ERROR_OUT_OF_MEMORY;
> }
> 
> *_retval = NS_strdup(result);
> PR_DELETE(result);
> if (!*_retval) {
>   return NS_ERROR_OUT_OF_MEMORY;
> }

The obvious thing to do is change NS_strdup() to strdup(), which is fallible. But we can do better than that! This strdup is required because PR_MALLOC (used by PL_Base64Encode) might use a different allocator to NS_strdup(). But it turns out you can pass in a pre-allocated buffer to PL_Base64Encode(). So we can avoid the strdup altogether.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Avoid OOM aborts in nsSecretDecoderRing::encode()

This patch removes an infallible duplication of the base64-encoded string,
which can get large.

Comment 2

[:Cykesiopka]

Comment on attachment 8753617 [details] [diff] [review]
Avoid OOM aborts in nsSecretDecoderRing::encode()

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

Yikes. Thanks for fixing this!
r+ assuming the things below are fixed.

::: security/manager/ssl/nsSDR.cpp
@@ +296,5 @@
>  
>  // Support routines
>  
>  nsresult
>  nsSecretDecoderRing::encode(const unsigned char* data, int32_t dataLen,

It looks like we have an int32_t |dataLen| here, but a uint32_t |aBinaryLen| in the new Base64Encode().

I think an explicit check to ensure |dataLen| is non-negative and an AssertedCast should be added, if only to make it clear that encode() is doing the right thing.

(I imagine it would be better to modify nsISecretDecoderRing.idl to get rid of this mismatch, but the file lives under netwerk/ for some reason. Probably follow up material.)

@@ +299,5 @@
>  nsresult
>  nsSecretDecoderRing::encode(const unsigned char* data, int32_t dataLen,
>                              char** _retval)
>  {
> +  return mozilla::Base64Encode((const char*)data, dataLen, _retval);

Nit: Please use BitwiseCast<const char*, const unsigned char*> instead of the C style cast - in Bug 1271501 I'm moving PSM away from reinterpret_cast, and C style casts as I encounter them.

The mozilla:: qualifier can be dropped to counter the longer line length.

Comment 3

[Nicholas Nethercote [inactive]]

Thank you for the fast review.

> Nit: Please use BitwiseCast<const char*, const unsigned char*> instead of
> the C style cast - in Bug 1271501 I'm moving PSM away from reinterpret_cast,
> and C style casts as I encounter them.

Ok. What is the difference between reinterpret_cast and BitwiseCast? mfbt/Casting.h doesn't explain in what situations BitwiseCast is best used...

Comment 4

[:Cykesiopka]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> Ok. What is the difference between reinterpret_cast and BitwiseCast?
> mfbt/Casting.h doesn't explain in what situations BitwiseCast is best used...

AFAICT, they're functionally the same, but BitwiseCast provides static asserts and more type checking to prevent people from shooting themselves in the foot.

As such, I'm going with "just use BitwiseCast for safety unless someone tells me a reason not to". Dunno if there are more "official" guidelines.

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8753617 [details] [diff] [review]
Avoid OOM aborts in nsSecretDecoderRing::encode()

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

::: security/manager/ssl/nsSDR.cpp
@@ +296,5 @@
>  
>  // Support routines
>  
>  nsresult
>  nsSecretDecoderRing::encode(const unsigned char* data, int32_t dataLen,

Drive-by: this function only gets called once - why not just get rid of it and replace the call with this call to mozilla::Base64Encode?

@@ +304,4 @@
>  }
>  
>  nsresult
>  nsSecretDecoderRing::decode(const char* data, unsigned char** result,

Can we fix/remove this function too, while we're here?

Comment 6

[Nicholas Nethercote [inactive]]

> 
> Drive-by: this function only gets called once - why not just get rid of it
> and replace the call with this call to mozilla::Base64Encode?
>
> Can we fix/remove this function too, while we're here?

Can we do these in a follow-up?

Comment 7

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Sure thing.

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/467c3250de1cf49a5904a4f3b5e9763f5dde69a6
Bug 1273711 - Avoid OOM aborts in nsSecretDecoderRing::encode(). r=cykesiopka.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/467c3250de1c

Comment 10

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Nicholas Nethercote [:njn] from comment #6)
> Can we do these in a follow-up?

I filed bug 1275309.