| Message ID | 20190605021854.26095-1-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | d4035ca849bdb90e95c87e2737a99ea657be0716 |
| Headers | show |
On 05/06/2019 03:18, Andreas Rheinhardt wrote: > cbs_h2645_read_more_rbsp_data does not handle malformed input very well: > 1. If there were <= 8 bits left in the bitreader, these bits were read > via show_bits. But show_bits requires the number of bits to be read to > be > 0 (internally it shifts by 32 - number of bits to be read which is > undefined behaviour if said number is zero; there is also an assert for > this, but it is only an av_assert2). Furthermore, in this case a shift > by -1 was performed which is of course undefined behaviour, too. > 2. If there were > 0 and <= 8 bits left and all of them were zero > (this can only happen for defective input), it was reported that there > was further RBSP data. > > This can lead to an infinite loop in H.265's cbs_h265_read_extension_data > corresponding to the [vsp]ps_extension_data_flag syntax elements. If the > relevant flag indicates the (potential) occurence of these syntax elements, > while all bits after this flag are zero, cbs_h2645_read_more_rbsp_data > always returns 1 on x86. Given that a checked bitstream reader is used, > we are also not "saved" by an overflow in the bitstream reader's index. > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > libavcodec/cbs_h2645.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c > index 0456937710..becb63a290 100644 > --- a/libavcodec/cbs_h2645.c > +++ b/libavcodec/cbs_h2645.c > @@ -328,9 +328,11 @@ static int cbs_h2645_read_more_rbsp_data(GetBitContext *gbc) > int bits_left = get_bits_left(gbc); > if (bits_left > 8) > return 1; > - if (show_bits(gbc, bits_left) == 1 << (bits_left - 1)) > + if (bits_left == 0) > return 0; > - return 1; > + if (show_bits(gbc, bits_left) & MAX_UINT_BITS(bits_left - 1)) > + return 1; > + return 0; > } > > #define more_rbsp_data(var) ((var) = cbs_h2645_read_more_rbsp_data(rw)) > Good catch! Applied. Thanks, - Mark
diff --git a/libavcodec/cbs_h2645.c b/libavcodec/cbs_h2645.c index 0456937710..becb63a290 100644 --- a/libavcodec/cbs_h2645.c +++ b/libavcodec/cbs_h2645.c @@ -328,9 +328,11 @@ static int cbs_h2645_read_more_rbsp_data(GetBitContext *gbc) int bits_left = get_bits_left(gbc); if (bits_left > 8) return 1; - if (show_bits(gbc, bits_left) == 1 << (bits_left - 1)) + if (bits_left == 0) return 0; - return 1; + if (show_bits(gbc, bits_left) & MAX_UINT_BITS(bits_left - 1)) + return 1; + return 0; } #define more_rbsp_data(var) ((var) = cbs_h2645_read_more_rbsp_data(rw))
cbs_h2645_read_more_rbsp_data does not handle malformed input very well: 1. If there were <= 8 bits left in the bitreader, these bits were read via show_bits. But show_bits requires the number of bits to be read to be > 0 (internally it shifts by 32 - number of bits to be read which is undefined behaviour if said number is zero; there is also an assert for this, but it is only an av_assert2). Furthermore, in this case a shift by -1 was performed which is of course undefined behaviour, too. 2. If there were > 0 and <= 8 bits left and all of them were zero (this can only happen for defective input), it was reported that there was further RBSP data. This can lead to an infinite loop in H.265's cbs_h265_read_extension_data corresponding to the [vsp]ps_extension_data_flag syntax elements. If the relevant flag indicates the (potential) occurence of these syntax elements, while all bits after this flag are zero, cbs_h2645_read_more_rbsp_data always returns 1 on x86. Given that a checked bitstream reader is used, we are also not "saved" by an overflow in the bitstream reader's index. Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- libavcodec/cbs_h2645.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)