Opened 5 years ago

Closed 5 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 cehoyos)

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 cehoyos 5 years ago.

Download all attachments as: .zip

Change History (9)

comment:1 Changed 5 years ago by mihnea

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 Changed 5 years ago by cehoyos

Can you provide a sample that overflows?

comment:3 Changed 5 years ago by cehoyos

  • Description modified (diff)

Changed 5 years ago by cehoyos

comment:4 Changed 5 years ago by cehoyos

Is attached patchdvboverflow.diff sufficient to fix the problem?

comment:5 Changed 5 years ago by mihnea

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 Changed 5 years ago by cehoyos

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 Changed 5 years ago by mihnea

Everything works fine after those commits, thanks.

comment:8 Changed 5 years ago by cehoyos

  • Resolution set to fixed
  • Status changed from new to closed

Thank you for testing again!

Note: See TracTickets for help on using tickets.