[FFmpeg-devel] [PATCH] avcodec/flac_parser: Do not loose header count in find_headers_search()

Andreas Rheinhardt andreas.rheinhardt at gmail.com
Tue Feb 4 13:10:00 EET 2020


Michael Niedermayer:
> Fixes: Timeout
> Fixes: out of array access
> Fixes: 20274/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-5649631988154368
> Fixes: 19275/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_FLAC_fuzzer-5757535722405888
> 
> 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/flac_parser.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/libavcodec/flac_parser.c b/libavcodec/flac_parser.c
> index 9ffa288548..3424583c49 100644
> --- a/libavcodec/flac_parser.c
> +++ b/libavcodec/flac_parser.c
> @@ -208,16 +208,20 @@ static int find_headers_search(FLACParseContext *fpc, uint8_t *buf,
>      uint32_t x;
>  
>      for (i = 0; i < mod_offset; i++) {
> -        if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8)
> -            size = find_headers_search_validate(fpc, search_start + i);
> +        if ((AV_RB16(buf + i) & 0xFFFE) == 0xFFF8) {
> +            int ret = find_headers_search_validate(fpc, search_start + i);
> +            size = FFMAX(size, ret);
> +        }
>      }
>  
>      for (; i < buf_size - 1; i += 4) {
>          x = AV_RN32(buf + i);
>          if (((x & ~(x + 0x01010101)) & 0x80808080)) {
>              for (j = 0; j < 4; j++) {
> -                if ((AV_RB16(buf + i + j) & 0xFFFE) == 0xFFF8)
> -                    size = find_headers_search_validate(fpc, search_start + i + j);
> +                if ((AV_RB16(buf + i + j) & 0xFFFE) == 0xFFF8) {
> +                    int ret = find_headers_search_validate(fpc, search_start + i + j);
> +                    size = FFMAX(size, ret);
> +                }
>              }
>          }
>      }
> 
1. Actually, find_headers_search_validate() can return an error and
these FFMAX (as well as the FFMAX already present in
find_new_headers()) will make sure that they are overwritten.
2. The linked list of buffered headers is currently traversed pretty
often: E.g. if it seems that no new headers have been found in a call
to find_new_headers(), the linked list will be traversed again in
order to count the number of buffered headers. Maybe one should
instead directly increment fpc->nb_headers_buffered after a new header
has been added to the list in find_headers_search_validate() and only
use the return value of find_headers_search_validate(),
find_headers_search(), find_new_headers() to forward the allocation
failure errors? (If one stores a pointer to the end of the linked
list, one can also remove the while-loop in
find_headers_search_validate(); don't know if this helps with the
timeout.)
There would be two scenarios in which this might lead to different
outcomes than the current code: In the case that you are fixing right
now where an prima-facie invalid header might mask a header that seems
to be valid. And in the case where an allocation failure happens and
the failure is currently forwarded. That's because nb_headers_buffered
is currently only updated when no allocation error is reported
(obviously, because the return value is used for both allocation
failure and new number of buffered headers). I am actually unsure what
to do in this scenario. Given that a parser is not allowed to return
errors, wouldn't it make sense to score the headers that we do have
(if there are at least FLAC_MIN_HEADERS)?
3. What array does this out-of-array-access affect? I presume it is
the link_penalty-array, but I just don't see how this is possible.

- Andreas


More information about the ffmpeg-devel mailing list