Buffer overflow in dvbsubdec.c

[mihnea]

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.

[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)

[cehoyos]

Can you provide a sample that overflows?

[cehoyos]

Is attached patchdvboverflow.diff sufficient to fix the problem?

[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).

[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?

[mihnea]

Everything works fine after those commits, thanks.

[cehoyos]

Thank you for testing again!