1207767 - UBSan: left shift of negative value in deblocking_common.cpp:201:17

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.264

This was found with fuzzing+UBSan build.

Output from UBSan:
codec/common/src/deblocking_common.cpp:201:17: runtime error: left shift of negative value -5

Which is referring to:
Deta = WELS_CLIP3 ((((q0 - p0) << 2) + (p1 - q1) + 4) >> 3, -iTc0, iTc0);

Comment 1

[sijchen]

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.

Comment 2

[Tyson Smith [:tsmith]]

(In reply to sijchen from comment #1)
> 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.

I'm not clear if you mean that this issue cannot be triggered or that it can be but that is part of normal operation. Left-shifts of negative values are undefined in the C spec.

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.

Comment 3

[Tyson Smith [:tsmith]]

Attachment: call_stack.txt

Comment 4

[Haibo Zhu]

As same as #1207762
I think it is a common false alarm. The purpose of this implementation is to get the 4 times value of the difference between the values of the two pixels. The range of the value will be -510 to 510. So the calculated results will not exceed the expected value. I think we can ignore this type of alarms.

Comment 5

[Tyson Smith [:tsmith]]

Again same as bug 1207762.

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).

Comment 6

[sijchen]

after getting more details I tend to think this should be fixed, but the final decision is upon @Haibo, however we may not catch the release 1.5 timeline.

Comment 7

[Haibo Zhu]

As same as in #1207762. 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.

Comment 8

[Tyson Smith [:tsmith]]

Verified with commit: fb61733b2.

Comment 9

[Hank Peng]

Talked to Sijia. Close it here since it is fixed and verified.

Comment 10

[Hank Peng]

Should be released with v1.5.