| Message ID | 20181031110115.31474-1-michael@niedermayer.cc |
|---|---|
| State | Accepted |
| Commit | 9acdf17b2c30c44e6e6a3d3b3c22989b7e1117c3 |
| Headers | show |
On Wed, Oct 31, 2018 at 12:03 PM Michael Niedermayer <michael@niedermayer.cc> wrote: > > Fixes: Infinite loop > Fixes: 10685/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PROSUMER_fuzzer-5652236881887232 > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/prosumer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/libavcodec/prosumer.c b/libavcodec/prosumer.c > index 6e98677b55..2fd9880ee1 100644 > --- a/libavcodec/prosumer.c > +++ b/libavcodec/prosumer.c > @@ -57,7 +57,7 @@ static int decompress(GetByteContext *gb, int size, PutByteContext *pb, const ui > b = lut[2 * idx]; > > while (1) { > - if (bytestream2_get_bytes_left_p(pb) <= 0) > + if (bytestream2_get_bytes_left_p(pb) <= 0 || bytestream2_get_eof(pb)) > return 0; > if (((b & 0xFF00u) != 0x8000u) || (b & 0xFFu)) { > if ((b & 0xFF00u) != 0x8000u) { Why does bytestream2_get_bytes_left_p not return <= 0 at eof, where there certainly arent any bytes left? - Hendrik
On 10/31/18, Michael Niedermayer <michael@niedermayer.cc> wrote: > Fixes: Infinite loop > Fixes: > 10685/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PROSUMER_fuzzer-5652236881887232 > > Found-by: continuous fuzzing process > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/prosumer.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > patchset looks ok.
On Wed, Oct 31, 2018 at 12:06:28PM +0100, Hendrik Leppkes wrote: > On Wed, Oct 31, 2018 at 12:03 PM Michael Niedermayer > <michael@niedermayer.cc> wrote: > > > > Fixes: Infinite loop > > Fixes: 10685/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PROSUMER_fuzzer-5652236881887232 > > > > Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/prosumer.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/libavcodec/prosumer.c b/libavcodec/prosumer.c > > index 6e98677b55..2fd9880ee1 100644 > > --- a/libavcodec/prosumer.c > > +++ b/libavcodec/prosumer.c > > @@ -57,7 +57,7 @@ static int decompress(GetByteContext *gb, int size, PutByteContext *pb, const ui > > b = lut[2 * idx]; > > > > while (1) { > > - if (bytestream2_get_bytes_left_p(pb) <= 0) > > + if (bytestream2_get_bytes_left_p(pb) <= 0 || bytestream2_get_eof(pb)) > > return 0; > > if (((b & 0xFF00u) != 0x8000u) || (b & 0xFFu)) { > > if ((b & 0xFF00u) != 0x8000u) { > > Why does bytestream2_get_bytes_left_p not return <= 0 at eof, where > there certainly arent any bytes left? there are 2 bytes left because a 4 byte write failed to fit in 2 bytes maybe bytestream2_get_bytes_left_p() should be made to return 0 once eof is set even if space remains [...]
On 10/31/2018 9:18 AM, Michael Niedermayer wrote: > On Wed, Oct 31, 2018 at 12:06:28PM +0100, Hendrik Leppkes wrote: >> On Wed, Oct 31, 2018 at 12:03 PM Michael Niedermayer >> <michael@niedermayer.cc> wrote: >>> >>> Fixes: Infinite loop >>> Fixes: 10685/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PROSUMER_fuzzer-5652236881887232 >>> >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >>> --- >>> libavcodec/prosumer.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/libavcodec/prosumer.c b/libavcodec/prosumer.c >>> index 6e98677b55..2fd9880ee1 100644 >>> --- a/libavcodec/prosumer.c >>> +++ b/libavcodec/prosumer.c >>> @@ -57,7 +57,7 @@ static int decompress(GetByteContext *gb, int size, PutByteContext *pb, const ui >>> b = lut[2 * idx]; >>> >>> while (1) { >>> - if (bytestream2_get_bytes_left_p(pb) <= 0) >>> + if (bytestream2_get_bytes_left_p(pb) <= 0 || bytestream2_get_eof(pb)) >>> return 0; >>> if (((b & 0xFF00u) != 0x8000u) || (b & 0xFFu)) { >>> if ((b & 0xFF00u) != 0x8000u) { >> >> Why does bytestream2_get_bytes_left_p not return <= 0 at eof, where >> there certainly arent any bytes left? > > there are 2 bytes left because a 4 byte write failed to fit in 2 bytes > > maybe bytestream2_get_bytes_left_p() should be made to return 0 once eof is > set even if space remains Unrelated to this, but seeing how all the main GetByteContext functions start with bytestream2_get and all the PutByteContext functions with bytestream2_put, the fact these two functions from the latter start with bytestream2_get is not ideal. You have bytestream2_get_bytes_left for GetByteContext and bytestream2_get_bytes_left_p with the suffix p hinting it's for PutByteContext, but then you have bytestream2_get_eof for PutByteContext without a suffix and nothing for GetByteContext. It's confusing and not exactly intuitive. In GetBitContext and PutBitContext they are properly called get_bits_left() and put_bits_left() respectively, for example.
On Wed, Oct 31, 2018 at 09:59:04AM -0300, James Almer wrote: > On 10/31/2018 9:18 AM, Michael Niedermayer wrote: > > On Wed, Oct 31, 2018 at 12:06:28PM +0100, Hendrik Leppkes wrote: > >> On Wed, Oct 31, 2018 at 12:03 PM Michael Niedermayer > >> <michael@niedermayer.cc> wrote: > >>> > >>> Fixes: Infinite loop > >>> Fixes: 10685/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PROSUMER_fuzzer-5652236881887232 > >>> > >>> Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > >>> Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >>> --- > >>> libavcodec/prosumer.c | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/libavcodec/prosumer.c b/libavcodec/prosumer.c > >>> index 6e98677b55..2fd9880ee1 100644 > >>> --- a/libavcodec/prosumer.c > >>> +++ b/libavcodec/prosumer.c > >>> @@ -57,7 +57,7 @@ static int decompress(GetByteContext *gb, int size, PutByteContext *pb, const ui > >>> b = lut[2 * idx]; > >>> > >>> while (1) { > >>> - if (bytestream2_get_bytes_left_p(pb) <= 0) > >>> + if (bytestream2_get_bytes_left_p(pb) <= 0 || bytestream2_get_eof(pb)) > >>> return 0; > >>> if (((b & 0xFF00u) != 0x8000u) || (b & 0xFFu)) { > >>> if ((b & 0xFF00u) != 0x8000u) { > >> > >> Why does bytestream2_get_bytes_left_p not return <= 0 at eof, where > >> there certainly arent any bytes left? > > > > there are 2 bytes left because a 4 byte write failed to fit in 2 bytes > > > > maybe bytestream2_get_bytes_left_p() should be made to return 0 once eof is > > set even if space remains > > Unrelated to this, but seeing how all the main GetByteContext functions > start with bytestream2_get and all the PutByteContext functions with > bytestream2_put, the fact these two functions from the latter start with > bytestream2_get is not ideal. > You have bytestream2_get_bytes_left for GetByteContext and > bytestream2_get_bytes_left_p with the suffix p hinting it's for > PutByteContext, but then you have bytestream2_get_eof for PutByteContext > without a suffix and nothing for GetByteContext. It's confusing and not > exactly intuitive. fully agree > > In GetBitContext and PutBitContext they are properly called > get_bits_left() and put_bits_left() respectively, for example. > _______________________________________________ > ffmpeg-devel mailing list > ffmpeg-devel@ffmpeg.org > http://ffmpeg.org/mailman/listinfo/ffmpeg-devel
On Wed, Oct 31, 2018 at 12:13:50PM +0100, Paul B Mahol wrote: > On 10/31/18, Michael Niedermayer <michael@niedermayer.cc> wrote: > > Fixes: Infinite loop > > Fixes: > > 10685/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PROSUMER_fuzzer-5652236881887232 > > > > Found-by: continuous fuzzing process > > https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/prosumer.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > patchset looks ok. will apply thx [...]
diff --git a/libavcodec/prosumer.c b/libavcodec/prosumer.c index 6e98677b55..2fd9880ee1 100644 --- a/libavcodec/prosumer.c +++ b/libavcodec/prosumer.c @@ -57,7 +57,7 @@ static int decompress(GetByteContext *gb, int size, PutByteContext *pb, const ui b = lut[2 * idx]; while (1) { - if (bytestream2_get_bytes_left_p(pb) <= 0) + if (bytestream2_get_bytes_left_p(pb) <= 0 || bytestream2_get_eof(pb)) return 0; if (((b & 0xFF00u) != 0x8000u) || (b & 0xFFu)) { if ((b & 0xFF00u) != 0x8000u) {
Fixes: Infinite loop Fixes: 10685/clusterfuzz-testcase-minimized-ffmpeg_AV_CODEC_ID_PROSUMER_fuzzer-5652236881887232 Found-by: continuous fuzzing process https://github.com/google/oss-fuzz/tree/master/projects/ffmpeg Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/prosumer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)