1295688 - InfallibleAllocPolicy should crash on overflow

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Andrew McCreight [:mccr8]]

Code that uses InfallibleAllocPolicy presumably wants for operations to always succeed. However, Vector and HashTable can end up detecting that growing the data structure more will fail due to integer overflow, and then will call reportAllocOverflow() and fail. I think these should crash. It could also do an OOM crash with the max value of size_t, but maybe that's too cute.

Comment 1

[Nicholas Nethercote [inactive]]

Equally alarming: InfallibleAllocPolicy::pod_{m,re}alloc will return nullptr if the size computation overflows!

FWIW, DMD's own InfallibleAllocPolicy *does* crash in pod_{m,re}alloc and reportAllocOverflow().

Comment 2

[Andrew McCreight [:mccr8]]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> Equally alarming: InfallibleAllocPolicy::pod_{m,re}alloc will return nullptr
> if the size computation overflows!

Oh, good point! I even looked at that code but I failed to connect the dots. I'll fix that, too.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Bug 1295688 - InfallibleAllocPolicy should crash on overflow.

Code that uses InfallibleAllocPolicy presumably wants for operations
to always succeed. However, Vector and HashTable can end up detecting
that growing the data structure will fail due to integer overflow, and
then will call reportAllocOverflow() and fail. I think these cases
should crash.

In addition, pod_malloc and pod_realloc should crash rather than
returning NULL when they detect overflow.

This calls mozalloc_abort rather than MOZ_CRASH directly to avoid
circular #includes.

Review commit: https://reviewboard.mozilla.org/r/72740/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72740/

Comment 4

[Andrew McCreight [:mccr8]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=833e56932029

Comment 5

[Mike Hommey [:glandium]]

Comment on attachment 8782680 [details]
Bug 1295688 - InfallibleAllocPolicy should crash on overflow.

https://reviewboard.mozilla.org/r/72740/#review71314

::: memory/mozalloc/mozalloc.h:30
(Diff revision 1)
>  #  include <string.h>
>  #endif
>  
>  #if defined(__cplusplus)
>  #include "mozilla/fallible.h"
> +#include "mozilla/mozalloc_abort.h"

Can you tell what kind of circular #includes you're avoiding? This file is already including things from MFBT, how does adding Assertions.h to the list make things worse?

::: memory/mozalloc/mozalloc.h:296
(Diff revision 1)
>  public:
>      template <typename T>
>      T* pod_malloc(size_t aNumElems)
>      {
>          if (aNumElems & mozilla::tl::MulOverflowMask<sizeof(T)>::value) {
> -            return nullptr;
> +            mozalloc_abort("overflow in pod_malloc");

Is there value having a separate message here? Why not simply call reportAllocOverflow?

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8782680 [details]
Bug 1295688 - InfallibleAllocPolicy should crash on overflow.

https://reviewboard.mozilla.org/r/72740/#review71316

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Mike Hommey [:glandium] from comment #5)
> Can you tell what kind of circular #includes you're avoiding? This file is
> already including things from MFBT, how does adding Assertions.h to the list
> make things worse?

I get errors like this in other files when I include Assertions.h and use MOZ_CRASH in mozalloc.h:

[...]
In file included from [...]/dist/include/mozilla/Assertions.h:22:
In file included from [...]/dist/include/nsTraceRefcnt.h:10:
In file included from [...]/dist/include/nscore.h:20:
[...]dist/include/mozilla/mozalloc.h:324:9: error: use of undeclared identifier 'MOZ_CRASH'

Assertions.h uses nsTraceRefcnt::WalkTheStack().

> Is there value having a separate message here? Why not simply call reportAllocOverflow?

That's a good point, I'll do that.

Comment 8

[Andrew McCreight [:mccr8]]

Comment on attachment 8782680 [details]
Bug 1295688 - InfallibleAllocPolicy should crash on overflow.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/72740/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c4d874b35d35
InfallibleAllocPolicy should crash on overflow. r=glandium

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c4d874b35d35