| Message ID | 20200714153454.9354-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [FFmpeg-devel] avcodec/golomb: Prevent shift by negative number | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On 7/14/2020 12:34 PM, Andreas Rheinhardt wrote: > This is designed for scenarios where the caller already checks that the > returned value is within a certain allowed range and returns an error > message if not. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/golomb.h | 32 ++++++++++++++++++++++++-------- > 1 file changed, 24 insertions(+), 8 deletions(-) > > diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h > index 5bfcfe085f..63069f63e5 100644 > --- a/libavcodec/golomb.h > +++ b/libavcodec/golomb.h > @@ -47,12 +47,7 @@ extern const uint8_t ff_interleaved_ue_golomb_vlc_code[256]; > extern const int8_t ff_interleaved_se_golomb_vlc_code[256]; > extern const uint8_t ff_interleaved_dirac_golomb_vlc_code[256]; > > -/** > - * Read an unsigned Exp-Golomb code in the range 0 to 8190. > - * > - * @returns the read value or a negative error code. > - */ > -static inline int get_ue_golomb(GetBitContext *gb) > +static inline int get_ue_golomb_internal(GetBitContext *gb, int emit_error_msg) > { > unsigned int buf; > > @@ -67,7 +62,8 @@ static inline int get_ue_golomb(GetBitContext *gb) > } else { > int log = 2 * av_log2(buf) - 31; > if (log < 0) { > - av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n"); > + if (emit_error_msg) > + av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n"); > return AVERROR_INVALIDDATA; > } > buf >>= log; > @@ -92,7 +88,8 @@ static inline int get_ue_golomb(GetBitContext *gb) > LAST_SKIP_BITS(re, gb, 32 - log); > CLOSE_READER(re, gb); > if (log < 7) { > - av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n"); > + if (emit_error_msg) > + av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n"); > return AVERROR_INVALIDDATA; > } > buf >>= log; > @@ -103,6 +100,25 @@ static inline int get_ue_golomb(GetBitContext *gb) > #endif > } > > +/** > + * Read an unsigned Exp-Golomb code in the range 0 to 8190. > + * > + * @returns the read value or a negative error code. > + */ > +static inline int get_ue_golomb(GetBitContext *gb) > +{ > + return get_ue_golomb_internal(gb, 1); > +} > + > +/** > + * Variant of get_ue_golomb that does not emit an error message > + * if the number is outside the permissible range. > + */ > +static inline int get_ue_golomb2(GetBitContext *gb) Seeing there's precedent for functions where the number suffix relates to the range of values they can parse, i think using a 2 here could be confusing at first glance. I suggest to use something else. Is it worth keeping the error messages at all, for that matter? They don't even use a proper logging context, so the printed output is ugly and unidentifiable by itself. > +{ > + return get_ue_golomb_internal(gb, 0); > +} > + > /** > * Read an unsigned Exp-Golomb code in the range 0 to UINT32_MAX-1. > */ >
James Almer: > On 7/14/2020 12:34 PM, Andreas Rheinhardt wrote: >> This is designed for scenarios where the caller already checks that the >> returned value is within a certain allowed range and returns an error >> message if not. >> >> Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> >> --- >> libavcodec/golomb.h | 32 ++++++++++++++++++++++++-------- >> 1 file changed, 24 insertions(+), 8 deletions(-) >> >> diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h >> index 5bfcfe085f..63069f63e5 100644 >> --- a/libavcodec/golomb.h >> +++ b/libavcodec/golomb.h >> @@ -47,12 +47,7 @@ extern const uint8_t ff_interleaved_ue_golomb_vlc_code[256]; >> extern const int8_t ff_interleaved_se_golomb_vlc_code[256]; >> extern const uint8_t ff_interleaved_dirac_golomb_vlc_code[256]; >> >> -/** >> - * Read an unsigned Exp-Golomb code in the range 0 to 8190. >> - * >> - * @returns the read value or a negative error code. >> - */ >> -static inline int get_ue_golomb(GetBitContext *gb) >> +static inline int get_ue_golomb_internal(GetBitContext *gb, int emit_error_msg) >> { >> unsigned int buf; >> >> @@ -67,7 +62,8 @@ static inline int get_ue_golomb(GetBitContext *gb) >> } else { >> int log = 2 * av_log2(buf) - 31; >> if (log < 0) { >> - av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n"); >> + if (emit_error_msg) >> + av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n"); >> return AVERROR_INVALIDDATA; >> } >> buf >>= log; >> @@ -92,7 +88,8 @@ static inline int get_ue_golomb(GetBitContext *gb) >> LAST_SKIP_BITS(re, gb, 32 - log); >> CLOSE_READER(re, gb); >> if (log < 7) { >> - av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n"); >> + if (emit_error_msg) >> + av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n"); >> return AVERROR_INVALIDDATA; >> } >> buf >>= log; >> @@ -103,6 +100,25 @@ static inline int get_ue_golomb(GetBitContext *gb) >> #endif >> } >> >> +/** >> + * Read an unsigned Exp-Golomb code in the range 0 to 8190. >> + * >> + * @returns the read value or a negative error code. >> + */ >> +static inline int get_ue_golomb(GetBitContext *gb) >> +{ >> + return get_ue_golomb_internal(gb, 1); >> +} >> + >> +/** >> + * Variant of get_ue_golomb that does not emit an error message >> + * if the number is outside the permissible range. >> + */ >> +static inline int get_ue_golomb2(GetBitContext *gb) > > Seeing there's precedent for functions where the number suffix relates > to the range of values they can parse, i think using a 2 here could be > confusing at first glance. I suggest to use something else. > > Is it worth keeping the error messages at all, for that matter? They > don't even use a proper logging context, so the printed output is ugly > and unidentifiable by itself. > I could live very well without the error messages. If there are no objections, I will just remove them (and not add them in the cached codepath). - Andreas
James Almer (12020-07-14): > Is it worth keeping the error messages at all, for that matter? They > don't even use a proper logging context, so the printed output is ugly > and unidentifiable by itself. Good point. We should probably try to eliminate all av_log(NULL) uses from the libraries, in fact. There are only ~300 of them, after all, that can be done. Regards,
diff --git a/libavcodec/golomb.h b/libavcodec/golomb.h index 5bfcfe085f..63069f63e5 100644 --- a/libavcodec/golomb.h +++ b/libavcodec/golomb.h @@ -47,12 +47,7 @@ extern const uint8_t ff_interleaved_ue_golomb_vlc_code[256]; extern const int8_t ff_interleaved_se_golomb_vlc_code[256]; extern const uint8_t ff_interleaved_dirac_golomb_vlc_code[256]; -/** - * Read an unsigned Exp-Golomb code in the range 0 to 8190. - * - * @returns the read value or a negative error code. - */ -static inline int get_ue_golomb(GetBitContext *gb) +static inline int get_ue_golomb_internal(GetBitContext *gb, int emit_error_msg) { unsigned int buf; @@ -67,7 +62,8 @@ static inline int get_ue_golomb(GetBitContext *gb) } else { int log = 2 * av_log2(buf) - 31; if (log < 0) { - av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n"); + if (emit_error_msg) + av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n"); return AVERROR_INVALIDDATA; } buf >>= log; @@ -92,7 +88,8 @@ static inline int get_ue_golomb(GetBitContext *gb) LAST_SKIP_BITS(re, gb, 32 - log); CLOSE_READER(re, gb); if (log < 7) { - av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n"); + if (emit_error_msg) + av_log(NULL, AV_LOG_ERROR, "Invalid UE golomb code\n"); return AVERROR_INVALIDDATA; } buf >>= log; @@ -103,6 +100,25 @@ static inline int get_ue_golomb(GetBitContext *gb) #endif } +/** + * Read an unsigned Exp-Golomb code in the range 0 to 8190. + * + * @returns the read value or a negative error code. + */ +static inline int get_ue_golomb(GetBitContext *gb) +{ + return get_ue_golomb_internal(gb, 1); +} + +/** + * Variant of get_ue_golomb that does not emit an error message + * if the number is outside the permissible range. + */ +static inline int get_ue_golomb2(GetBitContext *gb) +{ + return get_ue_golomb_internal(gb, 0); +} + /** * Read an unsigned Exp-Golomb code in the range 0 to UINT32_MAX-1. */
This is designed for scenarios where the caller already checks that the returned value is within a certain allowed range and returns an error message if not. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavcodec/golomb.h | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-)