| Message ID | 20190909201621.14091-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 9c84c162e9f9f000ef47d4fcd07354805f38d455 |
| Headers | show |
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
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".
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,
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(+)