diff mbox

[FFmpeg-devel] avcodec/libx264: remove FF_CODEC_CAP_INIT_THREADSAFE flag

Message ID 20181020103540.22452-1-cus@passwd.hu
State Accepted
Commit b02490a497009064b7f192802aa246aa0b6a4dad
Headers

Commit Message

Marton Balint Oct. 20, 2018, 10:35 a.m. UTC 
Libx264 uses strtok which is not thread safe. Strtok is used in
x264_param_default_preset in param_apply_tune in x264/common/base.c.
Therefore the flag must be removed.

Fixes ticket #7446.

Signed-off-by: Marton Balint <cus@passwd.hu>
---
 libavcodec/libx264.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

Comments

Hendrik Leppkes Oct. 20, 2018, 11:35 a.m. UTC | #1 
On Sat, Oct 20, 2018 at 12:35 PM Marton Balint <cus@passwd.hu> wrote:
>
> Libx264 uses strtok which is not thread safe. Strtok is used in
> x264_param_default_preset in param_apply_tune in x264/common/base.c.
> Therefore the flag must be removed.
>
> Fixes ticket #7446.
>
> Signed-off-by: Marton Balint <cus@passwd.hu>
> ---
>  libavcodec/libx264.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> index 54e6703d73..d6367bf557 100644
> --- a/libavcodec/libx264.c
> +++ b/libavcodec/libx264.c
> @@ -1063,8 +1063,7 @@ AVCodec ff_libx264_encoder = {
>      .priv_class       = &x264_class,
>      .defaults         = x264_defaults,
>      .init_static_data = X264_init_static,
> -    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
> -                        FF_CODEC_CAP_INIT_CLEANUP,
> +    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
>      .wrapper_name     = "libx264",
>  };
>  #endif
> @@ -1115,8 +1114,7 @@ AVCodec ff_libx262_encoder = {
>      .priv_class       = &X262_class,
>      .defaults         = x264_defaults,
>      .pix_fmts         = pix_fmts_8bit,
> -    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
> -                        FF_CODEC_CAP_INIT_CLEANUP,
> +    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
>      .wrapper_name     = "libx264",
>  };
>  #endif
> --
> 2.16.4
>

Was this ever reported to x264? Theoretically some other library or
another part of a calling application could still use strtok and break
it.

- Hendrik
Marton Balint Oct. 20, 2018, 12:30 p.m. UTC | #2 
On Sat, 20 Oct 2018, Hendrik Leppkes wrote:

> On Sat, Oct 20, 2018 at 12:35 PM Marton Balint <cus@passwd.hu> wrote:
>>
>> Libx264 uses strtok which is not thread safe. Strtok is used in
>> x264_param_default_preset in param_apply_tune in x264/common/base.c.
>> Therefore the flag must be removed.
>>
>> Fixes ticket #7446.
>>
>> Signed-off-by: Marton Balint <cus@passwd.hu>
>> ---
>>  libavcodec/libx264.c | 6 ++----
>>  1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
>> index 54e6703d73..d6367bf557 100644
>> --- a/libavcodec/libx264.c
>> +++ b/libavcodec/libx264.c
>> @@ -1063,8 +1063,7 @@ AVCodec ff_libx264_encoder = {
>>      .priv_class       = &x264_class,
>>      .defaults         = x264_defaults,
>>      .init_static_data = X264_init_static,
>> -    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
>> -                        FF_CODEC_CAP_INIT_CLEANUP,
>> +    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
>>      .wrapper_name     = "libx264",
>>  };
>>  #endif
>> @@ -1115,8 +1114,7 @@ AVCodec ff_libx262_encoder = {
>>      .priv_class       = &X262_class,
>>      .defaults         = x264_defaults,
>>      .pix_fmts         = pix_fmts_8bit,
>> -    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
>> -                        FF_CODEC_CAP_INIT_CLEANUP,
>> +    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
>>      .wrapper_name     = "libx264",
>>  };
>>  #endif
>> --
>> 2.16.4
>>
>
> Was this ever reported to x264?

Not by me.

