diff mbox

[FFmpeg-devel] libavcodec/h264_parse: don't use uninitialized value when chroma_format_idc==0

Message ID 20170907134207.27809-1-wachsler@google.com
State Accepted
Commit fde5c7dc79eb017790ba232442ad2a4eecea4bf1
Headers show

Commit Message

Mark Wachsler Sept. 7, 2017, 1:42 p.m. UTC 
When parsing a monochrome file, chroma_log2_weight_denom was used without
being initialized, which could lead to a bogus error message being printed, e.g.
  [h264 @ 0x61a000026480] chroma_log2_weight_denom 24576 is out of range
It also could led to warnings using AddressSanitizer.
---
 libavcodec/h264_parse.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

Comments

Mark Wachsler Sept. 7, 2017, 1:48 p.m. UTC | #1 
I have updated my patch to also fix the references to chroma_weight in the
MBAFF case, as suggested by Mark Thompson, but without erroring out on
invalid values.

On Thu, Sep 7, 2017 at 9:42 AM, Mark Wachsler <wachsler@google.com> wrote:

> When parsing a monochrome file, chroma_log2_weight_denom was used without
> being initialized, which could lead to a bogus error message being
> printed, e.g.
>   [h264 @ 0x61a000026480] chroma_log2_weight_denom 24576 is out of range
> It also could led to warnings using AddressSanitizer.
> ---
>  libavcodec/h264_parse.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
>
> diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c
> index 3d20075f6a..a7c71d9bbb 100644
> --- a/libavcodec/h264_parse.c
> +++ b/libavcodec/h264_parse.c
> @@ -34,21 +34,22 @@ int ff_h264_pred_weight_table(GetBitContext *gb,
> const SPS *sps,
>
>      pwt->use_weight             = 0;
>      pwt->use_weight_chroma      = 0;
> -    pwt->luma_log2_weight_denom = get_ue_golomb(gb);
> -    if (sps->chroma_format_idc)
> -        pwt->chroma_log2_weight_denom = get_ue_golomb(gb);
>
> +    pwt->luma_log2_weight_denom = get_ue_golomb(gb);
>      if (pwt->luma_log2_weight_denom > 7U) {
>          av_log(logctx, AV_LOG_ERROR, "luma_log2_weight_denom %d is out of
> range\n", pwt->luma_log2_weight_denom);
>          pwt->luma_log2_weight_denom = 0;
>      }
> -    if (pwt->chroma_log2_weight_denom > 7U) {
> -        av_log(logctx, AV_LOG_ERROR, "chroma_log2_weight_denom %d is out
> of range\n", pwt->chroma_log2_weight_denom);
> -        pwt->chroma_log2_weight_denom = 0;
> -    }
> +    luma_def = 1 << pwt->luma_log2_weight_denom;
>
> -    luma_def   = 1 << pwt->luma_log2_weight_denom;
> -    chroma_def = 1 << pwt->chroma_log2_weight_denom;
> +    if (sps->chroma_format_idc) {
> +        pwt->chroma_log2_weight_denom = get_ue_golomb(gb);
> +        if (pwt->chroma_log2_weight_denom > 7U) {
> +            av_log(logctx, AV_LOG_ERROR, "chroma_log2_weight_denom %d is
> out of range\n", pwt->chroma_log2_weight_denom);
> +            pwt->chroma_log2_weight_denom = 0;
> +        }
> +        chroma_def = 1 << pwt->chroma_log2_weight_denom;
> +    }
>
>      for (list = 0; list < 2; list++) {
>          pwt->luma_weight_flag[list]   = 0;
> @@ -102,9 +103,11 @@ int ff_h264_pred_weight_table(GetBitContext *gb,
> const SPS *sps,
>              if (picture_structure == PICT_FRAME) {
>                  pwt->luma_weight[16 + 2 * i][list][0] =
> pwt->luma_weight[16 + 2 * i + 1][list][0] = pwt->luma_weight[i][list][0];
>                  pwt->luma_weight[16 + 2 * i][list][1] =
> pwt->luma_weight[16 + 2 * i + 1][list][1] = pwt->luma_weight[i][list][1];
> -                for (j = 0; j < 2; j++) {
> -                    pwt->chroma_weight[16 + 2 * i][list][j][0] =
> pwt->chroma_weight[16 + 2 * i + 1][list][j][0] =
> pwt->chroma_weight[i][list][j][0];
> -                    pwt->chroma_weight[16 + 2 * i][list][j][1] =
> pwt->chroma_weight[16 + 2 * i + 1][list][j][1] =
> pwt->chroma_weight[i][list][j][1];
> +                if (sps->chroma_format_idc) {
> +                    for (j = 0; j < 2; j++) {
> +                        pwt->chroma_weight[16 + 2 * i][list][j][0] =
> pwt->chroma_weight[16 + 2 * i + 1][list][j][0] =
> pwt->chroma_weight[i][list][j][0];
> +                        pwt->chroma_weight[16 + 2 * i][list][j][1] =
> pwt->chroma_weight[16 + 2 * i + 1][list][j][1] =
> pwt->chroma_weight[i][list][j][1];
> +                    }
>                  }
>              }
>          }
> --
> 2.14.1.581.gf28d330327-goog
>
>
Michael Niedermayer Sept. 8, 2017, 12:49 a.m. UTC | #2 
On Thu, Sep 07, 2017 at 09:42:07AM -0400, Mark Wachsler wrote:
> When parsing a monochrome file, chroma_log2_weight_denom was used without
> being initialized, which could lead to a bogus error message being printed, e.g.
>   [h264 @ 0x61a000026480] chroma_log2_weight_denom 24576 is out of range
> It also could led to warnings using AddressSanitizer.
> ---
>  libavcodec/h264_parse.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)

applied

thanks

[...]
diff mbox

Patch

diff --git a/libavcodec/h264_parse.c b/libavcodec/h264_parse.c
index 3d20075f6a..a7c71d9bbb 100644
--- a/libavcodec/h264_parse.c
+++ b/libavcodec/h264_parse.c
@@ -34,21 +34,22 @@  int ff_h264_pred_weight_table(GetBitContext *gb, const SPS *sps,
 
     pwt->use_weight             = 0;
     pwt->use_weight_chroma      = 0;
-    pwt->luma_log2_weight_denom = get_ue_golomb(gb);
-    if (sps->chroma_format_idc)
-        pwt->chroma_log2_weight_denom = get_ue_golomb(gb);
 
+    pwt->luma_log2_weight_denom = get_ue_golomb(gb);
     if (pwt->luma_log2_weight_denom > 7U) {
         av_log(logctx, AV_LOG_ERROR, "luma_log2_weight_denom %d is out of range\n", pwt->luma_log2_weight_denom);
         pwt->luma_log2_weight_denom = 0;
     }
-    if (pwt->chroma_log2_weight_denom > 7U) {
-        av_log(logctx, AV_LOG_ERROR, "chroma_log2_weight_denom %d is out of range\n", pwt->chroma_log2_weight_denom);
-        pwt->chroma_log2_weight_denom = 0;
-    }
+    luma_def = 1 << pwt->luma_log2_weight_denom;
 
-    luma_def   = 1 << pwt->luma_log2_weight_denom;
-    chroma_def = 1 << pwt->chroma_log2_weight_denom;
+    if (sps->chroma_format_idc) {
+        pwt->chroma_log2_weight_denom = get_ue_golomb(gb);
+        if (pwt->chroma_log2_weight_denom > 7U) {
+            av_log(logctx, AV_LOG_ERROR, "chroma_log2_weight_denom %d is out of range\n", pwt->chroma_log2_weight_denom);
+            pwt->chroma_log2_weight_denom = 0;
+        }
+        chroma_def = 1 << pwt->chroma_log2_weight_denom;
+    }
 
     for (list = 0; list < 2; list++) {
         pwt->luma_weight_flag[list]   = 0;
@@ -102,9 +103,11 @@  int ff_h264_pred_weight_table(GetBitContext *gb, const SPS *sps,
             if (picture_structure == PICT_FRAME) {
                 pwt->luma_weight[16 + 2 * i][list][0] = pwt->luma_weight[16 + 2 * i + 1][list][0] = pwt->luma_weight[i][list][0];
                 pwt->luma_weight[16 + 2 * i][list][1] = pwt->luma_weight[16 + 2 * i + 1][list][1] = pwt->luma_weight[i][list][1];
-                for (j = 0; j < 2; j++) {
-                    pwt->chroma_weight[16 + 2 * i][list][j][0] = pwt->chroma_weight[16 + 2 * i + 1][list][j][0] = pwt->chroma_weight[i][list][j][0];
-                    pwt->chroma_weight[16 + 2 * i][list][j][1] = pwt->chroma_weight[16 + 2 * i + 1][list][j][1] = pwt->chroma_weight[i][list][j][1];
+                if (sps->chroma_format_idc) {
+                    for (j = 0; j < 2; j++) {
+                        pwt->chroma_weight[16 + 2 * i][list][j][0] = pwt->chroma_weight[16 + 2 * i + 1][list][j][0] = pwt->chroma_weight[i][list][j][0];
+                        pwt->chroma_weight[16 + 2 * i][list][j][1] = pwt->chroma_weight[16 + 2 * i + 1][list][j][1] = pwt->chroma_weight[i][list][j][1];
+                    }
                 }
             }
         }