[FFmpeg-devel] [PATCH] png parser

[Peter Holik]

Michael Niedermayer schrieb:
> On Tue, Jun 30, 2009 at 08:47:48AM +0200, Peter Holik wrote:
>> Michael Niedermayer schrieb:
>> > On Thu, Jun 25, 2009 at 07:55:08PM +0200, Peter Holik wrote:
>> >> Michael Niedermayer schrieb:
>> >> > On Thu, Jun 25, 2009 at 02:14:14PM +0200, Peter Holik wrote:
>> > [...]
>> >
>> >
>> >> +    for (;ppc->pc.frame_start_found && i < buf_size; i++) {
>> >> +        ppc->state = (ppc->state<<8) | buf[i];
>> >
>> > why is this not using the state variable from ParseContext ?
>>
>> ok, now ParseContext state is used
>
> [...]
>
>> +typedef struct PNGParseContext
>> +{
>> +    ParseContext pc;
>> +    uint32_t index;
>
>> +    uint32_t chunk_length;
>
> unneeded, it can be read out of state64, it not needed to be stored in the
> context

you really mean i should misuse an existing field?

last time you told me NOT to do such things.


>> +    uint32_t remaining_size;
>> +} PNGParseContext;
>> +
>> +static int png_parse(AVCodecParserContext *s, AVCodecContext *avctx,
>> +                     const uint8_t **poutbuf, int *poutbuf_size,
>> +                     const uint8_t *buf, int buf_size)
>> +{
>> +    PNGParseContext *ppc = s->priv_data;
>> +    int next = END_NOT_FOUND;
>> +    int i = 0;
>> +
>> +    *poutbuf_size = 0;
>> +    if (buf_size == 0)
>> +        return 0;
>> +
>> +    if (!ppc->pc.frame_start_found) {
>> +        uint64_t state64 = ppc->pc.state64;
>> +        for (; i < buf_size; i++) {
>> +            state64 = (state64 << 8) | buf[i];
>> +            if (state64 == PNGSIG || state64 == MNGSIG) {
>> +                i++;
>> +                ppc->pc.frame_start_found = 1;
>> +                break;
>> +            }
>> +        }
>> +        ppc->pc.state64 = state64;
>> +    } else
>> +        if (ppc->remaining_size) {
>> +            i = FFMIN(ppc->remaining_size, buf_size);
>> +            ppc->remaining_size -= i;
>
>> +            if (ppc->remaining_size)
>> +                goto flush;
>
> unneeded, the for loop between here and flush wont execute for i=buf_size

true, but if ppc->index == -1 then next is set!

also if ppc->remaining_size < buf_size then i is < buf_size

>> +            if (ppc->index == -1) {
>> +                next = i;
>> +                goto flush;
>> +            }
>> +        }
>> +
>> +    for (;ppc->pc.frame_start_found && i < buf_size; i++) {
>> +        ppc->pc.state = (ppc->pc.state<<8) | buf[i];
>> +        if (ppc->index == 3)
>> +            ppc->chunk_length = AV_RL32(&ppc->pc.state) + 4;
>> +        else if (ppc->index == 7) {
>> +            if (AV_RB32(&ppc->pc.state) == MKTAG('I', 'E', 'N', 'D')) {
>
>> +                if (i + ppc->chunk_length >= buf_size) {
>
> i + chunk_length can overflow

will change to

if (ppc->chunk_length >= buf_size - i) {

cu Peter