Discussion:
[PATCH 01/13] h264_cavlc: check the size of the intra PCM data.
(too old to reply)
Anton Khirnov
2013-11-15 21:13:39 UTC
Permalink
Fixes invalid reads.
Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC:libav-***@libav.org
---
libavcodec/h264_cavlc.c | 4 ++++
1 file changed, 4 insertions(+)

diff --git a/libavcodec/h264_cavlc.c b/libavcodec/h264_cavlc.c
index 5ed1d5d..d3f6dcb 100644
--- a/libavcodec/h264_cavlc.c
+++ b/libavcodec/h264_cavlc.c
@@ -765,6 +765,10 @@ decode_intra_mb:

// We assume these blocks are very rare so we do not optimize it.
h->intra_pcm_ptr = align_get_bits(&h->gb);
+ if (get_bits_left(&h->gb) < mb_size) {
+ av_log(h->avctx, AV_LOG_ERROR, "Not enough data for an intra PCM block.\n");
+ return AVERROR_INVALIDDATA;
+ }
skip_bits_long(&h->gb, mb_size);

// In deblocking, the quantizer is 0
--
1.7.10.4
Anton Khirnov
2013-11-15 21:13:46 UTC
Permalink
They are used when decoding the frame header.
Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC:libav-***@libav.org
---
libavcodec/mpeg4video.h | 2 ++
libavcodec/mpeg4video_parser.c | 2 ++
libavcodec/mpeg4videodec.c | 4 ++--
3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/libavcodec/mpeg4video.h b/libavcodec/mpeg4video.h
index c22fbfd..d7157d3 100644
--- a/libavcodec/mpeg4video.h
+++ b/libavcodec/mpeg4video.h
@@ -112,6 +112,8 @@ void ff_mpeg4_init_direct_mv(MpegEncContext *s);
*/
int ff_mpeg4_set_direct_mv(MpegEncContext *s, int mx, int my);

+void ff_mpeg4_init_tables(void);
+
extern uint8_t ff_mpeg4_static_rl_table_store[3][2][2 * MAX_RUN + MAX_LEVEL + 3];

