Message ID | 1490968192-55497-1-git-send-email-rsbultje@gmail.com |
---|---|
State | Superseded |
Headers | show |
On Fri, Mar 31, 2017 at 09:49:52AM -0400, Ronald S. Bultje wrote: > Fixes a reported (but false) race condition in tsan for fate-apng. > --- > libavcodec/png.h | 5 ---- > libavcodec/pngdec.c | 68 +++++++++++++++++++++++++++++++---------------------- > 2 files changed, 40 insertions(+), 33 deletions(-) > this causes a segfault ill send you the sample privatly ==29980== Thread 10: ==29980== Invalid write of size 8 ==29980== at 0x4C2E164: memcpy@@GLIBC_2.14 (mc_replace_strmem.c:877) ==29980== by 0xBBF4AC: png_filter_row (pngdec.c:258) ==29980== by 0xBBFFCD: png_handle_row (pngdec.c:335) ==29980== by 0xBC059A: png_decode_idat (pngdec.c:420) ==29980== by 0xBC1F6A: decode_idat_chunk (pngdec.c:754) ==29980== by 0xBC4525: decode_frame_common (pngdec.c:1204) ==29980== by 0xBC50CE: decode_frame_png (pngdec.c:1357) ==29980== by 0xBD932D: frame_worker_thread (pthread_frame.c:199) ==29980== by 0x777BE99: start_thread (pthread_create.c:308) ==29980== by 0x7A872EC: clone (clone.S:112) ==29980== Address 0xfb01288 is 20,520 bytes inside a block of size 20,527 alloc'd ==29980== at 0x4C2A6C5: memalign (vg_replace_malloc.c:727) ==29980== by 0x4C2A760: posix_memalign (vg_replace_malloc.c:876) ==29980== by 0x15AE5B7: av_malloc (mem.c:87) ==29980== by 0x159A0F1: av_buffer_alloc (buffer.c:72) ==29980== by 0x159A156: av_buffer_allocz (buffer.c:85) ==29980== by 0x159A856: pool_alloc_buffer (buffer.c:312) ==29980== by 0x159A984: av_buffer_pool_get (buffer.c:349) ==29980== by 0xCED638: video_get_buffer (utils.c:682) ==29980== by 0xCED9A6: avcodec_default_get_buffer2 (utils.c:740) ==29980== by 0x42A868: get_buffer (ffmpeg.c:2858) ==29980== by 0xCEE255: get_buffer_internal (utils.c:941) ==29980== by 0xCEE2D7: ff_get_buffer (utils.c:956) ==29980== ==29980== Invalid write of size 8 ==29980== at 0x13833B9: ff_add_bytes_l2_sse2 (pngdsp.asm:90) ==29980== by 0xBBFFCD: png_handle_row (pngdec.c:335) ==29980== by 0xBC059A: png_decode_idat (pngdec.c:420) ==29980== by 0xBC1F6A: decode_idat_chunk (pngdec.c:754) ==29980== by 0xBC4525: decode_frame_common (pngdec.c:1204) ==29980== by 0xBC50CE: decode_frame_png (pngdec.c:1357) ==29980== by 0xBD932D: frame_worker_thread (pthread_frame.c:199) ==29980== by 0x777BE99: start_thread (pthread_create.c:308) ==29980== by 0x7A872EC: clone (clone.S:112) ==29980== Address 0xfb01808 is 1,064 bytes inside a block of size 1,071 alloc'd ==29980== at 0x4C2A6C5: memalign (vg_replace_malloc.c:727) ==29980== by 0x4C2A760: posix_memalign (vg_replace_malloc.c:876) ==29980== by 0x15AE5B7: av_malloc (mem.c:87) ==29980== by 0x159A0F1: av_buffer_alloc (buffer.c:72) ==29980== by 0x159A156: av_buffer_allocz (buffer.c:85) ==29980== by 0x159A856: pool_alloc_buffer (buffer.c:312) ==29980== by 0x159A984: av_buffer_pool_get (buffer.c:349) ==29980== by 0xCED638: video_get_buffer (utils.c:682) ==29980== by 0xCED9A6: avcodec_default_get_buffer2 (utils.c:740) ==29980== by 0x42A868: get_buffer (ffmpeg.c:2858) ==29980== by 0xCEE255: get_buffer_internal (utils.c:941) ==29980== by 0xCEE2D7: ff_get_buffer (utils.c:956) ==29980== ==29980== Invalid write of size 8 ==29980== at 0x13833BF: ff_add_bytes_l2_sse2 (pngdsp.asm:90) ==29980== by 0xBBFFCD: png_handle_row (pngdec.c:335) ==29980== by 0xBC059A: png_decode_idat (pngdec.c:420) ==29980== by 0xBC1F6A: decode_idat_chunk (pngdec.c:754) ==29980== by 0xBC4525: decode_frame_common (pngdec.c:1204) ==29980== by 0xBC50CE: decode_frame_png (pngdec.c:1357) ==29980== by 0xBD932D: frame_worker_thread (pthread_frame.c:199) ==29980== by 0x777BE99: start_thread (pthread_create.c:308) ==29980== by 0x7A872EC: clone (clone.S:112) ==29980== Address 0xfb01810 is 1 bytes after a block of size 1,071 alloc'd ==29980== at 0x4C2A6C5: memalign (vg_replace_malloc.c:727) ==29980== by 0x4C2A760: posix_memalign (vg_replace_malloc.c:876) ==29980== by 0x15AE5B7: av_malloc (mem.c:87) ==29980== by 0x159A0F1: av_buffer_alloc (buffer.c:72) ==29980== by 0x159A156: av_buffer_allocz (buffer.c:85) ==29980== by 0x159A856: pool_alloc_buffer (buffer.c:312) ==29980== by 0x159A984: av_buffer_pool_get (buffer.c:349) ==29980== by 0xCED638: video_get_buffer (utils.c:682) ==29980== by 0xCED9A6: avcodec_default_get_buffer2 (utils.c:740) ==29980== by 0x42A868: get_buffer (ffmpeg.c:2858) ==29980== by 0xCEE255: get_buffer_internal (utils.c:941) ==29980== by 0xCEE2D7: ff_get_buffer (utils.c:956) ==29980== ==29980== Invalid write of size 1 ==29980== at 0xBBF877: png_filter_row (pngdec.c:281) ==29980== by 0xBBFFCD: png_handle_row (pngdec.c:335) ==29980== by 0xBC059A: png_decode_idat (pngdec.c:420) ==29980== by 0xBC1F6A: decode_idat_chunk (pngdec.c:754) ==29980== by 0xBC4525: decode_frame_common (pngdec.c:1204) ==29980== by 0xBC50CE: decode_frame_png (pngdec.c:1357) ==29980== by 0xBD932D: frame_worker_thread (pthread_frame.c:199) ==29980== by 0x777BE99: start_thread (pthread_create.c:308) ==29980== by 0x7A872EC: clone (clone.S:112) ==29980== Address 0xfb018a0 is not stack'd, malloc'd or (recently) free'd ==29980== ==29980== Invalid read of size 1 ==29980== at 0xBBF88F: png_filter_row (pngdec.c:284) ==29980== by 0xBBFFCD: png_handle_row (pngdec.c:335) ==29980== by 0xBC059A: png_decode_idat (pngdec.c:420) ==29980== by 0xBC1F6A: decode_idat_chunk (pngdec.c:754) ==29980== by 0xBC4525: decode_frame_common (pngdec.c:1204) ==29980== by 0xBC50CE: decode_frame_png (pngdec.c:1357) ==29980== by 0xBD932D: frame_worker_thread (pthread_frame.c:199) ==29980== by 0x777BE99: start_thread (pthread_create.c:308) ==29980== by 0x7A872EC: clone (clone.S:112) ==29980== Address 0xfb018a0 is not stack'd, malloc'd or (recently) free'd ==29980== ==29980== Invalid write of size 1 ==29980== at 0xBBF8D8: png_filter_row (pngdec.c:284) ==29980== by 0xBBFFCD: png_handle_row (pngdec.c:335) ==29980== by 0xBC059A: png_decode_idat (pngdec.c:420) ==29980== by 0xBC1F6A: decode_idat_chunk (pngdec.c:754) ==29980== by 0xBC4525: decode_frame_common (pngdec.c:1204) ==29980== by 0xBC50CE: decode_frame_png (pngdec.c:1357) ==29980== by 0xBD932D: frame_worker_thread (pthread_frame.c:199) ==29980== by 0x777BE99: start_thread (pthread_create.c:308) ==29980== by 0x7A872EC: clone (clone.S:112) ==29980== Address 0xfb018a1 is not stack'd, malloc'd or (recently) free'd ==29980== ==29980== Invalid read of size 1 ==29980== at 0xBBF8AD: png_filter_row (pngdec.c:284) ==29980== by 0xBBFFCD: png_handle_row (pngdec.c:335) ==29980== by 0xBC059A: png_decode_idat (pngdec.c:420) ==29980== by 0xBC1F6A: decode_idat_chunk (pngdec.c:754) ==29980== by 0xBC4525: decode_frame_common (pngdec.c:1204) ==29980== by 0xBC50CE: decode_frame_png (pngdec.c:1357) ==29980== by 0xBD932D: frame_worker_thread (pthread_frame.c:199) ==29980== by 0x777BE99: start_thread (pthread_create.c:308) ==29980== by 0x7A872EC: clone (clone.S:112) ==29980== Address 0xfb0180f is 0 bytes after a block of size 1,071 alloc'd ==29980== at 0x4C2A6C5: memalign (vg_replace_malloc.c:727) ==29980== by 0x4C2A760: posix_memalign (vg_replace_malloc.c:876) ==29980== by 0x15AE5B7: av_malloc (mem.c:87) ==29980== by 0x159A0F1: av_buffer_alloc (buffer.c:72) ==29980== by 0x159A156: av_buffer_allocz (buffer.c:85) ==29980== by 0x159A856: pool_alloc_buffer (buffer.c:312) ==29980== by 0x159A984: av_buffer_pool_get (buffer.c:349) ==29980== by 0xCED638: video_get_buffer (utils.c:682) ==29980== by 0xCED9A6: avcodec_default_get_buffer2 (utils.c:740) ==29980== by 0x42A868: get_buffer (ffmpeg.c:2858) ==29980== by 0xCEE255: get_buffer_internal (utils.c:941) ==29980== by 0xCEE2D7: ff_get_buffer (utils.c:956) ==29980== ==29980== Invalid read of size 4 ==29980== at 0x159A1DB: av_buffer_ref (buffer.c:102) ==29980== by 0x15A36C3: av_frame_ref (frame.c:423) ==29980== by 0xBC5140: decode_frame_png (pngdec.c:1366) ==29980== by 0xBD932D: frame_worker_thread (pthread_frame.c:199) ==29980== by 0x777BE99: start_thread (pthread_create.c:308) ==29980== by 0x7A872EC: clone (clone.S:112) ==29980== Address 0xb9b0aa926b509e76 is not stack'd, malloc'd or (recently) free'd ==29980== ==29980== ==29980== Process terminating with default action of signal 11 (SIGSEGV) ==29980== General Protection Fault ==29980== at 0x159A1DB: av_buffer_ref (buffer.c:102) ==29980== by 0x15A36C3: av_frame_ref (frame.c:423) ==29980== by 0xBC5140: decode_frame_png (pngdec.c:1366) ==29980== by 0xBD932D: frame_worker_thread (pthread_frame.c:199) ==29980== by 0x777BE99: start_thread (pthread_create.c:308) ==29980== by 0x7A872EC: clone (clone.S:112) [...]
diff --git a/libavcodec/png.h b/libavcodec/png.h index 948c2f7..e967fcf 100644 --- a/libavcodec/png.h +++ b/libavcodec/png.h @@ -42,11 +42,6 @@ #define PNG_FILTER_VALUE_PAETH 4 #define PNG_FILTER_VALUE_MIXED 5 -#define PNG_IHDR 0x0001 -#define PNG_IDAT 0x0002 -#define PNG_ALLIMAGE 0x0004 -#define PNG_PLTE 0x0008 - #define NB_PASSES 7 #define PNGSIG 0x89504e470d0a1a0a diff --git a/libavcodec/pngdec.c b/libavcodec/pngdec.c index c08665b..80645f2 100644 --- a/libavcodec/pngdec.c +++ b/libavcodec/pngdec.c @@ -36,6 +36,17 @@ #include <zlib.h> +enum PNGHeaderState { + PNG_IHDR = 1 << 0, + PNG_PLTE = 1 << 1, +}; +enum PNGImageState { + PNG_IMG_NONE, + PNG_IMG_ALL, + PNG_IMG_IDAT, +}; + + typedef struct PNGDecContext { PNGDSPContext dsp; AVCodecContext *avctx; @@ -45,7 +56,8 @@ typedef struct PNGDecContext { ThreadFrame last_picture; ThreadFrame picture; - int state; + enum PNGHeaderState hdr_state; + enum PNGImageState pic_state; int width, height; int cur_w, cur_h; int last_w, last_h; @@ -334,7 +346,7 @@ static void png_handle_row(PNGDecContext *s) } s->y++; if (s->y == s->cur_h) { - s->state |= PNG_ALLIMAGE; + s->pic_state = PNG_IMG_ALL; if (s->filter_type == PNG_FILTER_TYPE_LOCO) { if (s->bit_depth == 16) { deloco_rgb16((uint16_t *)ptr, s->row_size / 2, @@ -369,7 +381,7 @@ static void png_handle_row(PNGDecContext *s) memset(s->last_row, 0, s->row_size); for (;;) { if (s->pass == NB_PASSES - 1) { - s->state |= PNG_ALLIMAGE; + s->pic_state = PNG_IMG_ALL; goto the_end; } else { s->pass++; @@ -404,7 +416,7 @@ static int png_decode_idat(PNGDecContext *s, int length) return AVERROR_EXTERNAL; } if (s->zstream.avail_out == 0) { - if (!(s->state & PNG_ALLIMAGE)) { + if (s->pic_state != PNG_IMG_ALL) { png_handle_row(s); } s->zstream.avail_out = s->crow_size; @@ -541,12 +553,12 @@ static int decode_ihdr_chunk(AVCodecContext *avctx, PNGDecContext *s, if (length != 13) return AVERROR_INVALIDDATA; - if (s->state & PNG_IDAT) { + if (s->pic_state != PNG_IMG_NONE) { av_log(avctx, AV_LOG_ERROR, "IHDR after IDAT\n"); return AVERROR_INVALIDDATA; } - if (s->state & PNG_IHDR) { + if (s->hdr_state & PNG_IHDR) { av_log(avctx, AV_LOG_ERROR, "Multiple IHDR\n"); return AVERROR_INVALIDDATA; } @@ -569,7 +581,7 @@ static int decode_ihdr_chunk(AVCodecContext *avctx, PNGDecContext *s, s->filter_type = bytestream2_get_byte(&s->gb); s->interlace_type = bytestream2_get_byte(&s->gb); bytestream2_skip(&s->gb, 4); /* crc */ - s->state |= PNG_IHDR; + s->hdr_state |= PNG_IHDR; if (avctx->debug & FF_DEBUG_PICT_INFO) av_log(avctx, AV_LOG_DEBUG, "width=%d height=%d depth=%d color_type=%d " "compression_type=%d filter_type=%d interlace_type=%d\n", @@ -585,7 +597,7 @@ error: static int decode_phys_chunk(AVCodecContext *avctx, PNGDecContext *s) { - if (s->state & PNG_IDAT) { + if (s->pic_state != PNG_IMG_NONE) { av_log(avctx, AV_LOG_ERROR, "pHYs after IDAT\n"); return AVERROR_INVALIDDATA; } @@ -605,11 +617,11 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s, int ret; size_t byte_depth = s->bit_depth > 8 ? 2 : 1; - if (!(s->state & PNG_IHDR)) { + if (!(s->hdr_state & PNG_IHDR)) { av_log(avctx, AV_LOG_ERROR, "IDAT without IHDR\n"); return AVERROR_INVALIDDATA; } - if (!(s->state & PNG_IDAT)) { + if (s->pic_state == PNG_IMG_NONE) { /* init image info */ avctx->width = s->width; avctx->height = s->height; @@ -690,11 +702,10 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s, if ((ret = ff_thread_get_buffer(avctx, &s->previous_picture, AV_GET_BUFFER_FLAG_REF)) < 0) return ret; } - ff_thread_finish_setup(avctx); - p->pict_type = AV_PICTURE_TYPE_I; p->key_frame = 1; p->interlaced_frame = !!s->interlace_type; + ff_thread_finish_setup(avctx); /* compute the compressed row size */ if (!s->interlace_type) { @@ -734,7 +745,7 @@ static int decode_idat_chunk(AVCodecContext *avctx, PNGDecContext *s, s->zstream.next_out = s->crow_buf; } - s->state |= PNG_IDAT; + s->pic_state = PNG_IMG_IDAT; /* set image to non-transparent bpp while decompressing */ if (s->has_trns && s->color_type != PNG_COLOR_TYPE_PALETTE) @@ -770,7 +781,7 @@ static int decode_plte_chunk(AVCodecContext *avctx, PNGDecContext *s, } for (; i < 256; i++) s->palette[i] = (0xFFU << 24); - s->state |= PNG_PLTE; + s->hdr_state |= PNG_PLTE; bytestream2_skip(&s->gb, 4); /* crc */ return 0; @@ -781,18 +792,18 @@ static int decode_trns_chunk(AVCodecContext *avctx, PNGDecContext *s, { int v, i; - if (!(s->state & PNG_IHDR)) { + if (!(s->hdr_state & PNG_IHDR)) { av_log(avctx, AV_LOG_ERROR, "trns before IHDR\n"); return AVERROR_INVALIDDATA; } - if (s->state & PNG_IDAT) { + if (s->pic_state != PNG_IMG_NONE) { av_log(avctx, AV_LOG_ERROR, "trns after IDAT\n"); return AVERROR_INVALIDDATA; } if (s->color_type == PNG_COLOR_TYPE_PALETTE) { - if (length > 256 || !(s->state & PNG_PLTE)) + if (length > 256 || !(s->hdr_state & PNG_PLTE)) return AVERROR_INVALIDDATA; for (i = 0; i < length; i++) { @@ -906,7 +917,7 @@ static int decode_fctl_chunk(AVCodecContext *avctx, PNGDecContext *s, if (length != 26) return AVERROR_INVALIDDATA; - if (!(s->state & PNG_IHDR)) { + if (!(s->hdr_state & PNG_IHDR)) { av_log(avctx, AV_LOG_ERROR, "fctl before IHDR\n"); return AVERROR_INVALIDDATA; } @@ -1122,13 +1133,13 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s, } if (CONFIG_APNG_DECODER && avctx->codec_id == AV_CODEC_ID_APNG && length == 0) { - if (!(s->state & PNG_IDAT)) + if (s->pic_state == PNG_IMG_NONE) return 0; else goto exit_loop; } av_log(avctx, AV_LOG_ERROR, "%d bytes left\n", length); - if ( s->state & PNG_ALLIMAGE + if ( s->pic_state == PNG_IMG_ALL && avctx->strict_std_compliance <= FF_COMPLIANCE_NORMAL) goto exit_loop; ret = AVERROR_INVALIDDATA; @@ -1228,9 +1239,9 @@ static int decode_frame_common(AVCodecContext *avctx, PNGDecContext *s, break; } case MKTAG('I', 'E', 'N', 'D'): - if (!(s->state & PNG_ALLIMAGE)) + if (s->pic_state != PNG_IMG_ALL) av_log(avctx, AV_LOG_ERROR, "IEND without all image\n"); - if (!(s->state & (PNG_ALLIMAGE|PNG_IDAT))) { + if (s->pic_state == PNG_IMG_NONE) { ret = AVERROR_INVALIDDATA; goto fail; } @@ -1330,7 +1341,8 @@ static int decode_frame_png(AVCodecContext *avctx, return AVERROR_INVALIDDATA; } - s->y = s->state = s->has_trns = 0; + s->y = s->hdr_state = s->has_trns = 0; + s->pic_state = PNG_IMG_NONE; /* init the zlib */ s->zstream.zalloc = ff_png_zalloc; @@ -1377,7 +1389,7 @@ static int decode_frame_apng(AVCodecContext *avctx, FFSWAP(ThreadFrame, s->picture, s->last_picture); p = s->picture.f; - if (!(s->state & PNG_IHDR)) { + if (!(s->hdr_state & PNG_IHDR)) { if (!avctx->extradata_size) return AVERROR_INVALIDDATA; @@ -1397,14 +1409,14 @@ static int decode_frame_apng(AVCodecContext *avctx, goto end; } s->y = 0; - s->state &= ~(PNG_IDAT | PNG_ALLIMAGE); + s->pic_state = PNG_IMG_NONE; bytestream2_init(&s->gb, avpkt->data, avpkt->size); if ((ret = decode_frame_common(avctx, s, p, avpkt)) < 0) goto end; - if (!(s->state & PNG_ALLIMAGE)) + if (s->pic_state != PNG_IMG_ALL) av_log(avctx, AV_LOG_WARNING, "Frame did not contain a complete image\n"); - if (!(s->state & (PNG_ALLIMAGE|PNG_IDAT))) { + if (s->pic_state == PNG_IMG_NONE) { ret = AVERROR_INVALIDDATA; goto end; } @@ -1453,7 +1465,7 @@ static int update_thread_context(AVCodecContext *dst, const AVCodecContext *src) memcpy(pdst->palette, psrc->palette, sizeof(pdst->palette)); - pdst->state |= psrc->state & (PNG_IHDR | PNG_PLTE); + pdst->hdr_state = psrc->hdr_state; ff_thread_release_buffer(dst, &pdst->last_picture); if (psrc->last_picture.f->data[0] &&