Opened 14 years ago

Last modified 8 years ago

#264 new defect

Random crashes due to accessing memory beyond allocated size

Reported by: mikulas@… Owned by: reimar
Priority: important Component: vd
Version: unspecified Severity: major
Keywords: MPlayer Cc: diego@…, makovick@…
Blocked By: Blocking:
Reproduced by developer: Analyzed by developer:

Description

Bit operations in libavcodec require that every pointer has additional
FF_INPUT_BUFFER_PADDING_SIZE bytes allocated because optimized routines touch
memory after end. Unfortunatelly sometimes pointer is allocated without this
additional memory.

To trigger the bug, link mplayer with electric fence (you can additionally set
shell variable "EF_ALIGNMENT=1" to get all 1-byte overruns) and play avi files
--- it will crash in UPDATE_CACHE macro in libavcodec.

Without electric fence this bug happens too --- but randomly with very low
probability.

Fix is not trivial, because you have to check all places where init_get_bits is
called and all places that pass pointers to libavcodec, all demuxers etc. that
they pad the stream with FF_INPUT_BUFFER_PADDING_SIZE bytes.

Attachments (2)

MPLAYER-PATCH-OVERFLOW (11.3 KB) - added by mikulas@… 14 years ago.
first proposed patch for overflow problems
MPLAYER-PATCH-ANOTHER (373 bytes) - added by mikulas@… 14 years ago.
patch another reading beyond allocated memory

Download all attachments as: .zip

Change History (10)

comment:1 Changed 14 years ago by diego@…

Forwarded here from Michael Niedermayer:

why not provide some gdb output (backtrace mainly) and say which codec is
used? both audio & video or provide the full and uncut output of mplayer

Changed 14 years ago by mikulas@…

first proposed patch for overflow problems

comment:2 Changed 14 years ago by mikulas@…

The bug happens when playing a file using h263 codec in asf stream. (I won't
post it publicly because of copyright issues)

I tried to spot the buggy places in mplayer and created this patch.

The extradata allocations violate the rules libavcodec/avcodec.h at line 672
---
i.e. they all should be padded with FF_INPUT_BUFFER_PADDING_SIZE bytes. This
causes real bug in h263.c codec.

Another problem is in demux_asf.c file --- when FF_INPUT_BUFFER_PADDING_SIZE
bytes were added to allocation, the problem with bit reader in h263.c went away

and the file played fine.

Other FF_INPUT_BUFFER_PADDING_SIZE changes in demuxers in my patch are just
guesses --- I haven't seen it on real file (I have only mpeg/avi/asf/mov
files).

The if (!trak->stdata) return; in demux_mov.c fixes crash after "* constant
samplesize & variable duration not yet supported!
*
Contact the author if you have such sample file!" message (if anyone wants to
debug it further and add support for this file, contact me).

The other part in demux_mov.c fixes another reading beyond end of allocated
area
found with electric fence --- this happens on files that are normally playable
without efence.

comment:3 Changed 14 years ago by diego@…

  • attachments.isobsolete changed from 0 to 1

This patch is not against CVS and thus useless, since it does not apply
cleanly. If you wish to see this included you should best submit it yourself
on the ffmpeg-devel and mplayer-dev-eng mailing lists.

Changed 14 years ago by mikulas@…

patch another reading beyond allocated memory

comment:4 Changed 14 years ago by mikulas@…

Even if it applied correctly against CVS mplayer, someone still should check
that padding is made on all paths to libavcodec --- I don't claim I fixed
everything, I fixed only what I hit or what I thought could be bugs.

BTW. --- here is another patch for crash with -lefence when playing mov files
with proprietary audio codec --- function expCreatePalette reads too much data
from buffer provided by the codec and crashes with ElectricFence?.

comment:5 Changed 13 years ago by diego@…

  • Cc diego@… added
  • op_sys changed from Linux to All
  • Owner changed from diego@… to makovick@…

The first patch has been applied by Jindrich Mackovicka. The second patch is
still pending.

comment:6 Changed 12 years ago by diego@…

  • Cc makovick@… added
  • Owner changed from makovick@… to mru@…

comment:7 Changed 12 years ago by mans@…

  • Component changed from libavcodec to vd
  • Keywords MPlayer added; FFmpeg removed
  • Owner changed from mru@… to r_togni@…
  • product changed from FFmpeg to MPlayer

The first patch has been applied. The other one belongs to mplayer. Reassigning.

comment:8 Changed 8 years ago by compn

  • Owner changed from r_togni@… to reimar
Note: See TracTickets for help on using tickets.