Opened 12 years ago

Closed 12 years ago

#554 closed defect (fixed)

Buffer overflow in dvbsubdec.c

Reported by: Mihnea Owned by:
Priority: normal Component: avcodec
Version: git-master Keywords: dvbsub
Cc: Blocked By:
Blocking: Reproduced by developer: no
Analyzed by developer: no

Description (last modified by Carl Eugen Hoyos)

Hi.

The function dvbsub_parse_pixel_data_block() in libavcodec/dvbsubdec.c is prone to overflowing the region->pbuf buffer. That buffer is region->width*region->height bytes in length, but the check for overflow is done like this:

if (x_pos > region->width || y_pos > region->height)

The comparisons should obviously use greater than equal instead of greater, since you never want to write at region->height * region->width + something. However, if I change them, the "invalid object location" message triggers all the time because y_pos is incremented a few lines above like this:

if ((y_pos & 1) != top_bottom)
    y_pos++;

I suppose this is trying to align the starting line to odd or even to account for interlacing. I'm not sure how this works for progressive streams since I don't know anything about how DVB subtitles are encoded, but with a progressive stream it always reaches this piece of code with y_pos = region->height - 1, so the increment makes y_pos invalid, causing a buffer overflow with the current code, or triggering the error message if the comparison is fixed.

Attachments (1)

patchdvboverflow.diff (897 bytes ) - added by Carl Eugen Hoyos 12 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 by Mihnea, 12 years ago

Awesome, trac thought the or operator is a format hint. The first bit of code should read:

if (x_pos > region->width || y_pos > region->height)

comment:2 by Carl Eugen Hoyos, 12 years ago

Can you provide a sample that overflows?

comment:3 by Carl Eugen Hoyos, 12 years ago

Description: modified (diff)

by Carl Eugen Hoyos, 12 years ago

Attachment: patchdvboverflow.diff added

comment:4 by Carl Eugen Hoyos, 12 years ago

Is attached patchdvboverflow.diff sufficient to fix the problem?

comment:5 by Mihnea, 12 years ago

Unfortunately I can't provide a sample, the stream which triggers it comes though a VPN (it's supposed to go in a set top box).

The patch will fix the problem, assuming y positions are 1-based (or that the height specified in the region segment is 1 pixel shorter than the actual height).

comment:6 by Carl Eugen Hoyos, 12 years ago

Some fixes from Julian Gardner were committed yesterday, could you please test if the problems you saw are still reproducible with current git head?

comment:7 by Mihnea, 12 years ago

Everything works fine after those commits, thanks.

comment:8 by Carl Eugen Hoyos, 12 years ago

Resolution: fixed
Status: newclosed

Thank you for testing again!

Note: See TracTickets for help on using tickets.