> Theoretically some other library or another part of a calling 
> application could still use strtok and break it.

I agree, this should be fixed in x264 as well, but until then it 
seems harmless to remove the flag.

Regards,
Marton
Vittorio Giovara Oct. 20, 2018, 2:50 p.m. UTC | #3 
On Sat, Oct 20, 2018 at 12:30 PM Marton Balint <cus@passwd.hu> wrote:

>
>
> On Sat, 20 Oct 2018, Hendrik Leppkes wrote:
>
> > On Sat, Oct 20, 2018 at 12:35 PM Marton Balint <cus@passwd.hu> wrote:
> >>
> >> Libx264 uses strtok which is not thread safe. Strtok is used in
> >> x264_param_default_preset in param_apply_tune in x264/common/base.c.
> >> Therefore the flag must be removed.
> >>
> >> Fixes ticket #7446.
> >>
> >> Signed-off-by: Marton Balint <cus@passwd.hu>
> >> ---
> >>  libavcodec/libx264.c | 6 ++----
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> >> index 54e6703d73..d6367bf557 100644
> >> --- a/libavcodec/libx264.c
> >> +++ b/libavcodec/libx264.c
> >> @@ -1063,8 +1063,7 @@ AVCodec ff_libx264_encoder = {
> >>      .priv_class       = &x264_class,
> >>      .defaults         = x264_defaults,
> >>      .init_static_data = X264_init_static,
> >> -    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
> >> -                        FF_CODEC_CAP_INIT_CLEANUP,
> >> +    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
> >>      .wrapper_name     = "libx264",
> >>  };
> >>  #endif
> >> @@ -1115,8 +1114,7 @@ AVCodec ff_libx262_encoder = {
> >>      .priv_class       = &X262_class,
> >>      .defaults         = x264_defaults,
> >>      .pix_fmts         = pix_fmts_8bit,
> >> -    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
> >> -                        FF_CODEC_CAP_INIT_CLEANUP,
> >> +    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
> >>      .wrapper_name     = "libx264",
> >>  };
> >>  #endif
> >> --
> >> 2.16.4
> >>
> >
> > Was this ever reported to x264?
>
> Not by me.
>
> > Theoretically some other library or another part of a calling
> > application could still use strtok and break it.
>
> I agree, this should be fixed in x264 as well, but until then it
> seems harmless to remove the flag.
>

this could potentially break a couple of use cases where it "happens to
work" (especially those that rely on the aforementioned abaa1226)
rather that dropping the flag entirely, why not fixing x264 first, bump its
version, and apply the flag conditionally on that?
Marton Balint Oct. 20, 2018, 5:12 p.m. UTC | #4 
On Sat, 20 Oct 2018, Vittorio Giovara wrote:

> On Sat, Oct 20, 2018 at 12:30 PM Marton Balint <cus@passwd.hu> wrote:
>
>>
>>
>> On Sat, 20 Oct 2018, Hendrik Leppkes wrote:
>>
>> > On Sat, Oct 20, 2018 at 12:35 PM Marton Balint <cus@passwd.hu> wrote:
>> >>
>> >> Libx264 uses strtok which is not thread safe. Strtok is used in
>> >> x264_param_default_preset in param_apply_tune in x264/common/base.c.
>> >> Therefore the flag must be removed.
>> >>
>> >> Fixes ticket #7446.
>> >>
>> >> Signed-off-by: Marton Balint <cus@passwd.hu>
>> >> ---
>> >>  libavcodec/libx264.c | 6 ++----
>> >>  1 file changed, 2 insertions(+), 4 deletions(-)
>> >>
>> >> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
>> >> index 54e6703d73..d6367bf557 100644
>> >> --- a/libavcodec/libx264.c
>> >> +++ b/libavcodec/libx264.c
>> >> @@ -1063,8 +1063,7 @@ AVCodec ff_libx264_encoder = {
>> >>      .priv_class       = &x264_class,
>> >>      .defaults         = x264_defaults,
>> >>      .init_static_data = X264_init_static,
>> >> -    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
>> >> -                        FF_CODEC_CAP_INIT_CLEANUP,
>> >> +    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
>> >>      .wrapper_name     = "libx264",
>> >>  };
>> >>  #endif
>> >> @@ -1115,8 +1114,7 @@ AVCodec ff_libx262_encoder = {
>> >>      .priv_class       = &X262_class,
>> >>      .defaults         = x264_defaults,
>> >>      .pix_fmts         = pix_fmts_8bit,
>> >> -    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
>> >> -                        FF_CODEC_CAP_INIT_CLEANUP,
>> >> +    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
>> >>      .wrapper_name     = "libx264",
>> >>  };
>> >>  #endif
>> >> --
>> >> 2.16.4
>> >>
>> >
>> > Was this ever reported to x264?
>>
>> Not by me.
>>
>> > Theoretically some other library or another part of a calling
>> > application could still use strtok and break it.
>>
>> I agree, this should be fixed in x264 as well, but until then it
>> seems harmless to remove the flag.
>>
>
> this could potentially break a couple of use cases where it "happens to
> work" (especially those that rely on the aforementioned abaa1226)
> rather that dropping the flag entirely, why not fixing x264 first, bump its
> version, and apply the flag conditionally on that?