#if 0 //3IV1 is quite rare and it slows things down a tiny bit
diff --git a/libavcodec/mpeg4video_parser.c b/libavcodec/mpeg4video_parser.c
index a8def0e..dfddeda 100644
--- a/libavcodec/mpeg4video_parser.c
+++ b/libavcodec/mpeg4video_parser.c
@@ -105,6 +105,8 @@ static av_cold int mpeg4video_parse_init(AVCodecParserContext *s)
{
struct Mp4vParseContext *pc = s->priv_data;

+ ff_mpeg4_init_tables();
+
pc->first_picture = 1;
pc->enc.slice_context_count = 1;
return 0;
diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
index b36c572..dd8f565 100644
--- a/libavcodec/mpeg4videodec.c
+++ b/libavcodec/mpeg4videodec.c
@@ -47,7 +47,7 @@ static const int mb_type_b_map[4]= {
MB_TYPE_L0 | MB_TYPE_16x16,
};

-static void init_tables(void)
+void ff_mpeg4_init_tables(void)
{
static int done = 0;
if (!done) {
@@ -2224,7 +2224,7 @@ static av_cold int decode_init(AVCodecContext *avctx)
MpegEncContext *s = avctx->priv_data;
int ret;

- init_tables();
+ ff_mpeg4_init_tables();

s->divx_version=
s->divx_build=
--
1.7.10.4
Luca Barbato
2013-11-15 21:17:47 UTC
Permalink
On 15/11/13 22:13, Anton Khirnov wrote:
> They are used when decoding the frame header.
> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC:libav-***@libav.org
> ---
> libavcodec/mpeg4video.h | 2 ++
> libavcodec/mpeg4video_parser.c | 2 ++
> libavcodec/mpeg4videodec.c | 4 ++--
> 3 files changed, 6 insertions(+), 2 deletions(-)
>

Seems fine.
Martin Storsjö
2013-11-15 21:20:22 UTC
Permalink
On Fri, 15 Nov 2013, Anton Khirnov wrote:

> They are used when decoding the frame header.
> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC:libav-***@libav.org
> ---
> libavcodec/mpeg4video.h | 2 ++
> libavcodec/mpeg4video_parser.c | 2 ++
> libavcodec/mpeg4videodec.c | 4 ++--
> 3 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/libavcodec/mpeg4video.h b/libavcodec/mpeg4video.h
> index c22fbfd..d7157d3 100644
> --- a/libavcodec/mpeg4video.h
> +++ b/libavcodec/mpeg4video.h
> @@ -112,6 +112,8 @@ void ff_mpeg4_init_direct_mv(MpegEncContext *s);
> */
> int ff_mpeg4_set_direct_mv(MpegEncContext *s, int mx, int my);
>
> +void ff_mpeg4_init_tables(void);
> +
> extern uint8_t ff_mpeg4_static_rl_table_store[3][2][2 * MAX_RUN + MAX_LEVEL + 3];
>
> #if 0 //3IV1 is quite rare and it slows things down a tiny bit
> diff --git a/libavcodec/mpeg4video_parser.c b/libavcodec/mpeg4video_parser.c
> index a8def0e..dfddeda 100644
> --- a/libavcodec/mpeg4video_parser.c
> +++ b/libavcodec/mpeg4video_parser.c
> @@ -105,6 +105,8 @@ static av_cold int mpeg4video_parse_init(AVCodecParserContext *s)
> {
> struct Mp4vParseContext *pc = s->priv_data;
>
> + ff_mpeg4_init_tables();
> +

Contrary to the normal codec init function, this can get called from any
thread outside of the normal avcodec init lock. OTOH, if the static init
code is safe to be run in parallel from multiple threads I guess it isn't
an issue, but it's worth noting.

// Martin
Anton Khirnov
2013-11-15 21:13:47 UTC
Permalink
Fixes invalid reads.
Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC:libav-***@libav.org
---
libavcodec/motionpixels.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/libavcodec/motionpixels.c b/libavcodec/motionpixels.c
index 6a7dd23..84fbc4d 100644
--- a/libavcodec/motionpixels.c
+++ b/libavcodec/motionpixels.c
@@ -161,6 +161,7 @@ static int mp_get_vlc(MotionPixelsContext *mp, GetBitContext *gb)
int i;

i = (mp->codes_count == 1) ? 0 : get_vlc2(gb, mp->vlc.table, mp->max_codes_bits, 1);
+ i = FFMIN(i, FF_ARRAY_ELEMS(mp->codes) - 1);
return mp->codes[i].delta;
}

--
1.7.10.4
Luca Barbato
2013-11-15 21:18:01 UTC
Permalink
On 15/11/13 22:13, Anton Khirnov wrote:
> Fixes invalid reads.
> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC:libav-***@libav.org
> ---
> libavcodec/motionpixels.c | 1 +
> 1 file changed, 1 insertion(+)
>

Ok.
Anton Khirnov
2013-11-15 21:13:42 UTC
Permalink
Fixes invalid reads.
Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC:libav-***@libav.org
---
libavcodec/h264.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/h264.c b/libavcodec/h264.c
index 1444fc6..9870174 100644
--- a/libavcodec/h264.c
+++ b/libavcodec/h264.c
@@ -4552,7 +4552,7 @@ static int decode_nal_units(H264Context *h, const uint8_t *buf, int buf_size,
h->workaround_bugs |= FF_BUG_TRUNCATED;

if (!(h->workaround_bugs & FF_BUG_TRUNCATED))
- while (ptr[dst_length - 1] == 0 && dst_length > 0)
+ while (dst_length > 0 && ptr[dst_length - 1] == 0)
dst_length--;
bit_length = !dst_length ? 0
: (8 * dst_length -
--
1.7.10.4
Luca Barbato
2013-11-15 21:34:44 UTC
Permalink
On 15/11/13 22:13, Anton Khirnov wrote:
> Fixes invalid reads.
> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC:libav-***@libav.org
> ---
> libavcodec/h264.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>

Looks ok.

lu
Anton Khirnov
2013-11-15 21:13:44 UTC
Permalink
From: Aurelien Jacobs <***@gnuage.org>

Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC:libav-***@libav.org
Signed-off-by: Anton Khirnov <***@khirnov.net>
---
libavformat/matroskadec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index 764dbf8..bcd7d1a 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -1455,7 +1455,7 @@ static int matroska_read_header(AVFormatContext *s)
for (i=0; i < matroska->tracks.nb_elem; i++) {
MatroskaTrack *track = &tracks[i];
enum AVCodecID codec_id = AV_CODEC_ID_NONE;
- EbmlList *encodings_list = &tracks->encodings;
+ EbmlList *encodings_list = &track->encodings;
MatroskaTrackEncoding *encodings = encodings_list->elem;
uint8_t *extradata = NULL;
int extradata_size = 0;
--
1.7.10.4
Luca Barbato
2013-11-15 21:39:17 UTC
Permalink
On 15/11/13 22:13, Anton Khirnov wrote:
> From: Aurelien Jacobs <***@gnuage.org>
>
> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC:libav-***@libav.org
> Signed-off-by: Anton Khirnov <***@khirnov.net>
> ---
> libavformat/matroskadec.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>

Probably ok (tracks vs track is a typo waiting to happen =/)

lu
Anton Khirnov
2013-11-15 21:13:48 UTC
Permalink
Happens on a B-frame when neither low_delay nor last_picture_ptr is set
(probably corrupted streams only).

Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
---
libavcodec/vc1dec.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/libavcodec/vc1dec.c b/libavcodec/vc1dec.c
index 231f425..507574c 100644
--- a/libavcodec/vc1dec.c
+++ b/libavcodec/vc1dec.c
@@ -6078,12 +6078,11 @@ image:
if ((ret = av_frame_ref(pict, &s->current_picture_ptr->f)) < 0)
goto err;
ff_print_debug_info(s, s->current_picture_ptr);
+ *got_frame = 1;
} else if (s->last_picture_ptr != NULL) {
if ((ret = av_frame_ref(pict, &s->last_picture_ptr->f)) < 0)
goto err;
ff_print_debug_info(s, s->last_picture_ptr);
- }
- if (s->last_picture_ptr || s->low_delay) {
*got_frame = 1;
}
}
--
1.7.10.4
Vittorio Giovara
2013-11-18 17:18:03 UTC
Permalink
On Fri, Nov 15, 2013 at 10:13 PM, Anton Khirnov <***@khirnov.net> wrote:
> Happens on a B-frame when neither low_delay nor last_picture_ptr is set
> (probably corrupted streams only).
>
> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> ---
> libavcodec/vc1dec.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/libavcodec/vc1dec.c b/libavcodec/vc1dec.c
> index 231f425..507574c 100644
> --- a/libavcodec/vc1dec.c
> +++ b/libavcodec/vc1dec.c
> @@ -6078,12 +6078,11 @@ image:
> if ((ret = av_frame_ref(pict, &s->current_picture_ptr->f)) < 0)
> goto err;
> ff_print_debug_info(s, s->current_picture_ptr);
> + *got_frame = 1;
> } else if (s->last_picture_ptr != NULL) {
> if ((ret = av_frame_ref(pict, &s->last_picture_ptr->f)) < 0)
> goto err;
> ff_print_debug_info(s, s->last_picture_ptr);
> - }
> - if (s->last_picture_ptr || s->low_delay) {
> *got_frame = 1;
> }
> }

I guess ok.
Vittorio
Anton Khirnov
2013-11-15 21:13:45 UTC
Permalink
---
libavcodec/mpeg4videodec.c | 53 ++++++++++++++++++++++++--------------------
1 file changed, 29 insertions(+), 24 deletions(-)

diff --git a/libavcodec/mpeg4videodec.c b/libavcodec/mpeg4videodec.c
index 9a0b42b..b36c572 100644
--- a/libavcodec/mpeg4videodec.c
+++ b/libavcodec/mpeg4videodec.c
@@ -47,6 +47,33 @@ static const int mb_type_b_map[4]= {
MB_TYPE_L0 | MB_TYPE_16x16,
};

+static void init_tables(void)
+{
+ static int done = 0;
+ if (!done) {
+ done = 1;
+
+ ff_init_rl(&ff_mpeg4_rl_intra, ff_mpeg4_static_rl_table_store[0]);
+ ff_init_rl(&ff_rvlc_rl_inter, ff_mpeg4_static_rl_table_store[1]);
+ ff_init_rl(&ff_rvlc_rl_intra, ff_mpeg4_static_rl_table_store[2]);
+ INIT_VLC_RL(ff_mpeg4_rl_intra, 554);
+ INIT_VLC_RL(ff_rvlc_rl_inter, 1072);
+ INIT_VLC_RL(ff_rvlc_rl_intra, 1072);
+ INIT_VLC_STATIC(&dc_lum, DC_VLC_BITS, 10 /* 13 */,
+ &ff_mpeg4_DCtab_lum[0][1], 2, 1,
+ &ff_mpeg4_DCtab_lum[0][0], 2, 1, 512);
+ INIT_VLC_STATIC(&dc_chrom, DC_VLC_BITS, 10 /* 13 */,
+ &ff_mpeg4_DCtab_chrom[0][1], 2, 1,
+ &ff_mpeg4_DCtab_chrom[0][0], 2, 1, 512);
+ INIT_VLC_STATIC(&sprite_trajectory, SPRITE_TRAJ_VLC_BITS, 15,
+ &ff_sprite_trajectory_tab[0][1], 4, 2,
+ &ff_sprite_trajectory_tab[0][0], 4, 2, 128);
+ INIT_VLC_STATIC(&mb_type_b_vlc, MB_TYPE_B_VLC_BITS, 4,
+ &ff_mb_type_b_tab[0][1], 2, 1,
+ &ff_mb_type_b_tab[0][0], 2, 1, 16);
+ }
+}
+
/**
* Predict the ac.
* @param n block index (0-3 are luma, 4-5 are chroma)
@@ -2196,7 +2223,8 @@ static av_cold int decode_init(AVCodecContext *avctx)
{
MpegEncContext *s = avctx->priv_data;
int ret;
- static int done = 0;
+
+ init_tables();

s->divx_version=
s->divx_build=
@@ -2206,29 +2234,6 @@ static av_cold int decode_init(AVCodecContext *avctx)
if((ret=ff_h263_decode_init(avctx)) < 0)
return ret;

- if (!done) {
- done = 1;
-
- ff_init_rl(&ff_mpeg4_rl_intra, ff_mpeg4_static_rl_table_store[0]);
- ff_init_rl(&ff_rvlc_rl_inter, ff_mpeg4_static_rl_table_store[1]);
- ff_init_rl(&ff_rvlc_rl_intra, ff_mpeg4_static_rl_table_store[2]);
- INIT_VLC_RL(ff_mpeg4_rl_intra, 554);
- INIT_VLC_RL(ff_rvlc_rl_inter, 1072);
- INIT_VLC_RL(ff_rvlc_rl_intra, 1072);
- INIT_VLC_STATIC(&dc_lum, DC_VLC_BITS, 10 /* 13 */,
- &ff_mpeg4_DCtab_lum[0][1], 2, 1,
- &ff_mpeg4_DCtab_lum[0][0], 2, 1, 512);
- INIT_VLC_STATIC(&dc_chrom, DC_VLC_BITS, 10 /* 13 */,
- &ff_mpeg4_DCtab_chrom[0][1], 2, 1,
- &ff_mpeg4_DCtab_chrom[0][0], 2, 1, 512);
- INIT_VLC_STATIC(&sprite_trajectory, SPRITE_TRAJ_VLC_BITS, 15,
- &ff_sprite_trajectory_tab[0][1], 4, 2,
- &ff_sprite_trajectory_tab[0][0], 4, 2, 128);
- INIT_VLC_STATIC(&mb_type_b_vlc, MB_TYPE_B_VLC_BITS, 4,
- &ff_mb_type_b_tab[0][1], 2, 1,
- &ff_mb_type_b_tab[0][0], 2, 1, 16);
- }
-
s->h263_pred = 1;
s->low_delay = 0; //default, might be overriden in the vol header during header parsing
s->decode_mb= mpeg4_decode_mb;
--
1.7.10.4
Luca Barbato
2013-11-15 21:37:37 UTC
Permalink
On 15/11/13 22:13, Anton Khirnov wrote:
> ---
> libavcodec/mpeg4videodec.c | 53 ++++++++++++++++++++++++--------------------
> 1 file changed, 29 insertions(+), 24 deletions(-)
>

