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

[Paul B Mahol]

On 11/27/19, James Almer <jamrial at gmail.com> wrote:
> On 11/27/2019 3:42 PM, Paul B Mahol wrote:
>> On 11/27/19, Tomas Härdin <tjoppen at acc.umu.se> wrote:
>>> tis 2019-11-26 klockan 20:29 +0100 skrev 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
>>>>>>>
>>>>>>
>>>>>> Nothing of this can actually happen.
>>>
>>> This assumes max_pixels will never be increased above INT_MAX. "64k"
>>> video is most definitely within the range of possibility in the coming
>>> years, if it isn't already. Film archival and DPX come to mind.
>>>
>>>>> 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.
>>>
>>> This reads like an admission of pushing insecure code via "not my
>>> problem"
>>>
>>>>> 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.
>>>
>>> zmbv does
>>>
>>> All this is really about the lack of any way to reason about
>>> assumptions like "dimensions can't be larger than X*Y" at compile time,
>>> which is a thing I've been pointing out on this list for a while now.
>>>
>>
>> Nobody tells you not to fix it yourself.
>
> Just add a the suggested overflow checks, Christ. It's a one line
> addition each. What do you gain arguing like this?

Than next day he or you will come with another great idea.

> _______________________________________________
> 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".