Fine by me, if someone could pick this up on the x264 side that would be 
great.

Thanks,
Marton
Hendrik Leppkes Oct. 20, 2018, 6:14 p.m. UTC | #5 
On Sat, Oct 20, 2018 at 4:50 PM Vittorio Giovara
<vittorio.giovara@gmail.com> wrote:
>
> On Sat, Oct 20, 2018 at 12:30 PM Marton Balint <cus@passwd.hu> wrote:
>
> >
> >
> > On Sat, 20 Oct 2018, Hendrik Leppkes wrote:
> >
> > > On Sat, Oct 20, 2018 at 12:35 PM Marton Balint <cus@passwd.hu> wrote:
> > >>
> > >> Libx264 uses strtok which is not thread safe. Strtok is used in
> > >> x264_param_default_preset in param_apply_tune in x264/common/base.c.
> > >> Therefore the flag must be removed.
> > >>
> > >> Fixes ticket #7446.
> > >>
> > >> Signed-off-by: Marton Balint <cus@passwd.hu>
> > >> ---
> > >>  libavcodec/libx264.c | 6 ++----
> > >>  1 file changed, 2 insertions(+), 4 deletions(-)
> > >>
> > >> diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
> > >> index 54e6703d73..d6367bf557 100644
> > >> --- a/libavcodec/libx264.c
> > >> +++ b/libavcodec/libx264.c
> > >> @@ -1063,8 +1063,7 @@ AVCodec ff_libx264_encoder = {
> > >>      .priv_class       = &x264_class,
> > >>      .defaults         = x264_defaults,
> > >>      .init_static_data = X264_init_static,
> > >> -    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
> > >> -                        FF_CODEC_CAP_INIT_CLEANUP,
> > >> +    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
> > >>      .wrapper_name     = "libx264",
> > >>  };
> > >>  #endif
> > >> @@ -1115,8 +1114,7 @@ AVCodec ff_libx262_encoder = {
> > >>      .priv_class       = &X262_class,
> > >>      .defaults         = x264_defaults,
> > >>      .pix_fmts         = pix_fmts_8bit,
> > >> -    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
> > >> -                        FF_CODEC_CAP_INIT_CLEANUP,
> > >> +    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
> > >>      .wrapper_name     = "libx264",
> > >>  };
> > >>  #endif
> > >> --
> > >> 2.16.4
> > >>
> > >
> > > Was this ever reported to x264?
> >
> > Not by me.
> >
> > > Theoretically some other library or another part of a calling
> > > application could still use strtok and break it.
> >
> > I agree, this should be fixed in x264 as well, but until then it
> > seems harmless to remove the flag.
> >
>
> this could potentially break a couple of use cases where it "happens to
> work" (especially those that rely on the aforementioned abaa1226)
> rather that dropping the flag entirely, why not fixing x264 first, bump its
> version, and apply the flag conditionally on that?

