Closed
Bug 1207762
Opened 9 years ago
Closed 9 years ago
UBSan: left shift of negative value rec_mb.cpp:246:14
Categories
(Core :: Audio/Video: GMP, defect)
Core
Audio/Video: GMP
Tracking
()
RESOLVED
FIXED
People
(Reporter: tsmith, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: sec-audit, testcase)
Attachments
(2 files)
This was found with fuzzing+UBSan build.
Output from UBSan:
codec/decoder/core/src/rec_mb.cpp:246:14: runtime error: left shift of negative value -30
Which is referring to:
iFullMVx = WELS_CLIP3 (iFullMVx, ((-PADDING_LENGTH + 2) << 2), ((pMCRefMem->iPicWidth + PADDING_LENGTH - 19) << 2));
Reporter | ||
Updated•9 years ago
|
Blocks: WebRTC-OpenH264, fuzzing-openh264
Reporter | ||
Comment 1•9 years ago
|
||
Hi Tyson, we actually know this but we don't think it is a problem we need to fixed, this left shift won't make a problem since the data range of this input is limited.
Reporter | ||
Comment 3•9 years ago
|
||
(In reply to sijchen from comment #2)
> Hi Tyson, we actually know this but we don't think it is a problem we need
> to fixed, this left shift won't make a problem since the data range of this
> input is limited.
This is a runtime error reported by UBSan and the attached test case does actually trigger this issue. I am not exactly sure what the implications are that depends on how data resulting from this operation is used. I will attach a call stack to help debug this issue.
Flags: needinfo?(twsmith)
Reporter | ||
Comment 4•9 years ago
|
||
I think it is a common false alarm. The purpose of this implementation is to get the 4 times value of the motion vector or the padding offset. Here, the motion vector is at the range of -512 to 512 and the min value of the offset is -32. So the calculated results will not exceed the expected value. I think we can ignore this type of alarms.
Flags: needinfo?(haibozhu)
Reporter | ||
Comment 6•9 years ago
|
||
Well it's not a false alarm, things just seem to be working at the moment. The problem is that we are treating a '<< 2' as if it were a '* 4'. The problem isn't the value range, yes it should be a concern, but in this case what is happening in undefined regardless. That means we are at the mercy of the compiler. Different things could happen with different compilers, different build types: x86, arm, 32 or 64-bit, optimized, debug, non-debug, combination of the build types or even different compiler version.
So TLDR; a left shift of a negative number by 2 is NOT equal to multiplying it by 4 although it may act that way some or most of the time (even when working within seemingly unaffected value ranges).
Although I think it is not a bug, I have added a fix to remove it at commit af6a9a8 at master branch and commit 9c88070 at openh264v1.5 branch. Please help to verify it.
Reporter | ||
Comment 8•9 years ago
|
||
Thanks Haibo. Verified with commit: fb61733b2.
Talked to Sijia. Close it here since it is fixed and verified.
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Updated•9 years ago
|
Group: media-core-security → core-security-release
Comment 10•9 years ago
|
||
Should be released with v1.5.
Updated•8 years ago
|
Group: core-security-release
Assignee | ||
Updated•2 years ago
|
Component: OpenH264 → Audio/Video: GMP
Product: External Software Affecting Firefox → Core
You need to log in
before you can comment on or make changes to this bug.
Description
•