diff mbox series

[FFmpeg-devel,212/217] avcodec/snow: Fix race in ff_snow_common_init()

Message ID 20201202042244.519127-78-andreas.rheinhardt@gmail.com
State Accepted
Commit 6a94afbd5bf4aa3ccab12f038e8bbae7ef3973c0
Headers show
Series [FFmpeg-devel,01/45] avcodec/a64multienc: Fix memleak upon init failure
Related show

Checks

Context Check Description
andriy/x86 warning Failed to apply patch

Commit Message

Andreas Rheinhardt Dec. 2, 2020, 4:22 a.m. UTC
Commits d49210788b0836d56dd872d517fe73f83b080101 and
ee8ce211ead04c8684da0c9c143814e57e9b9eda set the
FF_CODEC_CAP_INIT_THREADSAFE flag for the Snow encoder resp. decoder;
yet these codecs init functions aren't threadsafe at all:
ff_snow_common_init() initializes static data, but there is no check
at all that it is only done once by one thread.

This commit adds such checks; this makes the decoder init-threadsafe as
long as the stack is properly aligned.

Signed-off-by: Andreas Rheinhardt <andreas.rheinhardt@gmail.com>
---
 libavcodec/snow.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

Comments

Michael Niedermayer Dec. 2, 2020, 8:49 p.m. UTC | #1
On Wed, Dec 02, 2020 at 05:22:39AM +0100, Andreas Rheinhardt wrote:
> Commits d49210788b0836d56dd872d517fe73f83b080101 and
> ee8ce211ead04c8684da0c9c143814e57e9b9eda set the
> FF_CODEC_CAP_INIT_THREADSAFE flag for the Snow encoder resp. decoder;
> yet these codecs init functions aren't threadsafe at all:
> ff_snow_common_init() initializes static data, but there is no check
> at all that it is only done once by one thread.

These commits originated from long ago when it was felt that writing
the same value in the same location by 2 threads and always writing that
value in a thread before the same thread read it would not qualify
as undefined behavior or a race.
The current C standard makes it UB (to the best of my knowledge) 
so your change is correct but i think your commit message:
"aren't threadsafe at all" is suggesting a major issue which i think
this is not.
That said, is there any case known where code like this produces unexpected
behavior on any real platform with any real compiler ? (not counting
thread saftey check tools detecting / aborting)
For most undefined behavior cases i can think of some optimization
messing up or something but in this case everything i could think of
requires some rather odd hypothetical hardware ...

thx

[...]
Andreas Rheinhardt Dec. 2, 2020, 10:45 p.m. UTC | #2
Michael Niedermayer:
> On Wed, Dec 02, 2020 at 05:22:39AM +0100, Andreas Rheinhardt wrote:
>> Commits d49210788b0836d56dd872d517fe73f83b080101 and
>> ee8ce211ead04c8684da0c9c143814e57e9b9eda set the
>> FF_CODEC_CAP_INIT_THREADSAFE flag for the Snow encoder resp. decoder;
>> yet these codecs init functions aren't threadsafe at all:
>> ff_snow_common_init() initializes static data, but there is no check
>> at all that it is only done once by one thread.
> 
> These commits originated from long ago when it was felt that writing
> the same value in the same location by 2 threads and always writing that
> value in a thread before the same thread read it would not qualify
> as undefined behavior or a race.
> The current C standard makes it UB (to the best of my knowledge) 

Indeed: "Two expression evaluations conflict if one of them modifies a
memory location and the other one reads or modifies the same memory
location." Notice that there is no exception in case the newly written
value coincides with the old value (or if both are write operations and
both write the same value). And lateron: "The execution of a program
contains a data race if it contains two conflicting actions in
different threads, at least one of which is not atomic, and neither
happens before the other. Any such data race results in undefined behavior."

> so your change is correct but i think your commit message:
> "aren't threadsafe at all" is suggesting a major issue which i think
> this is not.

Ok. Then how about something like:

avcodec/snow: Use ff_thread_once() in ff_snow_common_init()

ff_snow_common_init() currently initializes static data every time it is
invoked; given that both the Snow encoder and decoder have the
FF_CODEC_CAP_INIT_THREADSAFE flag set, this can lead to data races (and
therefore undefined behaviour) even though all threads write the same
values. This commit fixes this by using ff_thread_once() for the
initializations.

> That said, is there any case known where code like this produces unexpected
> behavior on any real platform with any real compiler ? (not counting
> thread saftey check tools detecting / aborting)

If the underlying writes are atomic, no; these are int and uint8_t,
respectively, so the writes will likely be atomic. [1] contains a nice
summary for how real the danger of this is.

> For most undefined behavior cases i can think of some optimization
> messing up or something but in this case everything i could think of
> requires some rather odd hypothetical hardware ...
> 
So do I, but it is nevertheless UB. And using a sanitizer without a lot
of false positives is also a nice benefit.
(Also notice that the very same has been said about signed integer
overflow.)

- Andreas

