1636408 - RLBox - RLBox ported components use MOZ_ASSERT instead of MOZ_RELEASE_ASSERT to check for malloc failures

Status: RESOLVED FIXED

(Core :: Audio/Video, defect, P3)

Description

[Shravan Narayan]

RLBox uses its own APIs to allocate memory instead of calling malloc or new so that it can allocate memory in the sandbox. Firefox uses a fallible malloc by default --- any malloc that fails and returns a null pointer crashes the program. Since rlbox's malloc is not fallible, we must explicitly add checks. In some of the places, these checks were performed with the macro 'MOZ_ASSERTinstead ofMOZ_RELEASE_ASSERT. These both check a condition and callsMOZ_CRASHto terminate firefox if the condition is not met, however the former only crashes in debug builds. We need to replace uses ofMOZ_CRASHwithMOZ_RELEASE_CRASH`.

The incorrect checks are in the OGG demuxer files
dom/media/ogg/OggCodecState.cpp
dom/media/ogg/OggDemuxer.cpp

Comment 1

[Aditi Dutta]

Attachment: Bug 1636408 - Replaced checks for malloc failures in RLBox ported components to use MOZ_RELEASE_ASSERT instead of MOZ_ASSERT.

Comment 2

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Terminology nits:

Firefox uses a fallible malloc by default --- any malloc that fails and returns a null pointer crashes the program.

I believe this is infallible/not fallible. I.e. memory allocation cannot fail -- if it does it is fatal.

Since rlbox's malloc is not fallible, we must explicitly add checks.

In this case rlbox's malloc would be fallible. I.e. it can fail and return NULL.

Comment 3

[Deian Stefan]

I wonder if we should eventially turn the sanbox_malloc into an infalliable call too; there is probably no good reason to allow failures and avoids bugs like this.

Comment 4

[Bryce Seager van Dyk [:bryce] (he/him) - Not reading bugmail]

Within media there are some cases where we want fallible allocation (return null, don't kill the process, gracefully handle in code) because files may contain large blocks of data that we could realistically fail allocating buffers for. I don't know if there are cases like this for the sandboxed case, but it is an argument for fallible allocation. That said, we still default to infallible allocation in code that does this (mp4 parsing, for example), and explicitly invoke fallible allocation when needed.

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Deian Stefan from comment #3)

I wonder if we should eventially turn the sanbox_malloc into an infalliable call too; there is probably no good reason to allow failures and avoids bugs like this.

Providing sandbox_xmalloc for this sort of thing would be useful, but like Bryce said, I think we do want both variants. I could see cases where we can handle allocation failures gracefully in sandboxed code.

Comment 6

[Deian Stefan]

Of course! (Sorry I didn't mean to imply not having a falliable version.)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9dee0cfaa60f
Replaced checks for malloc failures in RLBox ported components to use MOZ_RELEASE_ASSERT instead of MOZ_ASSERT. r=deian,froydnj

Comment 8

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/9dee0cfaa60f