What exactly would this break? avcodec should just do the right thing.
It would just be slower.
The flag can be re-introduced if a fixed version is released.

- Hendrik
Henrik Gramner Oct. 21, 2018, 2:07 p.m. UTC | #6 
Fixed in x264-sandbox. All uses of plain strtok() will be removed from
x264 in the next push.

I though all of the strtok() uses in x264 had already been converted
to strtok_r() but apparently that wasn't the case. Sorry about that.
Marton Balint Oct. 22, 2018, 12:25 a.m. UTC | #7 
On Sun, 21 Oct 2018, Henrik Gramner wrote:

> Fixed in x264-sandbox. All uses of plain strtok() will be removed from
> x264 in the next push.

Great, thanks! Can you give us an X264_BUILD number from which this will 
be fixed?

Thanks,
Marton
Carl Eugen Hoyos Oct. 22, 2018, 8:30 p.m. UTC | #8 
2018-10-22 2:25 GMT+02:00, Marton Balint <cus@passwd.hu>:
>
> On Sun, 21 Oct 2018, Henrik Gramner wrote:
>
>> Fixed in x264-sandbox. All uses of plain strtok() will be
>> removed from x264 in the next push.
>
> Great, thanks! Can you give us an X264_BUILD number from
> which this will be fixed?

Could you push your patch in the meantime?

Thank you, Carl Eugen
Derek Buitenhuis Oct. 23, 2018, 1:17 p.m. UTC | #9 
Hi,

On 21/10/2018 15:07, Henrik Gramner wrote:
> Fixed in x264-sandbox. All uses of plain strtok() will be removed from
> x264 in the next push.
> 
> I though all of the strtok() uses in x264 had already been converted
> to strtok_r() but apparently that wasn't the case. Sorry about that.

I'd like to point out that this patch or some variant may be required anyway.

libx264 only uses strtok_r or strtok_s if available on the platform.

See: https://git.videolan.org/?p=x264.git;a=blob;f=common/osdep.h;h=715ef8a00c01ad5a94de2b29a422429b9b1f0a53;hb=HEAD#l75

The real fix is to bundle a strtok_r implementation, or to just outright
require strtok_r/strtok_s. Sad, I know.

What platforms that x264 supports don't have it anyway?

- Derek
Henrik Gramner Oct. 23, 2018, 4:51 p.m. UTC | #10 
On Tue, Oct 23, 2018 at 3:22 PM Derek Buitenhuis
<derek.buitenhuis@gmail.com> wrote:
> I'd like to point out that this patch or some variant may be required anyway.
>
> libx264 only uses strtok_r or strtok_s if available on the platform.
>
> See: https://git.videolan.org/?p=x264.git;a=blob;f=common/osdep.h;h=715ef8a00c01ad5a94de2b29a422429b9b1f0a53;hb=HEAD#l75
>
> The real fix is to bundle a strtok_r implementation, or to just outright
> require strtok_r/strtok_s. Sad, I know.
>
> What platforms that x264 supports don't have it anyway?

Honestly no idea, probably something obscure.
Marton Balint Oct. 24, 2018, 9:43 p.m. UTC | #11 
On Tue, 23 Oct 2018, Henrik Gramner wrote:

> On Tue, Oct 23, 2018 at 3:22 PM Derek Buitenhuis
> <derek.buitenhuis@gmail.com> wrote:
>> I'd like to point out that this patch or some variant may be required anyway.
>>
>> libx264 only uses strtok_r or strtok_s if available on the platform.
>>
>> See: https://git.videolan.org/?p=x264.git;a=blob;f=common/osdep.h;h=715ef8a00c01ad5a94de2b29a422429b9b1f0a53;hb=HEAD#l75
>>
>> The real fix is to bundle a strtok_r implementation, or to just outright
>> require strtok_r/strtok_s. Sad, I know.
>>
>> What platforms that x264 supports don't have it anyway?
>
> Honestly no idea, probably something obscure.

Well, then maybe x264 configure should simply fail for those.

I pushed the patch, once the strtok fix is merged to stable and we 
know the build number, the flag can be re-introduced with an #if.