What about moving all of this in the .init_static_data when possible ?

lu
Anton Khirnov
2013-11-15 21:13:50 UTC
Permalink
Fixes invalid reads.
Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC:libav-***@libav.org
---
libavcodec/truemotion1.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/libavcodec/truemotion1.c b/libavcodec/truemotion1.c
index f7be92f..31d2265 100644
--- a/libavcodec/truemotion1.c
+++ b/libavcodec/truemotion1.c
@@ -513,6 +513,15 @@ hres,vres,i,i%vres (0 < i < 4)
index = s->index_stream[index_stream_index++] * 4; \
}

+#define INC_INDEX \
+do { \
+ if (index >= 1023) { \
+ av_log(s->avctx, AV_LOG_ERROR, "Invalid index value.\n"); \
+ return; \
+ } \
+ index++; \
+} while (0)
+
#define APPLY_C_PREDICTOR() \
predictor_pair = s->c_predictor_table[index]; \
horiz_pred += (predictor_pair >> 1); \
@@ -525,10 +534,10 @@ hres,vres,i,i%vres (0 < i < 4)
if (predictor_pair & 1) \
GET_NEXT_INDEX() \
else \
- index++; \
+ INC_INDEX; \
} \
} else \
- index++;
+ INC_INDEX;

#define APPLY_C_PREDICTOR_24() \
predictor_pair = s->c_predictor_table[index]; \
@@ -542,10 +551,10 @@ hres,vres,i,i%vres (0 < i < 4)
if (predictor_pair & 1) \
GET_NEXT_INDEX() \
else \
- index++; \
+ INC_INDEX; \
} \
} else \
- index++;
+ INC_INDEX;


