picture: check each plane dimension doesn't overflow

In 4:4:4 NV24 and NV42 ¹ this is how the CbCr planes are arranged. Unlike NV12, the chroma plane is twice the width of the luminance width.

Fixes #28980

Plus map NV24/NV42 to their libavutil equivalents.

1. https://www.kernel.org/doc/html/v4.10/media/uapi/v4l/pixfmt-nv24.html ↩

changed milestone to %4.0

added Component::Core Component::Decoders labels

approved this merge request

added MRStatus::Acceptable label

The comment above is there for reasons. You can't simply increase the value here, unless you reduce the maximum allowed width or height.

I don't understand what would overflow. The code is just setting the planes pitch/lines according to the picture format.

The buffer allocation is done later in picture_NewFromFormat() and it's adding the full pitch/lines to compute the size of the buffers. There are checks that the sum cannot be bigger than a size_t.

As for the rest of the code, it doesn't care about the dimensions differences between each planes. Each plane is used independently.

If there's an actual limitation with NV24/NV42 we should not allow to map it to VLC pictures at all. But I don't see it.

There's a potential integer overflow for at least i_pitch and probablyi_visible_pitch because width is checked as lower than INT_MAX.

If the numerator is smaller than the denominator, multiplying by w->num/w->den is bound to be lower than 1 which isn't true anymore with this assert.

IIUC, the point is that you should probably make sure that (width <= INT_MAX/(2 * p_dsc->pixel_size))

Assuming obviously that pixel_size can't be null.

changed this line in version 2 of the diff

I added a multiplication check on each plane so no plane dimension can ever be bigger than INT_MAX.

w->num/w->den

We don't do that, first we multiply by w->num and then the result is divided by w->den. So 2 by 1/2 ends up being 1, not 0. The operation split is even more visible now with ckd_mul().

We've been through this argument times and again. The product of the height and width cannot overflow 32-bit signed integers, and currently the limit assumes that there is no oversampling.

Or enjoy the thorough (documented) code audit to fix that limitation.

Isn't that protected by the following check?

#define PICTURE_SW_SIZE_MAX (UINT32_C(1) << 28) /* 256MB: 8K * 8K * 4*/
[...]
if (unlikely(pic_size >= PICTURE_SW_SIZE_MAX))
        goto error;

Or am I missing/misunderstanding something?

Ah picture_Setup is a public libvlccore API and should probably have a similar check than the one in picture_NewFromFormat but maybe I'm still missing something.

The current limits are 8192 pixels in each dimension, 8 bytes per pixel and 1:1 sampling. Multiplying it all together, we get up to 2 to the power 29. We refused to double the dimension to avoid getting to 31, which would be signed overflow. AFAICT, the same limit affects increasing the sampling.

added MRStatus::InReview label and removed MRStatus::Acceptable label

added 3 commits

• 0f8f77aa - picture: check each plane dimension doesn't overflow
• eb4a4a6f - picture: remove assert on plane dimension ratios
• 01251d4b - avcodec/chroma: map AV_PIX_FMT_NV24/42 to VLC_CODEC_NV24/42

Compare with previous version

changed title from picture: allow 2x oversampling of plane width to picture: check each plane dimension doesn't overflow

added MRStatus::Stale label and removed MRStatus::InReview label