diff mbox

[FFmpeg-devel] tsan warning about data race in h264 decoder

Message ID CALTJjxFVb9+MrHBzU8q7SB6Eo9kcOa9BuxCZFESKf8pQwqg2UQ@mail.gmail.com
State New
Headers show

Commit Message

Wan-Teh Chang July 18, 2017, 4:59 p.m. UTC 
Hi,

I'd like to report a tsan warning about a data race in the h264 decoder.

1. Steps to reproduce:

./configure --samples=~/fate-suite/ --toolchain=clang-tsan --disable-stripping

make fate-h264 THREADS=4

2. Here is an excerpt of the tsan warning in
tests/data/fate/h264-conformance-ba1_ft_c.err:

WARNING: ThreadSanitizer: data race (pid=31070)
  Write of size 8 at 0x7bbc000082a8 by thread T1 (mutexes: write M1628):
    #0 memcpy /work/release-test/final/llvm.src/projects/compiler-rt/lib/tsan/../sanitizer_common/sanitizer_common_interceptors.inc:655:5
(ffmpeg+0x10de9d)
    #1 h264_initialise_ref_list
home/wtc/tmp/ffmpeg/libavcodec/h264_refs.c:214:29 (ffmpeg+0x1186b3f)
    #2 ff_h264_build_ref_list
home/wtc/tmp/ffmpeg/libavcodec/h264_refs.c:306 (ffmpeg+0x1186b3f)
    #3 h264_slice_init
home/wtc/tmp/ffmpeg/libavcodec/h264_slice.c:1900:11 (ffmpeg+0x1191149)
    #4 ff_h264_queue_decode_slice
home/wtc/tmp/ffmpeg/libavcodec/h264_slice.c:2129 (ffmpeg+0x1191149)
    #5 decode_nal_units
home/wtc/tmp/ffmpeg/libavcodec/h264dec.c:675:24 (ffmpeg+0x7924ec)
    #6 h264_decode_frame home/wtc/tmp/ffmpeg/libavcodec/h264dec.c:1006
(ffmpeg+0x7924ec)
    #7 frame_worker_thread
home/wtc/tmp/ffmpeg/libavcodec/pthread_frame.c:201:21
(ffmpeg+0xae56dc)

  Previous read of size 8 at 0x7bbc000082a8 by main thread (mutexes:
write M1630):
    #0 memcpy /work/release-test/final/llvm.src/projects/compiler-rt/lib/tsan/../sanitizer_common/sanitizer_common_interceptors.inc:655:5
(ffmpeg+0x10de9d)
    #1 ff_h264_update_thread_context
home/wtc/tmp/ffmpeg/libavcodec/h264_slice.c:411:5 (ffmpeg+0x118b7dc)
    #2 update_context_from_thread
home/wtc/tmp/ffmpeg/libavcodec/pthread_frame.c:315:19
(ffmpeg+0xae4302)
    #3 submit_packet
home/wtc/tmp/ffmpeg/libavcodec/pthread_frame.c:418:15
(ffmpeg+0xae3783)
    #4 ff_thread_decode_frame
home/wtc/tmp/ffmpeg/libavcodec/pthread_frame.c:495 (ffmpeg+0xae3783)
    #5 decode_simple_internal
home/wtc/tmp/ffmpeg/libavcodec/decode.c:415:15 (ffmpeg+0x665ae4)
    #6 decode_simple_receive_frame
home/wtc/tmp/ffmpeg/libavcodec/decode.c:620 (ffmpeg+0x665ae4)
    #7 decode_receive_frame_internal
home/wtc/tmp/ffmpeg/libavcodec/decode.c:638 (ffmpeg+0x665ae4)
    #8 avcodec_send_packet
home/wtc/tmp/ffmpeg/libavcodec/decode.c:678:15 (ffmpeg+0x665017)
    #9 decode home/wtc/tmp/ffmpeg/ffmpeg.c:2265:15 (ffmpeg+0x1a748e)
    #10 decode_video home/wtc/tmp/ffmpeg/ffmpeg.c:2409 (ffmpeg+0x1a748e)
    #11 process_input_packet home/wtc/tmp/ffmpeg/ffmpeg.c:2644
(ffmpeg+0x1a748e)    #12 process_input
home/wtc/tmp/ffmpeg/ffmpeg.c:4432:5 (ffmpeg+0x1a2e69)
    #13 transcode_step home/wtc/tmp/ffmpeg/ffmpeg.c:4543 (ffmpeg+0x1a2e69)
    #14 transcode home/wtc/tmp/ffmpeg/ffmpeg.c:4597 (ffmpeg+0x1a2e69)
    #15 main home/wtc/tmp/ffmpeg/ffmpeg.c:4803:9 (ffmpeg+0x19daef)

3. The relevant source code is:

libavcodec/h264_refs.c:

135 static void h264_initialise_ref_list(H264Context *h, H264SliceContext *sl)
136 {
...
213     for (i = 0; i < sl->list_count; i++)
214         h->default_ref[i] = sl->ref_list[i][0];
215 }

