1404220 - fsanitize=enum (ubsan) runtime errors for AudioSampleFormat

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[Arthur Edelstein [:arthur]]

Running automated tests with fsanitize=enum produces runtime errors like the following:

> [task 2017-09-25T00:32:22.754Z] 00:32:22     INFO - TEST-START | dom/media/webaudio/test/test_ScriptProcessorCollected1.html
> [task 2017-09-25T00:32:23.065Z] 00:32:23     INFO - GECKO(2111) | ++DOMWINDOW == 12 (0x7f73e038e800) [pid = 2161] [serial = 22] [outer = 0x7f73e16d5000]
> [task 2017-09-25T00:32:23.900Z] 00:32:23     INFO - GECKO(2111) | /builds/worker/workspace/build/src/dom/media/AudioSegment.h:150:8: runtime error: load of value 32627, which is not a valid value for type 'AudioSampleFormat'

Comment 1

[Paul Adenot (:padenot)]

Thanks. I've just made an UBSan build, I'll try to repro and fix soon.

Comment 2

[Paul Adenot (:padenot)]

Attachment: UBSan example mozconfig

Comment 3

[Paul Adenot (:padenot)]

I can repro more or less the same thing doing the following:
- Make an UBSan build using the attached mozconfig (for example)
- Run Firefox in a debugger (or in rr, etc.), breaking on `__ubsan_handle_load_invalid_value`
- Run a page that does:

> var ac = new AudioContext();
> var c = new ConvolverNode(ac);
> c.buffer = ac.createBuffer(2, 1024, 22050);

It breaks with the following stack:

> #0  mozilla::dom::AudioBuffer::GetThreadSharedChannelsForRate (this=0x60e00029b0a0, aJSContext=0x620000006080) at /home/padenot/src/trees/mozilla-unified/dom/media/webaudio/AudioBuffer.cpp:475
> #1  0x00007fd6146e6d9a in mozilla::dom::ConvolverNode::SetBuffer (this=0x611000847c80, aCx=0x620000006080, aBuffer=0x60e00029b0a0, aRv=...) at /home/padenot/src/trees/mozilla-unified/dom/media/webaudio/ConvolverNode.cpp:258
> #2  0x00007fd6122b3ad2 in mozilla::dom::ConvolverNodeBinding::set_buffer (cx=0x620000006080, obj=..., self=0x611000847c80, args=...) at /home/padenot/src/trees/mozilla-unified/objdir-ff-asan/dom/bindings/ConvolverNodeBinding.cpp:231
> ...

Inside, `GetThreadSharedChannelsForRate`, `mSharedChannels.IsNull()` returns `true`, `StealJSArrayDataIntoSharedChannels` returns `nullptr`, and we directly return the array, without setting the volume and the type. Returning from `GetThreadSharedChannelsForRate` to `ConvolverNode::SetBuffer`, which checks the type, that hasn't been initialized.

There are couple ways for fix this:
- Initialize the `mBufferFormat` (to float) and `mVolume` (to 1.0) in the AudioBuffer ctor, possibly overwriting them later (in case we're decoding to int16).
- Lazy initialize them in the specific case we're reaching here (in `GetThreadSharedChannelsForRate`, in the case where the buffers haven't been initialized yet).

Karl, do you have thoughts about this? The fact that I'm able to repro, but not on the case above, means this can happen with different Web Audio API objects, maybe it's best to aim to a catch-all solution.

Comment 4

[Karl Tomlinson (:karlt)]

This is kind of a regression from bug 1391482, but, when the wrong path is taken, the zero channel count seems to make everything OK AFAICS.

-fno-sanitize-recover=enum may be required to generate a crash.

I don't know why --enable-address-sanitizer is required, but the build failed without that.
Thank you for posting your config, Paul.

Comment 5

[Karl Tomlinson (:karlt)]

In my old mental model of C++, I would have interpreted the "only meaningful
if mBuffer is nonnull" documentation to suggest that
ConvolverNode::SetBuffer() should check !IsNull() before making a decision
based on mBufferFormat:
http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/dom/media/AudioSegment.h#257

However, that was when I assumed it was OK to copy an indeterminate value as
long as the destination was considered indeterminate.

I have since discovered that copying an indeterminate value, even with no
conversion, is undefined except with single byte chars.

https://stackoverflow.com/questions/15231527/is-it-ok-to-copy-uninitialized-data-if-it-will-be-unused-set-later/15231718#15231718
The quote is in N3337.

