oss-fuzz icon indicating copy to clipboard operation
oss-fuzz copied to clipboard

Initial libyuv integration

Open ToSeven opened this issue 1 year ago • 7 comments

ToSeven avatar Jan 03 '24 07:01 ToSeven

ToSeven has previously contributed to projects/libyuv. The previous PR was #4282

github-actions[bot] avatar Jan 03 '24 07:01 github-actions[bot]

This looks to have stalled previously https://github.com/google/oss-fuzz/pull/4363 ? Could you provide context?

DavidKorczynski avatar Jan 03 '24 10:01 DavidKorczynski

This looks to have stalled previously #4363 ? Could you provide context?

Yeah, the previous PR only fuzzed an API of LIbyuv, and the quality of the fuzz drive was not good. Additionally, a maintainer declared that he was not familiar with fuzzing and was not a suitable person to review my code.

The current commit well fuzzes the Libyuv. In the process of my local fuzzing, I found my fuzzer had detected a bug.
https://chromium.googlesource.com/libyuv/libyuv/+/refs/heads/main/source/convert.cc Function I444ToI420 and function I422ToI420 do not check the parameters src_y src_u src_v, which cause the undefined behavior.

ToSeven avatar Jan 04 '24 08:01 ToSeven

@fbarchard Hi, I would appreciate it if you reviewed my code when you were free. My local fuzz testing based on this commit has found a bug, which gets your confirmation (https://bugs.chromium.org/p/libyuv/issues/detail?id=970#c5). The time it took to find this bug was only 1 minute. It can prove my code is good.

ToSeven avatar Feb 09 '24 14:02 ToSeven

libyuv issue #970 is resolved? If you're hitting a new bug in libyuv, please report with more details

fbarchard avatar Feb 10 '24 05:02 fbarchard

libyuv issue #970 is resolved? If you're hitting a new bug in libyuv, please report with more details

Yeah, you have resolved it. I expect you could review my code and further approve this commit.

ToSeven avatar Feb 10 '24 06:02 ToSeven

As there are no code reviews / bugs for libyuv, I think you're asking me to review your fuzz code? I'm not really qualified to review it, but it seems to assume the formats are all subsampled the same amount (half size), which they are not. If you look at the unittests in libyuv, there are macros for each type of image, where the main factors are number of planes and subsampling. NV12 etc are biplanar - 2 pointers I420 etc are 3 planes - 3 pointers I400 etc is 1 plane - grey There are 444, 422 and 420 subsampling. Channels can be 16 bit (for 10 bit) or 8 bit. There are also 'packed' formats like YUY2 which is 4:2:2 subsampled and a single plane.

For a given format (e.g. I420) and a width and height, you can compute how many bytes it requires. I think if you compute that, you'll find it doesnt match the 'size' the fuzz is passed. Its not a trivial function to compute a valid width/height from 'size' and it wont match exactly. ignoring planes, if you were given 10 bytes, 3x3 would use 9 bytes and waste 1. 5x2 would match exactly. with 4:2:0 subsampling, a 3x3 needs 2x2 UV planes.
with 4:2:2 subsampling, a 3x3 needs 2x3 UV planes.
with 4:4:4 subsampling, a 3x3 needs 3x3 UV planes. Expressed as either 1 biplanar UV plane for nv12, or 2 planes for planar formats.

given a pixel format and number of bytes, its not trivial to come up with valid width and height... it would be a best match or can fail...e.g. YUV with 3 planes requires at least 3 bytes. 2 bytes would either fail, or use 0 for the dimensions.

It looks like your fuzz expects a format to take bits per channel, but the formats have a fixed bits per channel. e.g. I010 is 10 bits. in the lsb of 16 bits. It can't be changed as a parameter... its always 10 bits.

fbarchard avatar Feb 10 '24 09:02 fbarchard