[FFmpeg-devel,1/2] avcodec/g2meet: Check if adjusted pixel was on the stack

Submitted by Michael Niedermayer on Sept. 9, 2019, 8:16 p.m.

Details

Message ID 20190909201621.14091-1-michael@niedermayer.cc
State Accepted
Commit 9c84c162e9f9f000ef47d4fcd07354805f38d455
Headers show

Commit Message

Michael Niedermayer Sept. 9, 2019, 8:16 p.m.
This basically checks if a pixel that was coded with prediction
and residual could have been stored using a previous case.
This avoids basically a string of 0 symbols stored in less than
50 bytes to hit a O(n²) codepath.

Fixes: Timeout (too slow to wait -> immedeatly)
Fixes: 8668/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_G2M_fuzzer-4895946310680576

Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg
Signed-off-by: Michael Niedermayer <michael@niedermayer.cc>
---
 libavcodec/g2meet.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Tomas Härdin Sept. 9, 2019, 9:03 p.m.
mån 2019-09-09 klockan 22:16 +0200 skrev Michael Niedermayer:
> This basically checks if a pixel that was coded with prediction
> and residual could have been stored using a previous case.
> This avoids basically a string of 0 symbols stored in less than
> 50 bytes to hit a O(n²) codepath.
> 
> Fixes: Timeout (too slow to wait -> immedeatly)
> Fixes: 8668/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_G2M_fuzzer-4895946310680576

go2unpleasantplaces indeed

Something tells me there are more ways than this to hit that codepath,
and I've made my feelings about hacks like this known already.

/Tomas
Michael Niedermayer Sept. 10, 2019, 2:06 p.m.
On Mon, Sep 09, 2019 at 11:03:48PM +0200, Tomas Härdin wrote:
> mån 2019-09-09 klockan 22:16 +0200 skrev Michael Niedermayer:
> > This basically checks if a pixel that was coded with prediction
> > and residual could have been stored using a previous case.
> > This avoids basically a string of 0 symbols stored in less than
> > 50 bytes to hit a O(n²) codepath.
> > 
> > Fixes: Timeout (too slow to wait -> immedeatly)
> > Fixes: 8668/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_G2M_fuzzer-4895946310680576
> 
> go2unpleasantplaces indeed
> 

> Something tells me there are more ways than this to hit that codepath,

yes, certainly, the question is how much input bytes that needs.
Because if it needs alot this is still a significant improvment.
Now why could it need more bytes?
The cache loops over all values in an entry and if the same value
is in it multiple times that is rather slow and causes no symbol
to be read. But if there are different values in an entry each
will cause a symbol to be read moving the bitstream forward a tiny
bit.
We eliminate/reduce (or at least thats the idea) to have duplicate values
in the cache. So that should change the input symbols from n to n*n
still how much that is in bytes would require further analysis,

But do you have a better idea than this patch ?
If not i would suggest to apply it and see what the fuzzer finds
with this.
Or we could add a counter and a threshold to the cache and
if its used more than constant*width*height then error out

The problem with g2m is its cache and stack design is integrated
into the bitstream. So fixing this in an obvious way would
change the bitstream, which of course doesnt work...

thx

> and I've made my feelings about hacks like this known already.
> 
> /Tomas
> 
> _______________________________________________
> 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".

Patch hide | download patch | download mbox

diff --git a/libavcodec/g2meet.c b/libavcodec/g2meet.c
index a1dec8d823..19e1c130ce 100644
--- a/libavcodec/g2meet.c
+++ b/libavcodec/g2meet.c
@@ -854,6 +854,9 @@  static int epic_decode_tile(ePICContext *dc, uint8_t *out, int tile_height,
                     uint32_t ref_pix = curr_row[x - 1];
                     if (!x || !epic_decode_from_cache(dc, ref_pix, &pix)) {
                         pix = epic_decode_pixel_pred(dc, x, y, curr_row, above_row);
+                        if (is_pixel_on_stack(dc, pix))
+                            return AVERROR_INVALIDDATA;
+
                         if (x) {
                             int ret = epic_add_pixel_to_cache(&dc->hash,
                                                               ref_pix,