Regards,
Marton
James Almer Oct. 24, 2018, 9:57 p.m. UTC | #12 
On 10/24/2018 6:43 PM, Marton Balint wrote:
> 
> 
> On Tue, 23 Oct 2018, Henrik Gramner wrote:
> 
>> On Tue, Oct 23, 2018 at 3:22 PM Derek Buitenhuis
>> <derek.buitenhuis@gmail.com> wrote:
>>> I'd like to point out that this patch or some variant may be required
>>> anyway.
>>>
>>> libx264 only uses strtok_r or strtok_s if available on the platform.
>>>
>>> See:
>>> https://git.videolan.org/?p=x264.git;a=blob;f=common/osdep.h;h=715ef8a00c01ad5a94de2b29a422429b9b1f0a53;hb=HEAD#l75
>>>
>>>
>>> The real fix is to bundle a strtok_r implementation, or to just outright
>>> require strtok_r/strtok_s. Sad, I know.
>>>
>>> What platforms that x264 supports don't have it anyway?
>>
>> Honestly no idea, probably something obscure.
> 
> Well, then maybe x264 configure should simply fail for those.
> 
> I pushed the patch, once the strtok fix is merged to stable and we know
> the build number, the flag can be re-introduced with an #if.
> 
> Regards,
> Marton

This should be backported to supported branches, or at least 2.8, 3.2,
3.4 and 4.0
Marton Balint Nov. 1, 2018, 10:22 p.m. UTC | #13 
On Wed, 24 Oct 2018, James Almer wrote:

> On 10/24/2018 6:43 PM, Marton Balint wrote:
>> 
>> 
>> On Tue, 23 Oct 2018, Henrik Gramner wrote:
>> 
>>> On Tue, Oct 23, 2018 at 3:22 PM Derek Buitenhuis
>>> <derek.buitenhuis@gmail.com> wrote:
>>>> I'd like to point out that this patch or some variant may be required
>>>> anyway.
>>>>
>>>> libx264 only uses strtok_r or strtok_s if available on the platform.
>>>>
>>>> See:
>>>> https://git.videolan.org/?p=x264.git;a=blob;f=common/osdep.h;h=715ef8a00c01ad5a94de2b29a422429b9b1f0a53;hb=HEAD#l75
>>>>
>>>>
>>>> The real fix is to bundle a strtok_r implementation, or to just outright
>>>> require strtok_r/strtok_s. Sad, I know.
>>>>
>>>> What platforms that x264 supports don't have it anyway?
>>>
>>> Honestly no idea, probably something obscure.
>> 
>> Well, then maybe x264 configure should simply fail for those.
>> 
>> I pushed the patch, once the strtok fix is merged to stable and we know
>> the build number, the flag can be re-introduced with an #if.
>> 
>> Regards,
>> Marton
>
> This should be backported to supported branches, or at least 2.8, 3.2,
> 3.4 and 4.0

I did it for 4.0, I leave the rest for others.

Regards,
Marton
diff mbox

Patch

diff --git a/libavcodec/libx264.c b/libavcodec/libx264.c
index 54e6703d73..d6367bf557 100644
--- a/libavcodec/libx264.c
+++ b/libavcodec/libx264.c
@@ -1063,8 +1063,7 @@  AVCodec ff_libx264_encoder = {
     .priv_class       = &x264_class,
     .defaults         = x264_defaults,
     .init_static_data = X264_init_static,
-    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
-                        FF_CODEC_CAP_INIT_CLEANUP,
+    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
     .wrapper_name     = "libx264",
 };
 #endif
@@ -1115,8 +1114,7 @@  AVCodec ff_libx262_encoder = {
     .priv_class       = &X262_class,
     .defaults         = x264_defaults,
     .pix_fmts         = pix_fmts_8bit,
-    .caps_internal    = FF_CODEC_CAP_INIT_THREADSAFE |
-                        FF_CODEC_CAP_INIT_CLEANUP,
+    .caps_internal    = FF_CODEC_CAP_INIT_CLEANUP,
     .wrapper_name     = "libx264",
 };
 #endif