https://stackoverflow.com/questions/32997185/is-copying-trivially-copyable-objects-always-defined-in-c14/32997822#32997822
agrees.

The wording has been changed before N4296, but the exceptions are char.

8.5 Initializers

If no initializer is specified for an object, the object is
default-initialized. When storage for an object with automatic or dynamic
storage duration is obtained, the object has an indeterminate value, and if no
initialization is performed for the object, that object retains an
indeterminate value until that value is replaced (5.18). [ Note: Objects with
static or thread storage duration are zero-initialized, see 3.6.2. — end note
] If an indeterminate value is produced by an evaluation, the behavior is
undefined except in the following cases:

(12.1) — If an indeterminate value of unsigned narrow character type (3.9.1)
is produced by the evaluation of:
(12.1.1) — the second or third operand of a conditional expression (5.16),
(12.1.2) — the right operand of a comma expression (5.19),
(12.1.3) — the operand of a cast or conversion to an unsigned narrow character
type (4.7, 5.2.3, 5.2.9, 5.4),
or
(12.1.4) — a discarded-value expression (Clause 5),
then the result of the operation is an indeterminate value.

(12.2) — If an indeterminate value of unsigned narrow character type is
produced by the evaluation of the right operand of a simple assignment
operator (5.18) whose first operand is an lvalue of unsigned narrow character
type, an indeterminate value replaces the value of the object referred to by
the left operand.

(12.3) — If an indeterminate value of unsigned narrow character type is
produced by the evaluation of the initialization expression when initializing
an object of unsigned narrow character type, that object is initialized to an
indeterminate value.

[ Example:
int f(bool b) {
  unsigned char c;
  unsigned char d = c; // OK, d has an indeterminate value
  int e = d; // undefined behavior
  return b ? d : 0; // undefined behavior if b is true
}
— end example ]

Comment 6

[Karl Tomlinson (:karlt)]

With undefined behavior when copying indeterminate values, I think it is crazy
that it is undefined behavior to copy a value-initialized AudioChunk.
i.e. with the current declaration of AudioChunk, this provides undefined
behavior:

AudioChunk chunk = AudioChunk();

-fsanitize=enum does not detect that, but I don't know why.

