diff mbox series

[FFmpeg-devel,12/19] avcodec/mpegvideo_enc: Don't segfault on unorthodox mpeg_quant

Message ID HE1PR0301MB21544D0BFBC271E203CB0ED68F769@HE1PR0301MB2154.eurprd03.prod.outlook.com
State Accepted
Commit d393c45051ddaf6146e7e29ec2ea97035a727529
Headers show
Series [FFmpeg-devel,1/9] avcodec/encode: Fix check for allowed LJPEG pixel formats
Related show

Checks

Context Check Description
andriy/x86_make success Make finished
andriy/x86_make_fate success Make fate finished
andriy/PPC64_make success Make finished
andriy/PPC64_make_fate success Make fate finished

Commit Message

Andreas Rheinhardt April 6, 2021, 9:55 p.m. UTC
The (deprecated) field AVCodecContext.mpeg_quant has no range
restriction; MpegEncContext.mpeg_quant is restricted to 0..1.
If the former is set, the latter is overwritten with it without
checking the range. This can trigger an av_assert2() with the MPEG-4
encoder when writing said field.

Fix this by just setting MpegEncContext.mpeg_quant to 1 if
AVCodecContext.mpeg_quant is set.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
---
This can not be triggered if one only sets options via the dictionary in
avcodec_open2(); one needs to set it directly in the AVCodecContext or
use the private class of AVCodecContext.

 libavcodec/mpegvideo_enc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Michael Niedermayer April 7, 2021, 9:05 p.m. UTC | #1
On Tue, Apr 06, 2021 at 11:55:09PM +0200, Andreas Rheinhardt wrote:
> The (deprecated) field AVCodecContext.mpeg_quant has no range
> restriction; MpegEncContext.mpeg_quant is restricted to 0..1.
> If the former is set, the latter is overwritten with it without
> checking the range. This can trigger an av_assert2() with the MPEG-4
> encoder when writing said field.
> 
> Fix this by just setting MpegEncContext.mpeg_quant to 1 if
> AVCodecContext.mpeg_quant is set.
> 
> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> ---
> This can not be triggered if one only sets options via the dictionary in
> avcodec_open2(); one needs to set it directly in the AVCodecContext or
> use the private class of AVCodecContext.

I tend toward checking mpeg_quant to be valid in init somewhere but this
here is fine too or in addition

thx

[...]
Andreas Rheinhardt April 7, 2021, 9:07 p.m. UTC | #2
Michael Niedermayer:
> On Tue, Apr 06, 2021 at 11:55:09PM +0200, Andreas Rheinhardt wrote:
>> The (deprecated) field AVCodecContext.mpeg_quant has no range
>> restriction; MpegEncContext.mpeg_quant is restricted to 0..1.
>> If the former is set, the latter is overwritten with it without
>> checking the range. This can trigger an av_assert2() with the MPEG-4
>> encoder when writing said field.
>>
>> Fix this by just setting MpegEncContext.mpeg_quant to 1 if
>> AVCodecContext.mpeg_quant is set.
>>
>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>> ---
>> This can not be triggered if one only sets options via the dictionary in
>> avcodec_open2(); one needs to set it directly in the AVCodecContext or
>> use the private class of AVCodecContext.
> 
> I tend toward checking mpeg_quant to be valid in init somewhere but this
> here is fine too or in addition
> 
Do you want to error out if AVCodecContext.mpeg_quant is outside of
0..1? Or do you want another check for MpegEncContext.mpeg_quant? The
option for the latter already uses the proper range, so I don't think we
need a check for that.

- Andreas
Michael Niedermayer April 7, 2021, 10:14 p.m. UTC | #3
On Wed, Apr 07, 2021 at 11:07:54PM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Tue, Apr 06, 2021 at 11:55:09PM +0200, Andreas Rheinhardt wrote:
> >> The (deprecated) field AVCodecContext.mpeg_quant has no range
> >> restriction; MpegEncContext.mpeg_quant is restricted to 0..1.
> >> If the former is set, the latter is overwritten with it without
> >> checking the range. This can trigger an av_assert2() with the MPEG-4
> >> encoder when writing said field.
> >>
> >> Fix this by just setting MpegEncContext.mpeg_quant to 1 if
> >> AVCodecContext.mpeg_quant is set.
> >>
> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >> ---
> >> This can not be triggered if one only sets options via the dictionary in
> >> avcodec_open2(); one needs to set it directly in the AVCodecContext or
> >> use the private class of AVCodecContext.
> > 
> > I tend toward checking mpeg_quant to be valid in init somewhere but this
> > here is fine too or in addition
> > 
> Do you want to error out if AVCodecContext.mpeg_quant is outside of
> 0..1? 