[1]: https://lwn.net/Articles/793253/
Michael Niedermayer Dec. 3, 2020, 12:16 a.m. UTC | #3
On Wed, Dec 02, 2020 at 11:45:10PM +0100, Andreas Rheinhardt wrote:
> Michael Niedermayer:
> > On Wed, Dec 02, 2020 at 05:22:39AM +0100, Andreas Rheinhardt wrote:
> >> Commits d49210788b0836d56dd872d517fe73f83b080101 and
> >> ee8ce211ead04c8684da0c9c143814e57e9b9eda set the
> >> FF_CODEC_CAP_INIT_THREADSAFE flag for the Snow encoder resp. decoder;
> >> yet these codecs init functions aren't threadsafe at all:
> >> ff_snow_common_init() initializes static data, but there is no check
> >> at all that it is only done once by one thread.
> > 
> > These commits originated from long ago when it was felt that writing
> > the same value in the same location by 2 threads and always writing that
> > value in a thread before the same thread read it would not qualify
> > as undefined behavior or a race.
> > The current C standard makes it UB (to the best of my knowledge) 
> 
> Indeed: "Two expression evaluations conflict if one of them modifies a
> memory location and the other one reads or modifies the same memory
> location." Notice that there is no exception in case the newly written
> value coincides with the old value (or if both are write operations and
> both write the same value). And lateron: "The execution of a program
> contains a data race if it contains two conflicting actions in
> different threads, at least one of which is not atomic, and neither
> happens before the other. Any such data race results in undefined behavior."
> 
> > so your change is correct but i think your commit message:
> > "aren't threadsafe at all" is suggesting a major issue which i think
> > this is not.
> 
> Ok. Then how about something like:
> 
> avcodec/snow: Use ff_thread_once() in ff_snow_common_init()
> 
> ff_snow_common_init() currently initializes static data every time it is
> invoked; given that both the Snow encoder and decoder have the
> FF_CODEC_CAP_INIT_THREADSAFE flag set, this can lead to data races (and
> therefore undefined behaviour) even though all threads write the same
> values. This commit fixes this by using ff_thread_once() for the
> initializations.

ok, thanks


> 
> > That said, is there any case known where code like this produces unexpected
> > behavior on any real platform with any real compiler ? (not counting
> > thread saftey check tools detecting / aborting)
> 
> If the underlying writes are atomic, no; these are int and uint8_t,
> respectively, so the writes will likely be atomic. [1] contains a nice
> summary for how real the danger of this is.
> 
> > For most undefined behavior cases i can think of some optimization
> > messing up or something but in this case everything i could think of
> > requires some rather odd hypothetical hardware ...
> > 
> So do I, but it is nevertheless UB. And using a sanitizer without a lot
> of false positives is also a nice benefit.
> (Also notice that the very same has been said about signed integer
> overflow.)

yes, indeed, and rethinking this, a compiler can in fact break with this
consider a function writing to global/static variable without any sync/atomic/...
the compiler can assume that this part of the function cannot run in parallel
with itself. And at this point the compiler could remove atomic/sync/... code
if it can only be needed for cases where the double write would be possible
This interpretation is still a bit borderline though, because here the compiler
would optimize code away because nothing prevents UB not because a conflicting
(UB) write actually occurs. Id have to reread the spec to see how exactly it is
worded to know if this is actually allowed. OTOH if its not allowed for a
compiler to do such a optimization then it seems that would be a missed
opertunity.

thx


[...]
diff mbox series

Patch

diff --git a/libavcodec/snow.c b/libavcodec/snow.c
index 066efc5171..bb65a6f43f 100644
--- a/libavcodec/snow.c
+++ b/libavcodec/snow.c
@@ -21,6 +21,7 @@ 
 #include "libavutil/intmath.h"
 #include "libavutil/log.h"
 #include "libavutil/opt.h"
+#include "libavutil/thread.h"
 #include "avcodec.h"
 #include "me_cmp.h"
 #include "snow_dwt.h"
@@ -427,10 +428,19 @@  mca( 8, 0,8)
 mca( 0, 8,8)
 mca( 8, 8,8)
 
+static av_cold void snow_static_init(void)
+{
+    for (int i = 0; i < MAX_REF_FRAMES; i++)
+        for (int j = 0; j < MAX_REF_FRAMES; j++)
+            ff_scale_mv_ref[i][j] = 256 * (i + 1) / (j + 1);
+    init_qexp();
+}
+
 av_cold int ff_snow_common_init(AVCodecContext *avctx){
+    static AVOnce init_static_once = AV_ONCE_INIT;
     SnowContext *s = avctx->priv_data;
     int width, height;
-    int i, j;
+    int i;
 
     s->avctx= avctx;
     s->max_ref_frames=1; //just make sure it's not an invalid value in case of no initial keyframe
@@ -480,8 +490,6 @@  av_cold int ff_snow_common_init(AVCodecContext *avctx){
     mcfh(0, 8)
     mcfh(8, 8)
 
-    init_qexp();
-
 //    dec += FFMAX(s->chroma_h_shift, s->chroma_v_shift);
 
     width= s->avctx->width;
@@ -495,8 +503,6 @@  av_cold int ff_snow_common_init(AVCodecContext *avctx){
         return AVERROR(ENOMEM);
 
     for(i=0; i<MAX_REF_FRAMES; i++) {
-        for(j=0; j<MAX_REF_FRAMES; j++)
-            ff_scale_mv_ref[i][j] = 256*(i+1)/(j+1);
         s->last_picture[i] = av_frame_alloc();
         if (!s->last_picture[i])
             return AVERROR(ENOMEM);
@@ -507,6 +513,8 @@  av_cold int ff_snow_common_init(AVCodecContext *avctx){
     if (!s->mconly_picture || !s->current_picture)
         return AVERROR(ENOMEM);
 
+    ff_thread_once(&init_static_once, snow_static_init);
+
     return 0;
 }