libavcodec/h264_slice.c:

288 int ff_h264_update_thread_context(AVCodecContext *dst,
289                                   const AVCodecContext *src)
290 {
...
409     memcpy(&h->poc,        &h1->poc,        sizeof(h->poc));
410
411     memcpy(h->default_ref, h1->default_ref, sizeof(h->default_ref));
412     memcpy(h->short_ref,   h1->short_ref,   sizeof(h->short_ref));
413     memcpy(h->long_ref,    h1->long_ref,    sizeof(h->long_ref));
414     memcpy(h->delayed_pic, h1->delayed_pic, sizeof(h->delayed_pic));
415     memcpy(h->last_pocs,   h1->last_pocs,   sizeof(h->last_pocs));

4. I can eliminate the tsan warning by deleting line 411 in
libavcodec/h264_slice.c, but I don't know if that is the correct fix.


Wan-Teh Chang

Comments

Ronald S. Bultje July 18, 2017, 6:41 p.m. UTC | #1 
Hi Wah-Teng,

On Tue, Jul 18, 2017 at 12:59 PM, Wan-Teh Chang <
wtc-at-google.com@ffmpeg.org> wrote:

> Hi,
>
> I'd like to report a tsan warning about a data race in the h264 decoder.
>
> 1. Steps to reproduce:
>
> ./configure --samples=~/fate-suite/ --toolchain=clang-tsan
> --disable-stripping
>
> make fate-h264 THREADS=4
>
> 2. Here is an excerpt of the tsan warning in
> tests/data/fate/h264-conformance-ba1_ft_c.err:
>
> WARNING: ThreadSanitizer: data race (pid=31070)
>   Write of size 8 at 0x7bbc000082a8 by thread T1 (mutexes: write M1628):
>     #0 memcpy /work/release-test/final/llvm.src/projects/compiler-rt/lib/
> tsan/../sanitizer_common/sanitizer_common_interceptors.inc:655:5
> (ffmpeg+0x10de9d)
>     #1 h264_initialise_ref_list
> home/wtc/tmp/ffmpeg/libavcodec/h264_refs.c:214:29 (ffmpeg+0x1186b3f)
>     #2 ff_h264_build_ref_list
> home/wtc/tmp/ffmpeg/libavcodec/h264_refs.c:306 (ffmpeg+0x1186b3f)
>     #3 h264_slice_init
> home/wtc/tmp/ffmpeg/libavcodec/h264_slice.c:1900:11 (ffmpeg+0x1191149)
>     #4 ff_h264_queue_decode_slice
> home/wtc/tmp/ffmpeg/libavcodec/h264_slice.c:2129 (ffmpeg+0x1191149)
>     #5 decode_nal_units
> home/wtc/tmp/ffmpeg/libavcodec/h264dec.c:675:24 (ffmpeg+0x7924ec)
>     #6 h264_decode_frame home/wtc/tmp/ffmpeg/libavcodec/h264dec.c:1006
> (ffmpeg+0x7924ec)
>     #7 frame_worker_thread
> home/wtc/tmp/ffmpeg/libavcodec/pthread_frame.c:201:21
> (ffmpeg+0xae56dc)
>
>   Previous read of size 8 at 0x7bbc000082a8 by main thread (mutexes:
> write M1630):
>     #0 memcpy /work/release-test/final/llvm.src/projects/compiler-rt/lib/
> tsan/../sanitizer_common/sanitizer_common_interceptors.inc:655:5
> (ffmpeg+0x10de9d)
>     #1 ff_h264_update_thread_context
> home/wtc/tmp/ffmpeg/libavcodec/h264_slice.c:411:5 (ffmpeg+0x118b7dc)
>     #2 update_context_from_thread
> home/wtc/tmp/ffmpeg/libavcodec/pthread_frame.c:315:19
> (ffmpeg+0xae4302)
>     #3 submit_packet
> home/wtc/tmp/ffmpeg/libavcodec/pthread_frame.c:418:15
> (ffmpeg+0xae3783)
>     #4 ff_thread_decode_frame
> home/wtc/tmp/ffmpeg/libavcodec/pthread_frame.c:495 (ffmpeg+0xae3783)
>     #5 decode_simple_internal
> home/wtc/tmp/ffmpeg/libavcodec/decode.c:415:15 (ffmpeg+0x665ae4)
>     #6 decode_simple_receive_frame
> home/wtc/tmp/ffmpeg/libavcodec/decode.c:620 (ffmpeg+0x665ae4)
>     #7 decode_receive_frame_internal
> home/wtc/tmp/ffmpeg/libavcodec/decode.c:638 (ffmpeg+0x665ae4)
>     #8 avcodec_send_packet
> home/wtc/tmp/ffmpeg/libavcodec/decode.c:678:15 (ffmpeg+0x665017)
>     #9 decode home/wtc/tmp/ffmpeg/ffmpeg.c:2265:15 (ffmpeg+0x1a748e)
>     #10 decode_video home/wtc/tmp/ffmpeg/ffmpeg.c:2409 (ffmpeg+0x1a748e)
>     #11 process_input_packet home/wtc/tmp/ffmpeg/ffmpeg.c:2644
> (ffmpeg+0x1a748e)    #12 process_input
> home/wtc/tmp/ffmpeg/ffmpeg.c:4432:5 (ffmpeg+0x1a2e69)
>     #13 transcode_step home/wtc/tmp/ffmpeg/ffmpeg.c:4543 (ffmpeg+0x1a2e69)
>     #14 transcode home/wtc/tmp/ffmpeg/ffmpeg.c:4597 (ffmpeg+0x1a2e69)
>     #15 main home/wtc/tmp/ffmpeg/ffmpeg.c:4803:9 (ffmpeg+0x19daef)
>
> 3. The relevant source code is:
>
> libavcodec/h264_refs.c:
>
> 135 static void h264_initialise_ref_list(H264Context *h, H264SliceContext
> *sl)
> 136 {
> ...
> 213     for (i = 0; i < sl->list_count; i++)
> 214         h->default_ref[i] = sl->ref_list[i][0];
> 215 }
>
> libavcodec/h264_slice.c:
>
> 288 int ff_h264_update_thread_context(AVCodecContext *dst,
> 289                                   const AVCodecContext *src)
> 290 {
> ...
> 409     memcpy(&h->poc,        &h1->poc,        sizeof(h->poc));
> 410
> 411     memcpy(h->default_ref, h1->default_ref, sizeof(h->default_ref));
> 412     memcpy(h->short_ref,   h1->short_ref,   sizeof(h->short_ref));
> 413     memcpy(h->long_ref,    h1->long_ref,    sizeof(h->long_ref));
> 414     memcpy(h->delayed_pic, h1->delayed_pic, sizeof(h->delayed_pic));
> 415     memcpy(h->last_pocs,   h1->last_pocs,   sizeof(h->last_pocs));
>
> 4. I can eliminate the tsan warning by deleting line 411 in
> libavcodec/h264_slice.c, but I don't know if that is the correct fix.
>
> diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
> index 6deb08fe6d..2fb89b189d 100644
> --- a/libavcodec/h264_slice.c
> +++ b/libavcodec/h264_slice.c
> @@ -408,7 +408,6 @@ int ff_h264_update_thread_context(AVCodecContext *dst,
>
>      memcpy(&h->poc,        &h1->poc,        sizeof(h->poc));
>
> -    memcpy(h->default_ref, h1->default_ref, sizeof(h->default_ref));
>      memcpy(h->short_ref,   h1->short_ref,   sizeof(h->short_ref));
>      memcpy(h->long_ref,    h1->long_ref,    sizeof(h->long_ref));
>      memcpy(h->delayed_pic, h1->delayed_pic, sizeof(h->delayed_pic));


Reminds me a little of
http://git.videolan.org/?p=ffmpeg.git;a=commit;h=e72690b18da064f6c0f04f09ccde72b6636e3159.
From a quick look, it appears that default_ref[] is unconditionally
initialized in initialise_ref_list() (called from ff_h264_build_ref_list(),
called from h264_slice_init()), so I think your suggested patch is correct.
Can you send it as a git-formatted patch?

Ronald
Wan-Teh Chang July 18, 2017, 11:43 p.m. UTC | #2 
Hi Ronald,

On Tue, Jul 18, 2017 at 11:41 AM, Ronald S. Bultje <rsbultje@gmail.com> wrote:
>
> Reminds me a little of
> http://git.videolan.org/?p=ffmpeg.git;a=commit;h=e72690b18da064f6c0f04f09ccde72b6636e3159.
> From a quick look, it appears that default_ref[] is unconditionally
> initialized in initialise_ref_list() (called from ff_h264_build_ref_list(),
> called from h264_slice_init()), so I think your suggested patch is correct.

Thanks a lot for taking a look and pointing me at the relevant code. I
reviewed those functions, and I think you are right.

I am curious: why do we need to sync short_ref[], long_ref[],
short_ref_count, and long_ref_count between threads?

> Can you send it as a git-formatted patch?

Done. Please see http://ffmpeg.org/pipermail/ffmpeg-devel/2017-July/213777.html.

Wan-Teh Chang
diff mbox

Patch

diff --git a/libavcodec/h264_slice.c b/libavcodec/h264_slice.c
index 6deb08fe6d..2fb89b189d 100644
--- a/libavcodec/h264_slice.c
+++ b/libavcodec/h264_slice.c
@@ -408,7 +408,6 @@  int ff_h264_update_thread_context(AVCodecContext *dst,

     memcpy(&h->poc,        &h1->poc,        sizeof(h->poc));

-    memcpy(h->default_ref, h1->default_ref, sizeof(h->default_ref));
     memcpy(h->short_ref,   h1->short_ref,   sizeof(h->short_ref));
     memcpy(h->long_ref,    h1->long_ref,    sizeof(h->long_ref));
     memcpy(h->delayed_pic, h1->delayed_pic, sizeof(h->delayed_pic));