[FFmpeg-devel] [PATCH 1/2] avcodec: add mvdv video decoder

[Paul B Mahol]

On 11/26/19, James Almer <jamrial at gmail.com> wrote:
> On 11/26/2019 6:47 AM, Paul B Mahol wrote:
>> On 11/25/19, Tomas Härdin <tjoppen at acc.umu.se> wrote:
>>> mån 2019-11-25 klockan 22:09 +0100 skrev Paul B Mahol:
>>>> Signed-off-by: Paul B Mahol <onemda at gmail.com>
>>>> +static int decode_mvdv(MidiVidContext *s, AVCodecContext *avctx,
>>>> AVFrame
>>>> *frame)
>>>> +{
>>>> +    GetByteContext *gb = &s->gb;
>>>> +    GetBitContext mask;
>>>> +    GetByteContext idx9;
>>>> +    uint16_t nb_vectors, intra_flag;
>>>> +    const uint8_t *vec;
>>>> +    const uint8_t *mask_start;
>>>> +    uint8_t *skip;
>>>> +    int mask_size;
>>>> +    int idx9bits = 0;
>>>> +    int idx9val = 0;
>>>> +    int num_blocks;
>>>> +
>>>> +    nb_vectors = bytestream2_get_le16(gb);
>>>> +    intra_flag = bytestream2_get_le16(gb);
>>>> +    if (intra_flag) {
>>>> +        num_blocks = (avctx->width / 2) * (avctx->height / 2);
>>>
>>> Will UB if width*height/4 > INT_MAX
>>>
>>>> +    } else {
>>>> +        int skip_linesize;
>>>> +
>>>> +        num_blocks = bytestream2_get_le32(gb);
>>>
>>> Might want to use uint32_t so this doesn't lead to weirdness on 32-bit
>>>
>>>> +        skip_linesize = avctx->width >> 1;
>>>> +        mask_start = gb->buffer_start + bytestream2_tell(gb);
>>>> +        mask_size = (avctx->width >> 5) * (avctx->height >> 2);
>>>
>>> This can also UB
>>>
>>> /Tomas
>>>
>>> _______________________________________________
>>> ffmpeg-devel mailing list
>>> ffmpeg-devel at ffmpeg.org
>>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>>
>>> To unsubscribe, visit link above, or email
>>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>
>> Nothing of this can actually happen.
>
> It can and i'm fairly sure it will happen as soon as the fuzzer starts
> testing this decoder using big dimensions.

I'm not that guy doing such work. Please stop bikesheding those
patches for once.

>
> You don't need asserts here, you just need to check the calculations
> will not overflow. Do something like "if ((int64_t)avctx->width *
> avctx->height / 4 > INT_MAX) return AVERROR_INVALIDDATA" and call it a day.
> Also, maybe num_blocks should be unsigned, seeing you set it using
> bytestream2_get_le32() for P-frames.

No decoder does this.

>
>>
>> Applied.
>> _______________________________________________
>> ffmpeg-devel mailing list
>> ffmpeg-devel at ffmpeg.org
>> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>>
>> To unsubscribe, visit link above, or email
>> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".
>>
>
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel at ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
>
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request at ffmpeg.org with subject "unsubscribe".