| Message ID | 20201008175756.290617-2-andreas.rheinhardt@gmail.com |
|---|---|
| State | Accepted |
| Commit | a21dec5d0a970a7dc710cb4d6af8f44f9be9539b |
| Headers | show |
| Series | [FFmpeg-devel,1/2] avcodec/mjpegdec: Use correct number of codes for VLC tables | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
On Thu, Oct 08, 2020 at 07:57:56PM +0200, Andreas Rheinhardt wrote: > Commit bbc0d0c1fe2b7ecdc4367295594f084f85ad22f5 made the mjpeg decoder > use default Huffman tables when none are given, yet when initializing > the default Huffman tables, it did not use the correct number of entries > of the arrays used to initialize the tables, but instead it used the > biggest entry + 1 (as if it were a continuous array 0..biggest entry). > This worked because the ff_init_vlc_sparse() (and its predecessors) > always skipped entries with a length of zero and the length of the > corresponding elements was always initialized to zero with only the > sizes of the actually existing elements being set to a size > 0 lateron. > > Yet since commit 1249698e1b424cff8e77e6a83cfdbc9d11e01aa7 this is no > longer so, as build_vlc() actually read the array containing the values > itself. This implies that the wrong length now leads to a read beyond > the end of the given array; this could lead to crashs (but usually > doesn't); it is detectable by ASAN* and this commit fixes it. > > *: AddressSanitizer: global-buffer-overflow on address xy > ... > xy is located 0 bytes to the right of global variable 'avpriv_mjpeg_val_ac_luminance' > > Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> > --- > Hard to believe that this wrong number has not been found earlier. > The code in question has been touched by about a dozen commits. > Patchset LGTM
diff --git a/libavcodec/mjpegdec.c b/libavcodec/mjpegdec.c index 4128c47303..0a5ef110d1 100644 --- a/libavcodec/mjpegdec.c +++ b/libavcodec/mjpegdec.c @@ -96,27 +96,26 @@ static int init_default_huffman_tables(MJpegDecodeContext *s) int index; const uint8_t *bits; const uint8_t *values; - int codes; int length; } ht[] = { { 0, 0, avpriv_mjpeg_bits_dc_luminance, - avpriv_mjpeg_val_dc, 12, 12 }, + avpriv_mjpeg_val_dc, 12 }, { 0, 1, avpriv_mjpeg_bits_dc_chrominance, - avpriv_mjpeg_val_dc, 12, 12 }, + avpriv_mjpeg_val_dc, 12 }, { 1, 0, avpriv_mjpeg_bits_ac_luminance, - avpriv_mjpeg_val_ac_luminance, 251, 162 }, + avpriv_mjpeg_val_ac_luminance, 162 }, { 1, 1, avpriv_mjpeg_bits_ac_chrominance, - avpriv_mjpeg_val_ac_chrominance, 251, 162 }, + avpriv_mjpeg_val_ac_chrominance, 162 }, { 2, 0, avpriv_mjpeg_bits_ac_luminance, - avpriv_mjpeg_val_ac_luminance, 251, 162 }, + avpriv_mjpeg_val_ac_luminance, 162 }, { 2, 1, avpriv_mjpeg_bits_ac_chrominance, - avpriv_mjpeg_val_ac_chrominance, 251, 162 }, + avpriv_mjpeg_val_ac_chrominance, 162 }, }; int i, ret; for (i = 0; i < FF_ARRAY_ELEMS(ht); i++) { ret = build_vlc(&s->vlcs[ht[i].class][ht[i].index], - ht[i].bits, ht[i].values, ht[i].codes, + ht[i].bits, ht[i].values, ht[i].length, 0, ht[i].class == 1); if (ret < 0) return ret;
Commit bbc0d0c1fe2b7ecdc4367295594f084f85ad22f5 made the mjpeg decoder use default Huffman tables when none are given, yet when initializing the default Huffman tables, it did not use the correct number of entries of the arrays used to initialize the tables, but instead it used the biggest entry + 1 (as if it were a continuous array 0..biggest entry). This worked because the ff_init_vlc_sparse() (and its predecessors) always skipped entries with a length of zero and the length of the corresponding elements was always initialized to zero with only the sizes of the actually existing elements being set to a size > 0 lateron. Yet since commit 1249698e1b424cff8e77e6a83cfdbc9d11e01aa7 this is no longer so, as build_vlc() actually read the array containing the values itself. This implies that the wrong length now leads to a read beyond the end of the given array; this could lead to crashs (but usually doesn't); it is detectable by ASAN* and this commit fixes it. *: AddressSanitizer: global-buffer-overflow on address xy ... xy is located 0 bytes to the right of global variable 'avpriv_mjpeg_val_ac_luminance' Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com> --- Hard to believe that this wrong number has not been found earlier. The code in question has been touched by about a dozen commits. libavcodec/mjpegdec.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)