[FFmpeg-devel] Possible crasher bug when decoding unreliable H264 data

[Mark Stevans]

On 6/21/2013 3:24 AM, Michael Niedermayer wrote:
> On Fri, Jun 21, 2013 at 02:21:24AM -0700, Mark Stevans wrote:
>> Sorry I don't have a complete stack trace on this one, but I didn't
>> save it from the last crash, and it takes about 4-6 hours to get the
>> bug to occur again.  And I don't have sample data, because I'm
>> playing a 100 KB/s stream
>> (rtmp://planeta-online.tv:1936/live/channel_22) with FFPlay for
>> hours to get the bug to happen.
>>
>> When playing unreliable H264 streams with FFPlay, I seem to get
>> core-dumps randomly every few hours.  The exact location is usually
>> the second instruction of "pred8x8_top_dc_8_mmxext" in
>> "h264_intrapred.asm", where it dereferences "dest_cr" after
>> subtracting "uvlinesize" from it, as called from the line reading
>>
>>      h->hpc.pred8x8[h->chroma_pred_mode](dest_cr, uvlinesize);
>>
>> in "h264_mb_template.c".  "uvlinesize" is typically something like
>> 320 at the time of crash, with "mb_y" zero.
>>
>> My take on this is that, when presented with garbaged stream data,
>> the H264 frame decoder sometimes tries to perform predictions that
>> involve higher rows (lower memory addresses): if "mb_y" happens to
>> be zero (the top row), this means that it tries to read memory from
>> "negative rows", addresses a few hundred bytes before the beginning
>> of the legitimate frame data.  Often, those addresses point to
>> harmless random bytes, but occasionally it actually points to
>> unmapped memory pages, causing Access Violations.
>>
>> My fix is crude: changing the code to read
>>
>>      if (mb_y <= 0 && (
>>          h->chroma_pred_mode == TOP_DC_PRED8x8 ||
>>          h->chroma_pred_mode == DC_PRED8x8 ||
>>          h->chroma_pred_mode == VERT_PRED8x8)
>>      ) {
>>          av_log(NULL, AV_LOG_WARNING, "Skipping prediction involving
>> previous data rows because mb_y is zero\n");
>>      } else {
>>          h->hpc.pred8x8[h->chroma_pred_mode](dest_cb, uvlinesize);
>>          h->hpc.pred8x8[h->chroma_pred_mode](dest_cr, uvlinesize);
>>      }
>
> Please see ff_h264_check_intra_pred_mode(), It already checks and
> sets a valid mode
> Thus the issue most likely is elsewhere

Michael: assuming I understand the current code (a big assumption), I 
have a proposed patch for "ff_h264_check_intra_pred_mode":

diff --git a/libavcodec/h264.c b/libavcodec/h264.c
index 00e4de8..8b9508b 100644
--- a/libavcodec/h264.c
+++ b/libavcodec/h264.c
@@ -478,43 +478,45 @@ int ff_h264_check_intra4x4_pred_mode(H264Context *h)
   */
  int ff_h264_check_intra_pred_mode(H264Context *h, int mode, int is_chroma)
  {
-    static const int8_t top[7]  = { LEFT_DC_PRED8x8, 1, -1, -1 };
-    static const int8_t left[7] = { TOP_DC_PRED8x8, -1, 2, -1, 
DC_128_PRED8x8 }
;
+    static const int8_t top[7]  = { LEFT_DC_PRED8x8, 1, -1, -1, 4, 
DC_128_PRED8
x8, 6 };
+    static const int8_t left[7] = { TOP_DC_PRED8x8, -1, 2, -1, 
DC_128_PRED8x8,
5, 6 };

      if (mode > 6U) {
          av_log(h->avctx, AV_LOG_ERROR,
-               "out of range intra chroma pred mode at %d %d\n",
-               h->mb_x, h->mb_y);
+               "out of range intra chroma pred mode %d at %d %d\n",
+               mode, h->mb_x, h->mb_y);
          return -1;
      }

+    int new_mode = mode;
+
      if (!(h->top_samples_available & 0x8000)) {
-        mode = top[mode];
-        if (mode < 0) {
+        new_mode = top[new_mode];
+        if (new_mode < 0) {
              av_log(h->avctx, AV_LOG_ERROR,
+                   "top block unavailable for requested intra mode %d 
at %d %d\n",
+                   mode, h->mb_x, h->mb_y);
              return -1;
          }
      }

      if ((h->left_samples_available & 0x8080) != 0x8080) {
-        mode = left[mode];
+        new_mode = left[new_mode];
          if (is_chroma && (h->left_samples_available & 0x8080)) {
              // mad cow disease mode, aka MBAFF + constrained_intra_pred
-            mode = ALZHEIMER_DC_L0T_PRED8x8 +
+            new_mode = ALZHEIMER_DC_L0T_PRED8x8 +
                     (!(h->left_samples_available & 0x8000)) +
-                   2 * (mode == DC_128_PRED8x8);
+                   2 * (new_mode == DC_128_PRED8x8);
          }
-        if (mode < 0) {
+        if (new_mode < 0) {
              av_log(h->avctx, AV_LOG_ERROR,
-                   "left block unavailable for requested intra mode at 
%d %d\n",
-                   h->mb_x, h->mb_y);
+                   "left block unavailable for requested intra mode %d 
at %d %d\n",
+                   mode, h->mb_x, h->mb_y);
              return -1;
          }
      }

-    return mode;
+    return new_mode;
  }

  const uint8_t *ff_h264_decode_nal(H264Context *h, const uint8_t *src,