Bug 223821 – UBSan: runtime error: load of value <unknown>, which is not a valid value for type 'const WebCore::RealtimeMediaSourceCapabilities::EchoCancellation'

• Component: WebRTC
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Fix runtime issues found by UBSan running layout tests:

Source/WebKit/Platform/IPC/Encoder.h:68:93: runtime error: load of value <unknown>, which is not a valid value for type 'const WebCore::RealtimeMediaSourceCapabilities::EchoCancellation'

Found in the following tests:

LayoutTests/fast/mediastream/MediaDevices-addEventListener.html
LayoutTests/fast/mediastream/constraint-intrinsic-size.html
LayoutTests/http/tests/media/media-stream/enumerate-devices-iframe-allow-attribute.html
LayoutTests/http/tests/media/media-stream/get-display-media-iframe-allow-attribute.html
LayoutTests/imported/w3c/web-platform-tests/mediacapture-record/MediaRecorder-bitrate.https.html
LayoutTests/webrtc/addICECandidate-closed.html
LayoutTests/webrtc/ice-candidate-sdpMLineIndex.html
LayoutTests/webrtc/libwebrtc/descriptionGetters.html
LayoutTests/webrtc/peerconnection-page-cache.html

[David Kilzer (:ddkilzer)]

Created attachment 424410 [details]
Patch v1

[Darin Adler]

Comment on attachment 424410 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=424410&action=review

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:110
> +    ValueUnion m_minOrValue { 0 };
> +    ValueUnion m_max { 0 };

In cases like this, I am pretty sure we can use just "{ }" instead of "{ 0 }" if we prefer. Has the same effect.

But also I am not sure this makes logical sense. The double is still uninitialized, so what’s the value to just initializing the int here?

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:111
> +    Type m_type { Undefined };

Could do the same here, but here I think there is more value in writing out Undefined, so I am not recommending it.

> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:209
> +    EchoCancellation m_echoCancellation { EchoCancellation::ReadOnly };

Same.

[David Kilzer (:ddkilzer)]

Comment on attachment 424410 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=424410&action=review

>> Source/WebCore/platform/mediastream/RealtimeMediaSourceCapabilities.h:110
>> +    ValueUnion m_max { 0 };
> 
> In cases like this, I am pretty sure we can use just "{ }" instead of "{ 0 }" if we prefer. Has the same effect.
> 
> But also I am not sure this makes logical sense. The double is still uninitialized, so what’s the value to just initializing the int here?

Verified that "{ }" is the same as "{ 0 }" for ValueUnion.  Will change.

Only the first non-static union member can be initialized with C++ member initializer syntax, so I swapped asInt and asDouble in ValueUnion to clear all memory since the double is 8 bytes and the int is 4 bytes.

[David Kilzer (:ddkilzer)]

Created attachment 424463 [details]
Patch for landing

[EWS]

Committed r275142: <https://commits.webkit.org/r275142>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 424463 [details].

[Radar WebKit Bug Importer]

<rdar://problem/75924950>

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #3)
> Only the first non-static union member can be initialized with C++ member
> initializer syntax, so I swapped asInt and asDouble in ValueUnion to clear
> all memory since the double is 8 bytes and the int is 4 bytes.

That works from a practical point of view, but not great from an "undefined behavior" point of view.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #7)
> (In reply to David Kilzer (:ddkilzer) from comment #3)
> > Only the first non-static union member can be initialized with C++ member
> > initializer syntax, so I swapped asInt and asDouble in ValueUnion to clear
> > all memory since the double is 8 bytes and the int is 4 bytes.
> 
> That works from a practical point of view, but not great from an "undefined
> behavior" point of view.

Can you give an example of a "undefined behavior" point of view that you're thinking of?

In this specific case, setting the double value to 0 will clear all the memory that could be used by the members of this particular union.

Or do you mean this in the more general sense of "clearing all memory used by a union used as a C++ member variable"?

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #8)
> In this specific case, setting the double value to 0 will clear all the
> memory that could be used by the members of this particular union.

That’s not specified by the standard. Setting the double value to 0 does not guarantee anything about the int value.

That’s the problem with "undefined behavior sanitizer"; where do you draw the line?

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #9)
> (In reply to David Kilzer (:ddkilzer) from comment #8)
> > In this specific case, setting the double value to 0 will clear all the
> > memory that could be used by the members of this particular union.
> 
> That’s not specified by the standard. Setting the double value to 0 does not
> guarantee anything about the int value.

I see.

> That’s the problem with "undefined behavior sanitizer"; where do you draw
> the line?

UBSan only flagged the m_echoCancellation member variable.  It did not flag the ValueUnion fields.  I noticed them via source code inspection.  Sorry if this wasn't clear in my original patch.

Maybe a better solution to using unions is to use WTF::Variant instead.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #10)
> (In reply to Darin Adler from comment #9)
> > That’s the problem with "undefined behavior sanitizer"; where do you draw
> > the line?
> 
> UBSan only flagged the m_echoCancellation member variable.  It did not flag
> the ValueUnion fields.  I noticed them via source code inspection.  Sorry if
> this wasn't clear in my original patch.
> 
> Maybe a better solution to using unions is to use WTF::Variant instead.

Okay, looking at the CapabilityValueOrRange() constructors again:

- All of them set m_minOrValue (either int or double).
- Three do not set m_max.
- All of them set m_type.

Is it better to remove the default member initializers and do everything explicitly in the constructors, or let the constructors override the values and use the default member initializers as a fallback?

For example, if we leave the default member initializers, we can change this:

    CapabilityValueOrRange()
        : m_type(Undefined)
    {
    }

To this:

    CapabilityValueOrRange() = default;

Hmmm...since both m_minOrValue and m_max should be the same time, perhaps it's better to initialize everything in the constructors in this case.  I'll file a new bug for that.  And maybe look to see how much work switching to WTF::Variant is.

[David Kilzer (:ddkilzer)]

(In reply to David Kilzer (:ddkilzer) from comment #11)
> Hmmm...since both m_minOrValue and m_max should be the same time, [...]

time => type

[Darin Adler]

I’m not sure what problem we are solving. The idea of initializing in constructors instead of in the header sounds good here. The idea of using Variant instead of union can be good too, as long as it doesn’t hurt performance or anything.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #13)
> I’m not sure what problem we are solving.

I was trying to determine how to address your follow-up comments.  So there is no specific action that is required?

[Darin Adler]

(In reply to David Kilzer (:ddkilzer) from comment #14)
> I was trying to determine how to address your follow-up comments.  So there
> is no specific action that is required?

My comments assumed that you were initializing these fields because of UBSan.

If you’re just initializing them to make our code more foolproof without any connection to "undefined behavior", then what you have in the patch is OK with me. Not *great*, because it seems super likely that some day someone will re-order the union since we don’t have a comment explaining why we put double first.

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #15)
> (In reply to David Kilzer (:ddkilzer) from comment #14)
> > I was trying to determine how to address your follow-up comments.  So there
> > is no specific action that is required?
> 
> My comments assumed that you were initializing these fields because of UBSan.
> 
> If you’re just initializing them to make our code more foolproof without any
> connection to "undefined behavior", then what you have in the patch is OK
> with me. Not *great*, because it seems super likely that some day someone
> will re-order the union since we don’t have a comment explaining why we put
> double first.

Tracking follow-up fixes here:

Bug 224090: [CoreIPC] Encoding/decoding of WebCore::CapabilityValueOrRange transmits padding bytes