Undefined behavior in GeckoMediaPluginServiceParent::RemoveOnGMPThread()

RESOLVED FIXED in Firefox 52

Status

()

P2
normal
Rank:
25
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: q1, Assigned: JamesCheng)

Tracking

48 Branch
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
GeckoMediaPluginServiceParent::RemoveOnGMPThread() (dom\media\gmp\GMPServiceParent.cpp) invokes undefined behavior in a |for| loop. The code depends upon the identity

   static_cast <size_t> (0) - 1 == std::numeric_limits<size_t>::max()

C++ does not make this guarantee.

The bug is in line 1151:

1131: void
1132: GeckoMediaPluginServiceParent::RemoveOnGMPThread(const nsAString& aDirectory,
1133:                                                  const bool aDeleteFromDisk,
1134:                                                  const bool aCanDefer)
1135: {
...
1151:   for (size_t i = mPlugins.Length() - 1; i < mPlugins.Length(); i--) {
(Reporter)

Comment 1

2 years ago
(the bug is that, when line 1151 executes with |i| == 0, |i--| is executed, making |i| underflow. The expression |i < mPlugins.Length()| then depends, for proper operation, upon |i| underflowing to something >= |mPlugins.Length()|, realistically |std::numeric_limits<size_t>::max()|). But C++11 s.5(4) says:

------
If during the evaluation of an expression, the result is...not in the range of representable values for its type, the behavior is undefined.
------

Since -1 is not representable as a |size_t|, the expression |i--| invokes undefined behavior when |i| == 0.
Thanks for the report. Moving to what I think is a better component.
Component: DOM → Audio/Video: GMP
Rank: 25
Priority: -- → P2
(Assignee)

Updated

2 years ago
Assignee: nobody → jacheng
Comment hidden (mozreview-request)

Comment 5

2 years ago
mozreview-review
Comment on attachment 8793990 [details]
Bug 1302881 - Undefined behavior in GeckoMediaPluginServiceParent::RemoveOnGMPThread().

https://reviewboard.mozilla.org/r/80564/#review79312
Attachment #8793990 - Flags: review?(cpearce) → review+
(Assignee)

Comment 6

2 years ago
Thank you for reviewing.
Keywords: checkin-needed

Comment 7

2 years ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f56a2d286cf7
Undefined behavior in GeckoMediaPluginServiceParent::RemoveOnGMPThread(). r=cpearce
Keywords: checkin-needed

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f56a2d286cf7
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Reporter)

Comment 9

2 years ago
> for (size_t i = mPlugins.Length(); i-- > 0; ) {

I think this code technically also invokes undefined behavior when |i == 0|, since the expression |i--| must be fully evaluated before the comparison |> 0| is done, though the result of evaluating the expression is |i| (which never underflows) and not the underflowed value |i-1| (which isn't used).
(Assignee)

Comment 10

2 years ago
Basically, I think it is a better solution since the original code[1] is used the undefined value for comparison. The current solution did not use the value and there is no chance to cause infinite for loop.

The nsTArray seems not support API like std::remove_if and erase, so reverse for loop is inevitable.

[1]
for (size_t i = mPlugins.Length() - 1; i < mPlugins.Length(); i--)
(Reporter)

Comment 11

2 years ago
This is into the language-lawyer weeds, but I view the phrase from C++11 s.5(4) that says:

------
If during the evaluation of an expression, the result is not mathematically defined or not in the range of representable values for its type, the behavior is undefined....
------

as permitting the program to do anything at all. This view is (somewhat ambiguously) supported by s.1.3.24, which says:

------
[defns.undefined]
undefined behavior
behavior for which this International Standard imposes no requirements
[ Note: Undefined behavior may be expected when this International Standard omits any explicit definition of behavior or when a program uses an erroneous construct or erroneous data. Permissible undefined behavior ranges from ignoring the situation completely with unpredictable results, to behaving during translation or program execution in a documented manner characteristic of the environment (with or without the issuance of a diagnostic message), to terminating a translation or execution (with the issuance of a diagnostic message)...
------

So, I try to avoid invoking any kind of undefined behavior. But that's just me. I do agree that on real platforms it's worse to use a value that is undefined than simply to create one.
Just got wind of this bug.

I beg to differ:

http://en.cppreference.com/w/cpp/language/operator_arithmetic#Overflows
"Unsigned integer arithmetic is always performed modulo 2^n where n is the number of bits in that particular integer. E.g. for unsigned int, adding one to UINT_MAX gives ​0​, and subtracting one from ​0​ gives UINT_MAX."

Or 3.9.1 in the C++11 std:
"4 Unsigned integers, declared unsigned, shall obey the laws of arithmetic modulo 2^n where n is the number of bits in the value representation of that particular size of integer.46
46) This implies that unsigned arithmetic does not overflow because a result that cannot be represented by the resulting
unsigned integer type is reduced modulo the number that is one greater than the largest value that can be represented by the resulting unsigned integer type."

(So the patch was not necessary, but it doesn't hurt.)

Signed overflows are UB.
You need to log in before you can comment on or make changes to this bug.