[FFmpeg-devel,v4] vaapi_encode_mjpeg: fix bad component id bug

Submitted by U. Artie Eoff on June 7, 2019, 9:45 p.m.

Details

Message ID 20190607214533.20540-1-ullysses.a.eoff@intel.com
State New
Headers show

Commit Message

U. Artie Eoff June 7, 2019, 9:45 p.m.
The compound literals assigned to "components"
only exist within the scope of the if/else
block (thanks Mark Thompson for the better
explanation).

Thus, after this if/else block, "components"
ends up pointing to an arbitrary/undefined
array.  With some compilers and depending on
optimization settings, these arbitrary values
may end up being the same value (i.e. 0 with
GNU GCC 9.x).  Unfortunately, the GNU GCC
compiler, at least, never prints any warnings
about this.

This patch fixes this issue by assigning the
constant arrays to local variables at function
scope and then pointing "components" to those
as necessary.

Fixes #7915

Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
---
 libavcodec/vaapi_encode_mjpeg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Reimar Döffinger June 8, 2019, 6:07 a.m.
On 07.06.2019, at 23:45, "U. Artie Eoff" <ullysses.a.eoff@intel.com> wrote:

> The compound literals assigned to "components"
> only exist within the scope of the if/else
> block (thanks Mark Thompson for the better
> explanation).
> 
> Thus, after this if/else block, "components"
> ends up pointing to an arbitrary/undefined
> array.  With some compilers and depending on
> optimization settings, these arbitrary values
> may end up being the same value (i.e. 0 with
> GNU GCC 9.x).  Unfortunately, the GNU GCC
> compiler, at least, never prints any warnings
> about this.
> 
> This patch fixes this issue by assigning the
> constant arrays to local variables at function
> scope and then pointing "components" to those
> as necessary.
> 
> Fixes #7915
> 
> Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
> ---
> libavcodec/vaapi_encode_mjpeg.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c
> index 4dcdc3d16bb0..bd029cc90315 100644
> --- a/libavcodec/vaapi_encode_mjpeg.c
> +++ b/libavcodec/vaapi_encode_mjpeg.c
> @@ -227,6 +227,8 @@ static int vaapi_encode_mjpeg_init_picture_params(AVCodecContext *avctx,
>     JPEGRawScanHeader                 *sh = &priv->scan.header;
>     VAEncPictureParameterBufferJPEG *vpic = pic->codec_picture_params;
>     const AVPixFmtDescriptor *desc;
> +    const uint8_t components_rgb[3] = { 'R', 'G', 'B' };
> +    const uint8_t components_yuv[3] = {  1,   2,   3  };

Not sure what the necessary scope is for these, but maybe safest to go for "static const"?
U. Artie Eoff June 8, 2019, 6:52 a.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Reimar Döffinger

> Sent: Friday, June 07, 2019 11:07 PM

> To: FFmpeg development discussions and patches <ffmpeg-devel@ffmpeg.org>

> Subject: Re: [FFmpeg-devel] [PATCH v4] vaapi_encode_mjpeg: fix bad component id bug

> 

> On 07.06.2019, at 23:45, "U. Artie Eoff" <ullysses.a.eoff@intel.com> wrote:

> 

> > The compound literals assigned to "components"

> > only exist within the scope of the if/else

> > block (thanks Mark Thompson for the better

> > explanation).

> >

> > Thus, after this if/else block, "components"

> > ends up pointing to an arbitrary/undefined

> > array.  With some compilers and depending on

> > optimization settings, these arbitrary values

> > may end up being the same value (i.e. 0 with

> > GNU GCC 9.x).  Unfortunately, the GNU GCC

> > compiler, at least, never prints any warnings

> > about this.

> >

> > This patch fixes this issue by assigning the

> > constant arrays to local variables at function

> > scope and then pointing "components" to those

> > as necessary.

> >

> > Fixes #7915

> >

> > Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>

> > ---

> > libavcodec/vaapi_encode_mjpeg.c | 6 ++++--

> > 1 file changed, 4 insertions(+), 2 deletions(-)

> >

> > diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c

> > index 4dcdc3d16bb0..bd029cc90315 100644

> > --- a/libavcodec/vaapi_encode_mjpeg.c

> > +++ b/libavcodec/vaapi_encode_mjpeg.c

> > @@ -227,6 +227,8 @@ static int vaapi_encode_mjpeg_init_picture_params(AVCodecContext *avctx,

> >     JPEGRawScanHeader                 *sh = &priv->scan.header;

> >     VAEncPictureParameterBufferJPEG *vpic = pic->codec_picture_params;

> >     const AVPixFmtDescriptor *desc;

> > +    const uint8_t components_rgb[3] = { 'R', 'G', 'B' };

> > +    const uint8_t components_yuv[3] = {  1,   2,   3  };

> 

> Not sure what the necessary scope is for these, but maybe safest to go for "static const"?


I don't think these need scope beyond this function since the values are just
"copied" into the frame header here.  So static would just be an [barely detectable]
optimization.  Unless I am missing something?

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

> 

> To unsubscribe, visit link above, or email

> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Mark Thompson June 12, 2019, 10:08 p.m.
On 07/06/2019 22:45, U. Artie Eoff wrote:
> The compound literals assigned to "components"
> only exist within the scope of the if/else
> block (thanks Mark Thompson for the better
> explanation).
> 
> Thus, after this if/else block, "components"
> ends up pointing to an arbitrary/undefined
> array.  With some compilers and depending on
> optimization settings, these arbitrary values
> may end up being the same value (i.e. 0 with
> GNU GCC 9.x).  Unfortunately, the GNU GCC
> compiler, at least, never prints any warnings
> about this.
> 
> This patch fixes this issue by assigning the
> constant arrays to local variables at function
> scope and then pointing "components" to those
> as necessary.
> 
> Fixes #7915
> 
> Signed-off-by: U. Artie Eoff <ullysses.a.eoff@intel.com>
> ---
>  libavcodec/vaapi_encode_mjpeg.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c
> index 4dcdc3d16bb0..bd029cc90315 100644
> --- a/libavcodec/vaapi_encode_mjpeg.c
> +++ b/libavcodec/vaapi_encode_mjpeg.c
> @@ -227,6 +227,8 @@ static int vaapi_encode_mjpeg_init_picture_params(AVCodecContext *avctx,
>      JPEGRawScanHeader                 *sh = &priv->scan.header;
>      VAEncPictureParameterBufferJPEG *vpic = pic->codec_picture_params;
>      const AVPixFmtDescriptor *desc;
> +    const uint8_t components_rgb[3] = { 'R', 'G', 'B' };
> +    const uint8_t components_yuv[3] = {  1,   2,   3  };
>      const uint8_t *components;
>      int t, i, quant_scale, len;
>  
> @@ -235,9 +237,9 @@ static int vaapi_encode_mjpeg_init_picture_params(AVCodecContext *avctx,
>      desc = av_pix_fmt_desc_get(priv->common.input_frames->sw_format);
>      av_assert0(desc);
>      if (desc->flags & AV_PIX_FMT_FLAG_RGB)
> -        components = (uint8_t[3]) { 'R', 'G', 'B' };
> +        components = components_rgb;
>      else
> -        components = (uint8_t[3]) {  1,   2,   3  };
> +        components = components_yuv;
>  
>      // Frame header.
>  
> 

LGTM; applied.

Thanks!

- Mark
Dominik 'Rathann' Mierzejewski June 13, 2019, 8:42 a.m.
On Friday, 07 June 2019 at 23:45, U. Artie Eoff wrote:
> The compound literals assigned to "components"
> only exist within the scope of the if/else
> block (thanks Mark Thompson for the better
> explanation).
> 
> Thus, after this if/else block, "components"
> ends up pointing to an arbitrary/undefined
> array.  With some compilers and depending on
> optimization settings, these arbitrary values
> may end up being the same value (i.e. 0 with
> GNU GCC 9.x).  Unfortunately, the GNU GCC
> compiler, at least, never prints any warnings
> about this.
> 
> This patch fixes this issue by assigning the
> constant arrays to local variables at function
> scope and then pointing "components" to those
> as necessary.
> 
> Fixes #7915

Brilliant detective work, Artie. Could you open a bug report with gcc
upstream? A case like this should get an explicit warning from gcc like
you pointed out.

Regards,
Dominik
Andreas Rheinhardt June 13, 2019, 9:11 a.m.
Dominik 'Rathann' Mierzejewski:
> On Friday, 07 June 2019 at 23:45, U. Artie Eoff wrote:
>> The compound literals assigned to "components"
>> only exist within the scope of the if/else
>> block (thanks Mark Thompson for the better
>> explanation).
>>
>> Thus, after this if/else block, "components"
>> ends up pointing to an arbitrary/undefined
>> array.  With some compilers and depending on
>> optimization settings, these arbitrary values
>> may end up being the same value (i.e. 0 with
>> GNU GCC 9.x).  Unfortunately, the GNU GCC
>> compiler, at least, never prints any warnings
>> about this.
>>
>> This patch fixes this issue by assigning the
>> constant arrays to local variables at function
>> scope and then pointing "components" to those
>> as necessary.
>>
>> Fixes #7915
> 
> Brilliant detective work, Artie. Could you open a bug report with gcc
> upstream? A case like this should get an explicit warning from gcc like
> you pointed out.
> 
> Regards,
> Dominik
> 
A request for this has already been opened [1].
It seems that this is also responsible for filesystem corruption on
linux when using a SSD cache + HDD combination (see [2]).

- Andreas

[1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89990
[2]: https://bugzilla.kernel.org/show_bug.cgi?id=203573
U. Artie Eoff June 13, 2019, 3:41 p.m.
> -----Original Message-----

> From: ffmpeg-devel [mailto:ffmpeg-devel-bounces@ffmpeg.org] On Behalf Of Andreas Rheinhardt

> Sent: Thursday, June 13, 2019 2:11 AM

> To: ffmpeg-devel@ffmpeg.org

> Subject: Re: [FFmpeg-devel] [PATCH v4] vaapi_encode_mjpeg: fix bad component id bug

> 

> Dominik 'Rathann' Mierzejewski:

> > On Friday, 07 June 2019 at 23:45, U. Artie Eoff wrote:

> >> The compound literals assigned to "components"

> >> only exist within the scope of the if/else

> >> block (thanks Mark Thompson for the better

> >> explanation).

> >>

> >> Thus, after this if/else block, "components"

> >> ends up pointing to an arbitrary/undefined

> >> array.  With some compilers and depending on

> >> optimization settings, these arbitrary values

> >> may end up being the same value (i.e. 0 with

> >> GNU GCC 9.x).  Unfortunately, the GNU GCC

> >> compiler, at least, never prints any warnings

> >> about this.

> >>

> >> This patch fixes this issue by assigning the

> >> constant arrays to local variables at function

> >> scope and then pointing "components" to those

> >> as necessary.

> >>

> >> Fixes #7915

> >

> > Brilliant detective work, Artie. Could you open a bug report with gcc

> > upstream? A case like this should get an explicit warning from gcc like

> > you pointed out.

> >

> > Regards,

> > Dominik

> >

> A request for this has already been opened [1].

> It seems that this is also responsible for filesystem corruption on

> linux when using a SSD cache + HDD combination (see [2]).

> 


According to comments in the gcc request, it looks like we
could use " -fsanitize=address".  Anyone care to test it
out to see if it will work?

> - Andreas

> 

> [1]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89990

> [2]: https://bugzilla.kernel.org/show_bug.cgi?id=203573

> _______________________________________________

> ffmpeg-devel mailing list

> ffmpeg-devel@ffmpeg.org

> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel

> 

> To unsubscribe, visit link above, or email

> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Moritz Barsnick June 14, 2019, 7:14 a.m.
On Thu, Jun 13, 2019 at 15:41:55 +0000, Eoff, Ullysses A wrote:
> According to comments in the gcc request, it looks like we
> could use " -fsanitize=address".  Anyone care to test it
> out to see if it will work?

The AddressSanatizer does not make this a warning at compile time (or
perhaps it may, additionally), but it instruments the binary with code
for analyzing and debugging address accesses, similar to valgrind:

https://github.com/google/sanitizers/wiki/AddressSanitizer

So this is not a permanent solution, but might be something for fate.

Moritz

Patch hide | download patch | download mbox

diff --git a/libavcodec/vaapi_encode_mjpeg.c b/libavcodec/vaapi_encode_mjpeg.c
index 4dcdc3d16bb0..bd029cc90315 100644
--- a/libavcodec/vaapi_encode_mjpeg.c
+++ b/libavcodec/vaapi_encode_mjpeg.c
@@ -227,6 +227,8 @@  static int vaapi_encode_mjpeg_init_picture_params(AVCodecContext *avctx,
     JPEGRawScanHeader                 *sh = &priv->scan.header;
     VAEncPictureParameterBufferJPEG *vpic = pic->codec_picture_params;
     const AVPixFmtDescriptor *desc;
+    const uint8_t components_rgb[3] = { 'R', 'G', 'B' };
+    const uint8_t components_yuv[3] = {  1,   2,   3  };
     const uint8_t *components;
     int t, i, quant_scale, len;
 
@@ -235,9 +237,9 @@  static int vaapi_encode_mjpeg_init_picture_params(AVCodecContext *avctx,
     desc = av_pix_fmt_desc_get(priv->common.input_frames->sw_format);
     av_assert0(desc);
     if (desc->flags & AV_PIX_FMT_FLAG_RGB)
-        components = (uint8_t[3]) { 'R', 'G', 'B' };
+        components = components_rgb;
     else
-        components = (uint8_t[3]) {  1,   2,   3  };
+        components = components_yuv;
 
     // Frame header.