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

[Michael Niedermayer]

On Fri, Jun 21, 2013 at 06:21:59AM -0700, Mark Stevans wrote:
> 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),

function fixed

-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Many things microsoft did are stupid, but not doing something just because
microsoft did it is even more stupid. If everything ms did were stupid they
would be bankrupt already.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://ffmpeg.org/pipermail/ffmpeg-devel/attachments/20130621/d1e280ef/attachment.asc>