[FFmpeg-devel] [PATCH] png parser

[Michael Niedermayer]

On Mon, Jul 13, 2009 at 05:01:00PM +0200, Peter Holik wrote:
> Michael Niedermayer schrieb:
> > On Mon, Jul 06, 2009 at 09:17:23AM +0200, Peter Holik wrote:
> >> Michael Niedermayer schrieb:
> >> > On Wed, Jul 01, 2009 at 05:04:51PM +0200, Peter Holik wrote:
> >> >> 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.
> >> >
> >> > no
> >> >
> >> > you already read it out of state(64) and thats what state64 is there
> >> > for, of course you must not store it in state* as a 32bit value rather
> >> > it naturally is in there because state64 represents the last 8 bytes
> >>
> >> I read out of state64 for the signature but not for png chunks.
> >> Inside the for loop is not update of state64.
> >> Well i could misuse state64 for chunk_length but for sourcecode readability
> >> i prefer to use ppc->chunk_length.
> >
> > Iam quite unhappy about that as it is worse readabilitywise IMHO and requires
> > chunk_length to be part of the context vs. a local variable but if its
> > important to you ...
> 
> chunk_length has to be part of the context because if buffer ends with
> the chunk length it can not be a local var.

its in state64


> 
> >> +    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(){
> > }else
> 
> ok done, but i don't know why needed.

because if you write 

if (ppc->index == 3)
    ppc->chunk_length = AV_RL32(&ppc->pc.state) + 4;
else if (ppc->index == 7) {

and later add 1 line, then you get a patch like:

-if (ppc->index == 3)
+if (ppc->index == 3) {
     ppc->chunk_length = AV_RL32(&ppc->pc.state) + 4;
-else if (ppc->index == 7) {
+    ppc->foobar=1;
+} else if (ppc->index == 7) {

vs.

 if (ppc->index == 3) {
     ppc->chunk_length = AV_RL32(&ppc->pc.state) + 4;
+    ppc->foobar=1;
 } else if (ppc->index == 7) {

i think its obvious which is more readable


> 
> >> +            if (AV_RB32(&ppc->pc.state) == MKTAG('I', 'E', 'N', 'D')) {
> >> +                if (ppc->chunk_length >= buf_size - i) {
> >
> >> +                    ppc->remaining_size = ppc->chunk_length - buf_size + i + 1;
> >
> > this can still overflow
> 
> how to prevent a overflow?

id simply check that chunk_length is sane, not sure if that is all that is
needed


[...]
-- 
Michael     GnuPG fingerprint: 9FF2128B147EF6730BADF133611EC787040B0FAB

Let us carefully observe those good qualities wherein our enemies excel us
and endeavor to excel them, by avoiding what is faulty, and imitating what
is excellent in them. -- Plutarch
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.mplayerhq.hu/pipermail/ffmpeg-devel/attachments/20090714/0e8f44f0/attachment.pgp>