We could make that have defined behavior by removing the user-defined default
AudioChunk constructor and replacing that with a default member initializer
(http://en.cppreference.com/w/cpp/language/data_members#Member_initialization).
AudioChunk() value-initialization would then first zero-initialize.

However, when AudioChunk is declared without an initializer (and without empty
parenthesis), as is typical, it is only default-initialized, which does not
initialize mBufferFormat.  We'd still have to resolve this bug, which could be
done, but this is a common pattern and each time we'd would need to
remember to set every member that does not have an initializing default
constructor, if that AudioChunk might be copied (or moved).  That seems to be
the design model of AudioChunk, but there are now so many members in this
class and some of them are already initialized by default constructors.

I think we'll probably save ourselves some trouble if we just default
initialize all members.  That's possibly useful even for most members, except
mDuration, I guess.  I'm thinking mDuration is not worth an exception though.
Often, though not for AudioBuffer I guess, I expect the compiler can work
out that the initial values are replaced.  When it can't, I don't expect this
to cost much.

(An alternative is to declare copy/move constructors and an assignment
operator that would not touch certain values when the AudioChunk is null, but
I'm not so keen on that because that would introduce additional branch
instructions.)

N4140

8.5 Initializers

8 To value-initialize an object of type T means:
(8.1) — if T is a (possibly cv-qualified) class type (Clause 9) with either no
default constructor (12.1) or a default constructor that is user-provided or
deleted, then the object is default-initialized;
(8.2) — if T is a (possibly cv-qualified) class type without a user-provided
or deleted default constructor, then the object is zero-initialized and the
semantic constraints for default-initialization are checked, and if T has a
non-trivial default constructor, the object is default-initialized;
(8.3) — if T is an array type, then each element is value-initialized;
(8.4) — otherwise, the object is zero-initialized.

11 An object whose initializer is an empty set of parentheses, i.e., (), shall
be value-initialized.

The form () is permitted in certain other initialization contexts (5.3.4,
5.2.3, 12.6.)

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Paul Adenot (:padenot) from comment #3)
> Karl, do you have thoughts about this? The fact that I'm able to repro, but
> not on the case above, means this can happen with different Web Audio API
> objects, maybe it's best to aim to a catch-all solution.

Yes, I can also reproduce on test_convolverNode.html but not
test_ScriptProcessorCollected1.html.

Much of WebAudio uses AudioBlock, which initializes format on allocation, but
there is still some use of AudioChunk, mostly with AudioBuffer, and so risk of
similar issues elsewhere, perhaps addressed by fixing AudioBuffer.

However, I'm in favor of a catch-all solution.  Have I convinced you?

Some patches at
https://treeherder.mozilla.org/#/jobs?repo=try&revision=28b6097e24830ddf5d746f46a8703584a8871b0d

Sorry if I'm intruding, but I felt like this was my fault.

Comment 8

[Paul Adenot (:padenot)]

(In reply to Karl Tomlinson (:karlt) from comment #7)
> However, I'm in favor of a catch-all solution.  Have I convinced you?
> 
> Some patches at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=28b6097e24830ddf5d746f46a8703584a8871b0d
> 
> Sorry if I'm intruding, but I felt like this was my fault.

We agree. I was going to write the patch if this was a clear, short and obvious fix, but since I came up with a couple different solutions and that there were unknowns, I decided to NI instead, since this was surfaced by the AudioBuffer/int16 patches.

I had a quick look at the patches, I think they would work.

Comment 9

[Arthur Edelstein [:arthur]]

I(In reply to Paul Adenot (:padenot) from comment #8)
> (In reply to Karl Tomlinson (:karlt) from comment #7)
> > However, I'm in favor of a catch-all solution.  Have I convinced you?
> > 
> > Some patches at
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=28b6097e24830ddf5d746f46a8703584a8871b0d

> I had a quick look at the patches, I think they would work.

I ran these patches through Mozilla automated tests with fsanitize=enum and I can confirm that all the detected AudioSampleFormat errors are gone now. Thanks, Karl and Paul!

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: bug 1404220 provide default initializers for all AudioChunk members

This avoids any risk of undefined behavior when evaluating uninitialized
members, during copies for example, and makes it safe to test mBufferFormat
when null.

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

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: bug 1404220 reset mBufferFormat (and mVolume) when resetting mSharedChannels

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

Comment 12

[Karl Tomlinson (:karlt)]

Attachment: bug 1404220 remove explicit unit volume AudioChunk settings, which are now initialized at construction

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

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: bug 1404220 assign zero to AUDIO_FORMAT_SILENCE enumerator

I don't know whether or not another zero initial AudioChunk member value
makes initialization more efficient, but zero for silence is more intuitive
for humans.

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

Comment 14

[Paul Adenot (:padenot)]

Comment on attachment 8916481 [details]
bug 1404220 provide default initializers for all AudioChunk members

https://reviewboard.mozilla.org/r/187604/#review192676

Comment 15

[Paul Adenot (:padenot)]

Comment on attachment 8916482 [details]
bug 1404220 reset mBufferFormat (and mVolume) when resetting mSharedChannels

https://reviewboard.mozilla.org/r/187606/#review192678

Comment 16

[Paul Adenot (:padenot)]

Comment on attachment 8916483 [details]
bug 1404220 remove explicit unit volume AudioChunk settings, which are now initialized at construction

https://reviewboard.mozilla.org/r/187608/#review192680

Comment 17

[Paul Adenot (:padenot)]

Comment on attachment 8916484 [details]
bug 1404220 assign zero to AUDIO_FORMAT_SILENCE enumerator

https://reviewboard.mozilla.org/r/187610/#review192682

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ccfa1fbb181c
provide default initializers for all AudioChunk members r=padenot
https://hg.mozilla.org/integration/autoland/rev/fe8cc992cbc9
reset mBufferFormat (and mVolume) when resetting mSharedChannels r=padenot
https://hg.mozilla.org/integration/autoland/rev/981fab1b34e8
remove explicit unit volume AudioChunk settings, which are now initialized at construction r=padenot
https://hg.mozilla.org/integration/autoland/rev/6ffe631f9038
assign zero to AUDIO_FORMAT_SILENCE enumerator r=padenot

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/ccfa1fbb181c
https://hg.mozilla.org/mozilla-central/rev/fe8cc992cbc9
https://hg.mozilla.org/mozilla-central/rev/981fab1b34e8
https://hg.mozilla.org/mozilla-central/rev/6ffe631f9038