that was the idea, i did not test this though 


> Or do you want another check for MpegEncContext.mpeg_quant? The
> option for the latter already uses the proper range, so I don't think we
> need a check for that.
> 
> - 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".
Andreas Rheinhardt April 7, 2021, 10:17 p.m. UTC | #4
Michael Niedermayer:
> On Wed, Apr 07, 2021 at 11:07:54PM +0200, Andreas Rheinhardt wrote:
>> Michael Niedermayer:
>>> On Tue, Apr 06, 2021 at 11:55:09PM +0200, Andreas Rheinhardt wrote:
>>>> The (deprecated) field AVCodecContext.mpeg_quant has no range
>>>> restriction; MpegEncContext.mpeg_quant is restricted to 0..1.
>>>> If the former is set, the latter is overwritten with it without
>>>> checking the range. This can trigger an av_assert2() with the MPEG-4
>>>> encoder when writing said field.
>>>>
>>>> Fix this by just setting MpegEncContext.mpeg_quant to 1 if
>>>> AVCodecContext.mpeg_quant is set.
>>>>
>>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
>>>> ---
>>>> This can not be triggered if one only sets options via the dictionary in
>>>> avcodec_open2(); one needs to set it directly in the AVCodecContext or
>>>> use the private class of AVCodecContext.
>>>
>>> I tend toward checking mpeg_quant to be valid in init somewhere but this
>>> here is fine too or in addition
>>>
>> Do you want to error out if AVCodecContext.mpeg_quant is outside of
>> 0..1? 
> 
> that was the idea, i did not test this though 
> 

I think it's not worth it for an option that will be removed on the next
bump (AVCodecContext.mpeg_quant is deprecated).

> 
>> Or do you want another check for MpegEncContext.mpeg_quant? The
>> option for the latter already uses the proper range, so I don't think we
>> need a check for that.
>>
>> - Andreas
Michael Niedermayer April 7, 2021, 10:25 p.m. UTC | #5
On Thu, Apr 08, 2021 at 12:17:04AM +0200, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Wed, Apr 07, 2021 at 11:07:54PM +0200, Andreas Rheinhardt wrote:
> >> Michael Niedermayer:
> >>> On Tue, Apr 06, 2021 at 11:55:09PM +0200, Andreas Rheinhardt wrote:
> >>>> The (deprecated) field AVCodecContext.mpeg_quant has no range
> >>>> restriction; MpegEncContext.mpeg_quant is restricted to 0..1.
> >>>> If the former is set, the latter is overwritten with it without
> >>>> checking the range. This can trigger an av_assert2() with the MPEG-4
> >>>> encoder when writing said field.
> >>>>
> >>>> Fix this by just setting MpegEncContext.mpeg_quant to 1 if
> >>>> AVCodecContext.mpeg_quant is set.
> >>>>
> >>>> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@outlook.com>
> >>>> ---
> >>>> This can not be triggered if one only sets options via the dictionary in
> >>>> avcodec_open2(); one needs to set it directly in the AVCodecContext or
> >>>> use the private class of AVCodecContext.
> >>>
> >>> I tend toward checking mpeg_quant to be valid in init somewhere but this
> >>> here is fine too or in addition
> >>>
> >> Do you want to error out if AVCodecContext.mpeg_quant is outside of
> >> 0..1? 
> > 
> > that was the idea, i did not test this though 
> > 
> 
> I think it's not worth it for an option that will be removed on the next
> bump (AVCodecContext.mpeg_quant is deprecated).

ok then forget about the idea


[...]
diff mbox series

Patch

diff --git a/libavcodec/mpegvideo_enc.c b/libavcodec/mpegvideo_enc.c
index d1f2460409..0a1d0db86d 100644
--- a/libavcodec/mpegvideo_enc.c
+++ b/libavcodec/mpegvideo_enc.c
@@ -564,7 +564,7 @@  FF_ENABLE_DEPRECATION_WARNINGS
 #if FF_API_PRIVATE_OPT
     FF_DISABLE_DEPRECATION_WARNINGS
     if (avctx->mpeg_quant)
-        s->mpeg_quant = avctx->mpeg_quant;
+        s->mpeg_quant = 1;
     FF_ENABLE_DEPRECATION_WARNINGS
 #endif