| Message ID | 20181020103540.22452-1-cus@passwd.hu |
|---|---|
| State | Accepted |
| Commit | b02490a497009064b7f192802aa246aa0b6a4dad |
| Headers | show |
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
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
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?
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
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
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.
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
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
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
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.
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
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
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 --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
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(-)