Re: [FFmpeg-devel] [PATCH 7/8] avcodec/svq3: Check that for 8 byte space before subtracting

[Andreas Rheinhardt <andreas.rheinhardt@outlook.com>]

Michael Niedermayer:
> On Wed, May 14, 2025 at 06:34:25PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> No testcase
>>>
>>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
>>> ---
>>>  libavcodec/svq3.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/libavcodec/svq3.c b/libavcodec/svq3.c
>>> index f730358e2f9..30bc9334af7 100644
>>> --- a/libavcodec/svq3.c
>>> +++ b/libavcodec/svq3.c
>>> @@ -1173,7 +1173,7 @@ static av_cold int svq3_decode_init(AVCodecContext *avctx)
>>>          int w,h;
>>>  
>>>          size = AV_RB32(&extradata[4]);
>>> -        if (size > extradata_end - extradata - 8)
>>> +        if (extradata_end - extradata < 8 || size > extradata_end - extradata - 8)
>>>              return AVERROR_INVALIDDATA;
>>>          init_get_bits(&gb, extradata + 8, size * 8);
>>>  
>>
>> Can't be triggered: This code is only executed iff marker_found is 1;
>> and given the "m + 8 < avctx->extradata_size" check in the loop it is
>> guaranteed that there are at least eight bytes of extradata available.
> 
> True
> 
> Did we ever had someone miss such distributed checks and
> produce buggy code through a change ?
> If not then i think you are correct here and lets skip adding an
> explicit check, its ugly to have such redundant checks
> 

We could avoid the whole marker_found branch (and the variable) by
moving the whole if (marker_found) block into a function of its own that
is called where currently marker_found is set to one. I'll send a patch
for this.

- Andreas

_______________________________________________
ffmpeg-devel mailing list
ffmpeg-devel@ffmpeg.org
https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

To unsubscribe, visit link above, or email
ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".