#define APPLY_Y_PREDICTOR() \
@@ -560,10 +569,10 @@ hres,vres,i,i%vres (0 < i < 4)
if (predictor_pair & 1) \
GET_NEXT_INDEX() \
else \
- index++; \
+ INC_INDEX; \
} \
} else \
- index++;
+ INC_INDEX;

#define APPLY_Y_PREDICTOR_24() \
predictor_pair = s->y_predictor_table[index]; \
@@ -577,10 +586,10 @@ hres,vres,i,i%vres (0 < i < 4)
if (predictor_pair & 1) \
GET_NEXT_INDEX() \
else \
- index++; \
+ INC_INDEX; \
} \
} else \
- index++;
+ INC_INDEX;

#define OUTPUT_PIXEL_PAIR() \
*current_pixel_pair = *vert_pred + horiz_pred; \
--
1.7.10.4
Luca Barbato
2013-11-15 21:41:16 UTC
Permalink
On 15/11/13 22:13, Anton Khirnov wrote:
> Fixes invalid reads.
> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC:libav-***@libav.org
> ---
> libavcodec/truemotion1.c | 25 +++++++++++++++++--------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>

Given the code I won't ask you to return an error message or use
MACRO();-style.

lu
Anton Khirnov
2013-11-15 21:13:49 UTC
Permalink
Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC:libav-***@libav.org
---
libavformat/avidec.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/libavformat/avidec.c b/libavformat/avidec.c
index edc9c93..c71d545 100644
--- a/libavformat/avidec.c
+++ b/libavformat/avidec.c
@@ -484,6 +484,7 @@ static int avi_read_header(AVFormatContext *s)
ast = s->streams[0]->priv_data;
av_freep(&s->streams[0]->codec->extradata);
av_freep(&s->streams[0]->codec);
+ av_freep(&s->streams[0]->info);
av_freep(&s->streams[0]);
s->nb_streams = 0;
if (CONFIG_DV_DEMUXER) {
--
1.7.10.4
Luca Barbato
2013-11-15 21:39:41 UTC
Permalink
On 15/11/13 22:13, Anton Khirnov wrote:
> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC:libav-***@libav.org
> ---
> libavformat/avidec.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/libavformat/avidec.c b/libavformat/avidec.c
> index edc9c93..c71d545 100644
> --- a/libavformat/avidec.c
> +++ b/libavformat/avidec.c
> @@ -484,6 +484,7 @@ static int avi_read_header(AVFormatContext *s)
> ast = s->streams[0]->priv_data;
> av_freep(&s->streams[0]->codec->extradata);
> av_freep(&s->streams[0]->codec);
> + av_freep(&s->streams[0]->info);
> av_freep(&s->streams[0]);
> s->nb_streams = 0;
> if (CONFIG_DV_DEMUXER) {
>

Ok.
Anton Khirnov
2013-11-15 21:13:40 UTC
Permalink
Also add an error message an return a more suitable error code
(INVALIDDATA, not EINVAL);
Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC:libav-***@libav.org
---
libavcodec/gifdec.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/libavcodec/gifdec.c b/libavcodec/gifdec.c
index 136d112..b1207ae 100644
--- a/libavcodec/gifdec.c
+++ b/libavcodec/gifdec.c
@@ -87,8 +87,11 @@ static int gif_read_image(GifState *s, AVFrame *frame)

/* verify that all the image is inside the screen dimensions */
if (left + width > s->screen_width ||
- top + height > s->screen_height)
- return AVERROR(EINVAL);
+ top + height > s->screen_height ||
+ !width || !height) {
+ av_log(s->avctx, AV_LOG_ERROR, "Invalid image dimensions.\n");
+ return AVERROR_INVALIDDATA;
+ }

/* build the palette */
n = (1 << bits_per_pixel);
--
1.7.10.4
Luca Barbato
2013-11-15 21:42:00 UTC
Permalink
On 15/11/13 22:13, Anton Khirnov wrote:


Ok.
Anton Khirnov
2013-11-15 21:13:51 UTC
Permalink
Fixes invalid reads.
Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC:libav-***@libav.org
---
libavcodec/pcx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libavcodec/pcx.c b/libavcodec/pcx.c
index 1a5357b..61c971e 100644
--- a/libavcodec/pcx.c
+++ b/libavcodec/pcx.c
@@ -109,7 +109,7 @@ static int pcx_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
nplanes = buf[65];
bytes_per_scanline = nplanes * bytes_per_line;

- if (bytes_per_scanline < w * bits_per_pixel * nplanes / 8 ||
+ if (bytes_per_scanline < (w * bits_per_pixel * nplanes + 7) / 8 ||
(!compressed && bytes_per_scanline > buf_size / h)) {
av_log(avctx, AV_LOG_ERROR, "PCX data is corrupted\n");
return AVERROR_INVALIDDATA;
--
1.7.10.4
Luca Barbato
2013-11-15 21:34:09 UTC
Permalink
On 15/11/13 22:13, Anton Khirnov wrote:
> Fixes invalid reads.
> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC:libav-***@libav.org
> ---
> libavcodec/pcx.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>

Maybe is worthy adding an utility function. Looks ok.
Anton Khirnov
2013-11-15 21:13:41 UTC
Permalink
---
libavcodec/gifdec.c | 67 +++++++++++++++++++++++++--------------------------
libavcodec/lzw.c | 7 +++---
libavcodec/lzw.h | 2 +-
3 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/libavcodec/gifdec.c b/libavcodec/gifdec.c
index b1207ae..cdb7f23 100644
--- a/libavcodec/gifdec.c
+++ b/libavcodec/gifdec.c
@@ -46,8 +46,7 @@ typedef struct GifState {
int gce_delay;

/* LZW compatible decoder */
- const uint8_t *bytestream;
- const uint8_t *bytestream_end;
+ GetByteContext gb;
LZWState *lzw;

/* aux buffers */
@@ -66,11 +65,11 @@ static int gif_read_image(GifState *s, AVFrame *frame)
int is_interleaved, has_local_palette, y, pass, y1, linesize, n, i;
uint8_t *ptr, *spal, *palette, *ptr1;

- left = bytestream_get_le16(&s->bytestream);
- top = bytestream_get_le16(&s->bytestream);
- width = bytestream_get_le16(&s->bytestream);
- height = bytestream_get_le16(&s->bytestream);
- flags = bytestream_get_byte(&s->bytestream);
+ left = bytestream2_get_le16(&s->gb);
+ top = bytestream2_get_le16(&s->gb);
+ width = bytestream2_get_le16(&s->gb);
+ height = bytestream2_get_le16(&s->gb);
+ flags = bytestream2_get_byte(&s->gb);
is_interleaved = flags & 0x40;
has_local_palette = flags & 0x80;
bits_per_pixel = (flags & 0x07) + 1;
@@ -78,7 +77,7 @@ static int gif_read_image(GifState *s, AVFrame *frame)
av_dlog(s->avctx, "gif: image x=%d y=%d w=%d h=%d\n", left, top, width, height);

if (has_local_palette) {
- bytestream_get_buffer(&s->bytestream, s->local_palette, 3 * (1 << bits_per_pixel));
+ bytestream2_get_buffer(&s->gb, s->local_palette, 3 * (1 << bits_per_pixel));
palette = s->local_palette;
} else {
palette = s->global_palette;
@@ -107,9 +106,9 @@ static int gif_read_image(GifState *s, AVFrame *frame)
s->image_palette[s->transparent_color_index] = 0;

/* now get the image data */
- code_size = bytestream_get_byte(&s->bytestream);
- ff_lzw_decode_init(s->lzw, code_size, s->bytestream,
- s->bytestream_end - s->bytestream, FF_LZW_GIF);
+ code_size = bytestream2_get_byte(&s->gb);
+ ff_lzw_decode_init(s->lzw, code_size, s->gb.buffer,
+ bytestream2_get_bytes_left(&s->gb), FF_LZW_GIF);

/* read all the image */
linesize = frame->linesize[0];
@@ -152,7 +151,8 @@ static int gif_read_image(GifState *s, AVFrame *frame)
}
/* read the garbage data until end marker is found */
ff_lzw_decode_tail(s->lzw);
- s->bytestream = ff_lzw_cur_ptr(s->lzw);
+
+ bytestream2_skip(&s->gb, ff_lzw_size_read(s->lzw));
return 0;
}

@@ -161,8 +161,8 @@ static int gif_read_extension(GifState *s)
int ext_code, ext_len, i, gce_flags, gce_transparent_index;

/* extension */
- ext_code = bytestream_get_byte(&s->bytestream);
- ext_len = bytestream_get_byte(&s->bytestream);
+ ext_code = bytestream2_get_byte(&s->gb);
+ ext_len = bytestream2_get_byte(&s->gb);

av_dlog(s->avctx, "gif: ext_code=0x%x len=%d\n", ext_code, ext_len);

@@ -171,9 +171,9 @@ static int gif_read_extension(GifState *s)
if (ext_len != 4)
goto discard_ext;
s->transparent_color_index = -1;
- gce_flags = bytestream_get_byte(&s->bytestream);
- s->gce_delay = bytestream_get_le16(&s->bytestream);
- gce_transparent_index = bytestream_get_byte(&s->bytestream);
+ gce_flags = bytestream2_get_byte(&s->gb);
+ s->gce_delay = bytestream2_get_le16(&s->gb);
+ gce_transparent_index = bytestream2_get_byte(&s->gb);
if (gce_flags & 0x01)
s->transparent_color_index = gce_transparent_index;
else
@@ -184,7 +184,7 @@ static int gif_read_extension(GifState *s)
gce_flags, s->gce_delay,
s->transparent_color_index, s->gce_disposal);

- ext_len = bytestream_get_byte(&s->bytestream);
+ ext_len = bytestream2_get_byte(&s->gb);
break;
}

@@ -192,8 +192,8 @@ static int gif_read_extension(GifState *s)
discard_ext:
while (ext_len != 0) {
for (i = 0; i < ext_len; i++)
- bytestream_get_byte(&s->bytestream);
- ext_len = bytestream_get_byte(&s->bytestream);
+ bytestream2_get_byte(&s->gb);
+ ext_len = bytestream2_get_byte(&s->gb);

av_dlog(s->avctx, "gif: ext_len1=%d\n", ext_len);
}
@@ -206,31 +206,31 @@ static int gif_read_header1(GifState *s)
int v, n;
int has_global_palette;

- if (s->bytestream_end < s->bytestream + 13)
+ if (bytestream2_get_bytes_left(&s->gb) < 13)
return AVERROR_INVALIDDATA;

/* read gif signature */
- bytestream_get_buffer(&s->bytestream, sig, 6);
+ bytestream2_get_buffer(&s->gb, sig, 6);
if (memcmp(sig, gif87a_sig, 6) != 0 &&
memcmp(sig, gif89a_sig, 6) != 0)
return AVERROR_INVALIDDATA;

/* read screen header */
s->transparent_color_index = -1;
- s->screen_width = bytestream_get_le16(&s->bytestream);
- s->screen_height = bytestream_get_le16(&s->bytestream);
+ s->screen_width = bytestream2_get_le16(&s->gb);
+ s->screen_height = bytestream2_get_le16(&s->gb);
if( (unsigned)s->screen_width > 32767
|| (unsigned)s->screen_height > 32767){
av_log(NULL, AV_LOG_ERROR, "picture size too large\n");
return AVERROR_INVALIDDATA;
}

- v = bytestream_get_byte(&s->bytestream);
+ v = bytestream2_get_byte(&s->gb);
s->color_resolution = ((v & 0x70) >> 4) + 1;
has_global_palette = (v & 0x80);
s->bits_per_pixel = (v & 0x07) + 1;
- s->background_color_index = bytestream_get_byte(&s->bytestream);
- bytestream_get_byte(&s->bytestream); /* ignored */
+ s->background_color_index = bytestream2_get_byte(&s->gb);
+ bytestream2_get_byte(&s->gb); /* ignored */

av_dlog(s->avctx, "gif: screen_w=%d screen_h=%d bpp=%d global_palette=%d\n",
s->screen_width, s->screen_height, s->bits_per_pixel,
@@ -238,17 +238,17 @@ static int gif_read_header1(GifState *s)

if (has_global_palette) {
n = 1 << s->bits_per_pixel;
- if (s->bytestream_end < s->bytestream + n * 3)
+ if (bytestream2_get_bytes_left(&s->gb) < n * 3)
return AVERROR_INVALIDDATA;
- bytestream_get_buffer(&s->bytestream, s->global_palette, n * 3);
+ bytestream2_get_buffer(&s->gb, s->global_palette, n * 3);
}
return 0;
}

static int gif_parse_next_image(GifState *s, AVFrame *frame)
{
- while (s->bytestream < s->bytestream_end) {
- int code = bytestream_get_byte(&s->bytestream);
+ while (bytestream2_get_bytes_left(&s->gb) > 0) {
+ int code = bytestream2_get_byte(&s->gb);
int ret;

av_dlog(s->avctx, "gif: code=%02x '%c'\n", code, code);
@@ -289,8 +289,7 @@ static int gif_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
AVFrame *picture = data;
int ret;

- s->bytestream = buf;
- s->bytestream_end = buf + buf_size;
+ bytestream2_init(&s->gb, buf, buf_size);
if ((ret = gif_read_header1(s)) < 0)
return ret;

@@ -309,7 +308,7 @@ static int gif_decode_frame(AVCodecContext *avctx, void *data, int *got_frame,
return ret;

*got_frame = 1;
- return s->bytestream - buf;
+ return bytestream2_tell(&s->gb);
}

static av_cold int gif_decode_close(AVCodecContext *avctx)
diff --git a/libavcodec/lzw.c b/libavcodec/lzw.c
index 2c99014..0167140 100644
--- a/libavcodec/lzw.c
+++ b/libavcodec/lzw.c
@@ -43,7 +43,7 @@ static const uint16_t mask[17] =
};

struct LZWState {
- const uint8_t *pbuf, *ebuf;
+ const uint8_t *buf_start, *pbuf, *ebuf;
int bbits;
unsigned int bbuf;

@@ -92,9 +92,10 @@ static int lzw_get_code(struct LZWState * s)
return c & s->curmask;
}

-const uint8_t* ff_lzw_cur_ptr(LZWState *p)
+int ff_lzw_size_read(LZWState *p)
{
- return ((struct LZWState*)p)->pbuf;
+ struct LZWState *s = p;
+ return s->pbuf - s->buf_start;
}

void ff_lzw_decode_tail(LZWState *p)
diff --git a/libavcodec/lzw.h b/libavcodec/lzw.h
index ab782f5..d925d35 100644
--- a/libavcodec/lzw.h
+++ b/libavcodec/lzw.h
@@ -47,7 +47,7 @@ void ff_lzw_decode_open(LZWState **p);
void ff_lzw_decode_close(LZWState **p);
int ff_lzw_decode_init(LZWState *s, int csize, const uint8_t *buf, int buf_size, int mode);
int ff_lzw_decode(LZWState *s, uint8_t *buf, int len);
-const uint8_t* ff_lzw_cur_ptr(LZWState *lzw);
+int ff_lzw_size_read(LZWState *lzw);
void ff_lzw_decode_tail(LZWState *lzw);

/** LZW encode state */
--
1.7.10.4
Luca Barbato
2013-11-15 21:32:09 UTC
Permalink
On 15/11/13 22:13, Anton Khirnov wrote:
> ---
> libavcodec/gifdec.c | 67 +++++++++++++++++++++++++--------------------------
> libavcodec/lzw.c | 7 +++---
> libavcodec/lzw.h | 2 +-
> 3 files changed, 38 insertions(+), 38 deletions(-)
>

Write down also the lzw changes in the commit message, beside that looks ok.

lu
Anton Khirnov
2013-11-15 21:13:43 UTC
Permalink
It might be passed to code requiring padding, such as lzo decompression.

Fixes invalid reads.
Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
CC:libav-***@libav.org
---
libavformat/matroskadec.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/libavformat/matroskadec.c b/libavformat/matroskadec.c
index f798342..764dbf8 100644
--- a/libavformat/matroskadec.c
+++ b/libavformat/matroskadec.c
@@ -734,9 +734,11 @@ static int ebml_read_ascii(AVIOContext *pb, int size, char **str)
static int ebml_read_binary(AVIOContext *pb, int length, EbmlBin *bin)
{
av_free(bin->data);
- if (!(bin->data = av_malloc(length)))
+ if (!(bin->data = av_malloc(length + FF_INPUT_BUFFER_PADDING_SIZE)))
return AVERROR(ENOMEM);

+ memset(bin->data + length, 0, FF_INPUT_BUFFER_PADDING_SIZE);
+
bin->size = length;
bin->pos = avio_tell(pb);
if (avio_read(pb, bin->data, length) != length) {
--
1.7.10.4
Luca Barbato
2013-11-15 21:38:12 UTC
Permalink
On 15/11/13 22:13, Anton Khirnov wrote:
> It might be passed to code requiring padding, such as lzo decompression.
>

Looks fine.
Anton Khirnov
2013-11-16 08:26:18 UTC
Permalink
On Fri, 15 Nov 2013 23:20:22 +0200 (EET), Martin Storsjö <***@martin.st> wrote:
> On Fri, 15 Nov 2013, Anton Khirnov wrote:
>
> > They are used when decoding the frame header.
> > Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> > CC:libav-***@libav.org
> > ---
> > libavcodec/mpeg4video.h | 2 ++
> > libavcodec/mpeg4video_parser.c | 2 ++
> > libavcodec/mpeg4videodec.c | 4 ++--
> > 3 files changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/libavcodec/mpeg4video.h b/libavcodec/mpeg4video.h
> > index c22fbfd..d7157d3 100644
> > --- a/libavcodec/mpeg4video.h
> > +++ b/libavcodec/mpeg4video.h
> > @@ -112,6 +112,8 @@ void ff_mpeg4_init_direct_mv(MpegEncContext *s);
> > */
> > int ff_mpeg4_set_direct_mv(MpegEncContext *s, int mx, int my);
> >
> > +void ff_mpeg4_init_tables(void);
> > +
> > extern uint8_t ff_mpeg4_static_rl_table_store[3][2][2 * MAX_RUN + MAX_LEVEL + 3];
> >
> > #if 0 //3IV1 is quite rare and it slows things down a tiny bit
> > diff --git a/libavcodec/mpeg4video_parser.c b/libavcodec/mpeg4video_parser.c
> > index a8def0e..dfddeda 100644
> > --- a/libavcodec/mpeg4video_parser.c
> > +++ b/libavcodec/mpeg4video_parser.c
> > @@ -105,6 +105,8 @@ static av_cold int mpeg4video_parse_init(AVCodecParserContext *s)
> > {
> > struct Mp4vParseContext *pc = s->priv_data;
> >
> > + ff_mpeg4_init_tables();
> > +
>
> Contrary to the normal codec init function, this can get called from any
> thread outside of the normal avcodec init lock. OTOH, if the static init
> code is safe to be run in parallel from multiple threads I guess it isn't
> an issue, but it's worth noting.
>

Well it's not really safe, as there's no lock around it.

Perhaps we could move those tables in the context instead. At least the
sprite_trajectory ones, which are the object of this patch, as they don't look
very large.
(though really I'd do it for all the tables inited at runtime - a few kb per
each additional context isn't worth the problems with global state)

--
Anton Khirnov
Luca Barbato
2013-11-16 11:49:32 UTC
Permalink
On 15/11/13 22:13, Anton Khirnov wrote:
> Fixes invalid reads.
> Found-by: Mateusz "j00ru" Jurczyk and Gynvael Coldwind
> CC:libav-***@libav.org
> ---
> libavcodec/h264_cavlc.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/libavcodec/h264_cavlc.c b/libavcodec/h264_cavlc.c
> index 5ed1d5d..d3f6dcb 100644
> --- a/libavcodec/h264_cavlc.c
> +++ b/libavcodec/h264_cavlc.c
> @@ -765,6 +765,10 @@ decode_intra_mb:
>
> // We assume these blocks are very rare so we do not optimize it.
> h->intra_pcm_ptr = align_get_bits(&h->gb);
> + if (get_bits_left(&h->gb) < mb_size) {
> + av_log(h->avctx, AV_LOG_ERROR, "Not enough data for an intra PCM block.\n");
> + return AVERROR_INVALIDDATA;
> + }
> skip_bits_long(&h->gb, mb_size);
>
> // In deblocking, the quantizer is 0
>

Ok.
Anton Khirnov
2013-11-21 19:57:05 UTC
Permalink
On Fri, 15 Nov 2013 22:37:37 +0100, Luca Barbato <***@gentoo.org> wrote:
> On 15/11/13 22:13, Anton Khirnov wrote:
> > ---
> > libavcodec/mpeg4videodec.c | 53 ++++++++++++++++++++++++--------------------
> > 1 file changed, 29 insertions(+), 24 deletions(-)
> >
>
> What about moving all of this in the .init_static_data when possible ?
>

init_static_data is called on registering, so it's really not suitable for this

--
Anton Khirnov
Continue reading on narkive:
Loading...