[FFmpeg-devel] [PATCH 4/5] avcodec/alac: Check for bps of 0

[Michael Niedermayer]

On Sun, Aug 25, 2019 at 12:06:30PM -0300, James Almer wrote:
> On 8/25/2019 4:32 AM, Michael Niedermayer wrote:
> > On Fri, Aug 23, 2019 at 11:20:48AM -0300, James Almer wrote:
> >> On 8/8/2019 8:23 PM, Michael Niedermayer wrote:
> >>> Fixes: shift exponent 32 is too large for 32-bit type 'unsigned int'
> >>> Fixes: 15764/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_ALAC_fuzzer-5102101203517440
> >>>
> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
> >>> Signed-off-by: Michael Niedermayer <michael at niedermayer.cc>
> >>> ---
> >>>  libavcodec/alac.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/libavcodec/alac.c b/libavcodec/alac.c
> >>> index 6086e2caa8..1196925aa7 100644
> >>> --- a/libavcodec/alac.c
> >>> +++ b/libavcodec/alac.c
> >>> @@ -250,7 +250,7 @@ static int decode_element(AVCodecContext *avctx, AVFrame *frame, int ch_index,
> >>>  
> >>>      alac->extra_bits = get_bits(&alac->gb, 2) << 3;
> >>>      bps = alac->sample_size - alac->extra_bits + channels - 1;
> >>> -    if (bps > 32U) {
> >>> +    if (bps > 32 || bps < 1) {
> >>>          avpriv_report_missing_feature(avctx, "bps %d", bps);
> >>>          return AVERROR_PATCHWELCOME;
> >>
> >> bps 0 (or negative) is obviously a broken file, 
> > 
> > id say very likely a broken file, yes
> > 
> > 
> >> so asking for a sample
> >> is pointless. Just return invalid data in those cases, and leave this
> >> check for > 32.
> > 
> > thats a few lines more code, for an error code and different/no message
> > its a bit difficult to guess where people prefer the extra code to be
> > correct and where they prefer somewhat incorrect solutions to minimize 
> > fuzzer found bugfixes.
> 
> I don't know who this was aimed to, but afaik i don't ask or don't
> intend to ask for "incorrect solutions" for fuzzer found issues.

no, of course not, but in some reviews people complained along the
lines of there being too much fuzzer found fixes/checks.



> I simply stated that asking for a sample in a situation where it's known
> that the sample is simply a broken/corrupt one makes no sense and can
> end up wasting a user's time for bothering to submit it. The extra lines
> to properly abort in such cases are justified.

yes, your request was fine, sorry if it appeared that i meant something
else

Thanks

[...]

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

You can kill me, but you cannot change the truth.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20190826/5300bf80/attachment.sig>