diff mbox

[FFmpeg-devel] png: split header state and data state in two separate variables.

Message ID 1490968192-55497-1-git-send-email-rsbultje@gmail.com
State Superseded
Headers show

Commit Message

Ronald S. Bultje March 31, 2017, 1:49 p.m. UTC
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(-)

Comments

Michael Niedermayer March 31, 2017, 10:53 p.m. UTC | #1
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 mbox

Patch

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] &&