| Message ID | 20240428213052.3800493-2-michael@niedermayer.cc |
|---|---|
| State | New |
| Headers | show |
| Series | [FFmpeg-devel,1/5] tools/opt_common: Check for malloc failure | expand |
| Context | Check | Description |
|---|---|---|
| yinshiyou/make_loongarch64 | success | Make finished |
| yinshiyou/make_fate_loongarch64 | success | Make fate finished |
Apr 28, 2024, 23:31 by michael@niedermayer.cc: > Inspired by CID1465483 Unintentional integer overflow > > Sponsored-by: Sovereign Tech Fund > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > --- > libavcodec/aaccoder.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/libavcodec/aaccoder.c b/libavcodec/aaccoder.c > index 4ce54ca8867..6e5817e237b 100644 > --- a/libavcodec/aaccoder.c > +++ b/libavcodec/aaccoder.c > @@ -178,6 +178,8 @@ static av_always_inline float quantize_and_encode_band_cost_template( > int coef = av_clip_uintp2(quant(fabsf(in[i+j]), Q, ROUNDING), 13); > int len = av_log2(coef); > > + av_assert2(len >= 4); > + > put_bits(pb, len - 4 + 1, (1 << (len - 4 + 1)) - 2); > put_sbits(pb, len, coef); > } > I'm not sure that's correct to do. Any specific cases where this happens?
On Sun, Apr 28, 2024 at 11:41:20PM +0200, Lynne wrote: > Apr 28, 2024, 23:31 by michael@niedermayer.cc: > > > Inspired by CID1465483 Unintentional integer overflow > > > > Sponsored-by: Sovereign Tech Fund > > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > > --- > > libavcodec/aaccoder.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/libavcodec/aaccoder.c b/libavcodec/aaccoder.c > > index 4ce54ca8867..6e5817e237b 100644 > > --- a/libavcodec/aaccoder.c > > +++ b/libavcodec/aaccoder.c > > @@ -178,6 +178,8 @@ static av_always_inline float quantize_and_encode_band_cost_template( > > int coef = av_clip_uintp2(quant(fabsf(in[i+j]), Q, ROUNDING), 13); > > int len = av_log2(coef); > > > > + av_assert2(len >= 4); > > + > > put_bits(pb, len - 4 + 1, (1 << (len - 4 + 1)) - 2); > > put_sbits(pb, len, coef); > > } > > > > I'm not sure that's correct to do. Any specific cases where this happens? if len is 3 or less then put_bits will have a negative value or undefined shift coverity sasid this: " overflow_before_widen: Potentially overflowing expression 1 << len - 4 + 1 with type int (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type BitBuf (64 bits, unsigned). To avoid overflow, cast 1 to type BitBuf." So what coverity really said is that the expression could exeed 32bit _because_ its used in 64bit context. Thats just stupid from coverity also teh clip above limits this to 13 bit so i dont see how it can overflow in the "too large" direction and i marked this one as false positive. I wasnt 100% sure about the too small side, i tested it and its never too small but coverity didnt claim it could be too small, so that question is outside the issue So i added a assert in this patch. thx [...]
Apr 28, 2024, 23:57 by michael@niedermayer.cc: > On Sun, Apr 28, 2024 at 11:41:20PM +0200, Lynne wrote: > >> Apr 28, 2024, 23:31 by michael@niedermayer.cc: >> >> > Inspired by CID1465483 Unintentional integer overflow >> > >> > Sponsored-by: Sovereign Tech Fund >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> >> > --- >> > libavcodec/aaccoder.c | 2 ++ >> > 1 file changed, 2 insertions(+) >> > >> > diff --git a/libavcodec/aaccoder.c b/libavcodec/aaccoder.c >> > index 4ce54ca8867..6e5817e237b 100644 >> > --- a/libavcodec/aaccoder.c >> > +++ b/libavcodec/aaccoder.c >> > @@ -178,6 +178,8 @@ static av_always_inline float quantize_and_encode_band_cost_template( >> > int coef = av_clip_uintp2(quant(fabsf(in[i+j]), Q, ROUNDING), 13); >> > int len = av_log2(coef); >> > >> > + av_assert2(len >= 4); >> > + >> > put_bits(pb, len - 4 + 1, (1 << (len - 4 + 1)) - 2); >> > put_sbits(pb, len, coef); >> > } >> > >> >> I'm not sure that's correct to do. Any specific cases where this happens? >> > > if len is 3 or less then put_bits will have a negative value or > undefined shift > > coverity sasid this: > " overflow_before_widen: Potentially overflowing expression 1 << len - 4 + 1 with type int (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type BitBuf (64 bits, unsigned). To avoid overflow, cast 1 to type BitBuf." > > So what coverity really said is that the expression could exeed 32bit _because_ its > used in 64bit context. Thats just stupid from coverity also teh clip above > limits this to 13 bit so i dont see how it can overflow in the "too large" direction > and i marked this one as false positive. > > I wasnt 100% sure about the too small side, i tested it and its never too small > but coverity didnt claim it could be too small, so that question is outside the issue > So i added a assert in this patch. > Could you instead use 64-bit arithmetic for `coef`? Such a general assert here could result in false alarms, like the AAC encoder suffered from before, which would make development harder.
On Mon, Apr 29, 2024 at 12:26:24AM +0200, Lynne wrote: > Apr 28, 2024, 23:57 by michael@niedermayer.cc: > > > On Sun, Apr 28, 2024 at 11:41:20PM +0200, Lynne wrote: > > > >> Apr 28, 2024, 23:31 by michael@niedermayer.cc: > >> > >> > Inspired by CID1465483 Unintentional integer overflow > >> > > >> > Sponsored-by: Sovereign Tech Fund > >> > Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> > >> > --- > >> > libavcodec/aaccoder.c | 2 ++ > >> > 1 file changed, 2 insertions(+) > >> > > >> > diff --git a/libavcodec/aaccoder.c b/libavcodec/aaccoder.c > >> > index 4ce54ca8867..6e5817e237b 100644 > >> > --- a/libavcodec/aaccoder.c > >> > +++ b/libavcodec/aaccoder.c > >> > @@ -178,6 +178,8 @@ static av_always_inline float quantize_and_encode_band_cost_template( > >> > int coef = av_clip_uintp2(quant(fabsf(in[i+j]), Q, ROUNDING), 13); > >> > int len = av_log2(coef); > >> > > >> > + av_assert2(len >= 4); > >> > + > >> > put_bits(pb, len - 4 + 1, (1 << (len - 4 + 1)) - 2); > >> > put_sbits(pb, len, coef); > >> > } > >> > > >> > >> I'm not sure that's correct to do. Any specific cases where this happens? > >> > > > > if len is 3 or less then put_bits will have a negative value or > > undefined shift > > > > coverity sasid this: > > " overflow_before_widen: Potentially overflowing expression 1 << len - 4 + 1 with type int (32 bits, signed) is evaluated using 32-bit arithmetic, and then used in a context that expects an expression of type BitBuf (64 bits, unsigned). To avoid overflow, cast 1 to type BitBuf." > > > > So what coverity really said is that the expression could exeed 32bit _because_ its > > used in 64bit context. Thats just stupid from coverity also teh clip above > > limits this to 13 bit so i dont see how it can overflow in the "too large" direction > > and i marked this one as false positive. > > > > I wasnt 100% sure about the too small side, i tested it and its never too small > > but coverity didnt claim it could be too small, so that question is outside the issue > > So i added a assert in this patch. > > > > Could you instead use 64-bit arithmetic for `coef`? > Such a general assert here could result in false alarms, like the AAC > encoder suffered from before, which would make development harder. If you dont like the assert, then its best to just drop the patch. It doesnt fix anything, it was just intended to clarify that the case of len < 4 doesnt occur. The coverity issue is a false positive thx [...]
diff --git a/libavcodec/aaccoder.c b/libavcodec/aaccoder.c index 4ce54ca8867..6e5817e237b 100644 --- a/libavcodec/aaccoder.c +++ b/libavcodec/aaccoder.c @@ -178,6 +178,8 @@ static av_always_inline float quantize_and_encode_band_cost_template( int coef = av_clip_uintp2(quant(fabsf(in[i+j]), Q, ROUNDING), 13); int len = av_log2(coef); + av_assert2(len >= 4); + put_bits(pb, len - 4 + 1, (1 << (len - 4 + 1)) - 2); put_sbits(pb, len, coef); }
Inspired by CID1465483 Unintentional integer overflow Sponsored-by: Sovereign Tech Fund Signed-off-by: Michael Niedermayer <michael@niedermayer.cc> --- libavcodec/aaccoder.c | 2 ++ 1 file changed, 2 insertions(+)