[9fans] pngread: alloc chunk's length

[9fans] pngread: alloc chunk's length

[adr via 9fans]

Hi,

Png is using a fix size to allocate space for the chunks. I noticed
it because it couldn't open some png files (the chunk size was
bigger than IDATSIZE).

This patch removes IDATSIZE and instead makes png to allocate the
size of the chunk before reading it.


--- sys/src/cmd/jpg/readpng.c   Thu Jan 24 23:39:55 2013
+++ /sys/src/cmd/jpg/readpng.c  Sat Jul 10 13:09:13 2021
@@ -10,8 +10,6 @@
 
 enum
 {
-       IDATSIZE = 1000000,
-
        /* filtering algorithms */
        FilterNone =    0,      /* new[x][y] = buf[x][y] */
        FilterSub =     1,      /* new[x][y] = buf[x][y] + new[x-1][y] */ 
@@ -51,7 +49,6 @@
 struct ZlibR
 {
        Biobuf *io;             /* input buffer */
-       uchar *buf;             /* malloc'ed staging buffer */
        uchar *p;                       /* next byte to decompress */
        uchar *e;                       /* end of buffer */
        ZlibW *w;
@@ -94,19 +91,26 @@
 }
 
 static int
-getchunk(Biobuf *b, char *type, uchar *d, int m)
+chunklen(Biobuf *b)
 {
-       uchar buf[8];
+       uchar buf[4];
+
+       if(Bread(b, buf, 4) != 4)
+               return -1;
+       return get4(buf);
+}
+
+static int
+getchunk(Biobuf *b, char *type, uchar *d, int n)
+{
+       uchar buf[4];
        ulong crc = 0, crc2;
-       int n, nr;
+       int nr;
 
-       if(Bread(b, buf, 8) != 8)
+       if(Bread(b, buf, 4) != 4)
                return -1;
-       n = get4(buf);
-       memmove(type, buf+4, 4);
+       memmove(type, buf, 4);
        type[4] = 0;
-       if(n > m)
-               sysfatal("getchunk needed %d, had %d", n, m);
        nr = Bread(b, d, n);
        if(nr != n)
                sysfatal("getchunk read %d, expected %d", nr, n);
@@ -117,7 +121,7 @@
        crc2 = get4(buf);
        if(crc != crc2)
                sysfatal("getchunk crc failed");
-       return n;
+       return 0;
 }
 
 static int
@@ -129,25 +133,31 @@
 
        if(z->p >= z->e){
        Again:
-               z->p = z->buf;
+               n = chunklen(z->io);
+               if(n < 0)
+                       return -1;
+               z->p = pngmalloc(n, 0);
                z->e = z->p;
-               n = getchunk(z->io, type, z->p, IDATSIZE);
-               if(n < 0 || strcmp(type, "IEND") == 0)
+               getchunk(z->io, type, z->p, n);
+               if(strcmp(type, "IEND") == 0){
+                       free(z->p);
                        return -1;
+               }
                z->e = z->p + n;
                if(!strcmp(type,"PLTE")){
                        if(n < 3 || n > 3*256 || n%3)
                                sysfatal("invalid PLTE chunk len %d", n);
                        memcpy(z->w->palette, z->p, n);
                        z->w->palsize = 256;
+                       free(z->p);
                        goto Again;
                }
-               if(type[0] & PropertyBit)
+               if(type[0] & PropertyBit){
+                       free(z->p);
                        goto Again;  /* skip auxiliary chunks fornow */
-               if(strcmp(type,"IDAT")){
-                       sysfatal("unrecognized mandatory chunk %s", type);
-                       goto Again;
                }
+               if(strcmp(type,"IDAT"))
+                       sysfatal("unrecognized mandatory chunk %s", type);
        }
        return *z->p++;
 }
@@ -383,18 +393,23 @@
 {
        char type[5];
        int bpc, colorfmt, dx, dy, err, n, nchan, nout, useadam7;
-       uchar *buf, *h;
+       uchar *buf, *mag, *h;
        Rawimage *image;
        ZlibR zr;
        ZlibW zw;
 
-       buf = pngmalloc(IDATSIZE, 0);
-       if(Bread(b, buf, sizeof PNGmagic) != sizeof PNGmagic ||
-           memcmp(PNGmagic, buf, sizeof PNGmagic) != 0)
+       mag = pngmalloc(sizeof PNGmagic, 0);
+       if(Bread(b, mag, sizeof PNGmagic) != sizeof PNGmagic ||
+           memcmp(PNGmagic, mag, sizeof PNGmagic) != 0)
                sysfatal("bad PNGmagic");
+       free(mag);
 
-       n = getchunk(b, type, buf, IDATSIZE);
-       if(n < 13 || strcmp(type,"IHDR") != 0)
+       n = chunklen(b);
+       if(n < 0)
+               sysfatal("missing IHDR chunk");
+       buf = pngmalloc(n, 0);
+       getchunk(b, type, buf, n);
+       if(strcmp(type,"IHDR") != 0)
                sysfatal("missing IHDR chunk");
        h = buf;
        dx = get4(h);
@@ -460,7 +475,7 @@
        memset(&zr, 0, sizeof zr);
        zr.w = &zw;
        zr.io = b;
-       zr.buf = buf;
+       free(buf);
 
        memset(&zw, 0, sizeof zw);
        if(useadam7)
@@ -483,7 +498,6 @@
        if(err)
                sysfatal("inflatezlib %s\n", flateerr(err));
 
-       free(buf);
        free(zw.scan);
        free(zw.lastscan);
        return image;

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-Mdb82973047397d68fdedb7c3
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[adr via 9fans]

Checking the sent mail I noticed that I forgot to remove mag...

--- sys/src/cmd/jpg/readpng.c   Thu Jan 24 23:39:55 2013
+++ /sys/src/cmd/jpg/readpng.c  Sat Jul 10 13:09:13 2021
@@ -10,8 +10,6 @@
 
 enum
 {
-       IDATSIZE = 1000000,
-
        /* filtering algorithms */
        FilterNone =    0,      /* new[x][y] = buf[x][y] */
        FilterSub =     1,      /* new[x][y] = buf[x][y] + new[x-1][y] */ 
@@ -51,7 +49,6 @@
 struct ZlibR
 {
        Biobuf *io;             /* input buffer */
-       uchar *buf;             /* malloc'ed staging buffer */
        uchar *p;                       /* next byte to decompress */
        uchar *e;                       /* end of buffer */
        ZlibW *w;
@@ -94,19 +91,26 @@
 }
 
 static int
-getchunk(Biobuf *b, char *type, uchar *d, int m)
+chunklen(Biobuf *b)
 {
-       uchar buf[8];
+       uchar buf[4];
+
+       if(Bread(b, buf, 4) != 4)
+               return -1;
+       return get4(buf);
+}
+
+static int
+getchunk(Biobuf *b, char *type, uchar *d, int n)
+{
+       uchar buf[4];
        ulong crc = 0, crc2;
-       int n, nr;
+       int nr;
 
-       if(Bread(b, buf, 8) != 8)
+       if(Bread(b, buf, 4) != 4)
                return -1;
-       n = get4(buf);
-       memmove(type, buf+4, 4);
+       memmove(type, buf, 4);
        type[4] = 0;
-       if(n > m)
-               sysfatal("getchunk needed %d, had %d", n, m);
        nr = Bread(b, d, n);
        if(nr != n)
                sysfatal("getchunk read %d, expected %d", nr, n);
@@ -117,7 +121,7 @@
        crc2 = get4(buf);
        if(crc != crc2)
                sysfatal("getchunk crc failed");
-       return n;
+       return 0;
 }
 
 static int
@@ -129,25 +133,31 @@
 
        if(z->p >= z->e){
        Again:
-               z->p = z->buf;
+               n = chunklen(z->io);
+               if(n < 0)
+                       return -1;
+               z->p = pngmalloc(n, 0);
                z->e = z->p;
-               n = getchunk(z->io, type, z->p, IDATSIZE);
-               if(n < 0 || strcmp(type, "IEND") == 0)
+               getchunk(z->io, type, z->p, n);
+               if(strcmp(type, "IEND") == 0){
+                       free(z->p);
                        return -1;
+               }
                z->e = z->p + n;
                if(!strcmp(type,"PLTE")){
                        if(n < 3 || n > 3*256 || n%3)
                                sysfatal("invalid PLTE chunk len %d", n);
                        memcpy(z->w->palette, z->p, n);
                        z->w->palsize = 256;
+                       free(z->p);
                        goto Again;
                }
-               if(type[0] & PropertyBit)
+               if(type[0] & PropertyBit){
+                       free(z->p);
                        goto Again;  /* skip auxiliary chunks fornow */
-               if(strcmp(type,"IDAT")){
-                       sysfatal("unrecognized mandatory chunk %s", type);
-                       goto Again;
                }
+               if(strcmp(type,"IDAT"))
+                       sysfatal("unrecognized mandatory chunk %s", type);
        }
        return *z->p++;
 }
@@ -388,13 +398,18 @@
        ZlibR zr;
        ZlibW zw;
 
-       buf = pngmalloc(IDATSIZE, 0);
+       buf = pngmalloc(sizeof PNGmagic, 0);
        if(Bread(b, buf, sizeof PNGmagic) != sizeof PNGmagic ||
            memcmp(PNGmagic, buf, sizeof PNGmagic) != 0)
                sysfatal("bad PNGmagic");
+       free(buf);
 
-       n = getchunk(b, type, buf, IDATSIZE);
-       if(n < 13 || strcmp(type,"IHDR") != 0)
+       n = chunklen(b);
+       if(n < 0)
+               sysfatal("missing IHDR chunk");
+       buf = pngmalloc(n, 0);
+       getchunk(b, type, buf, n);
+       if(strcmp(type,"IHDR") != 0)
                sysfatal("missing IHDR chunk");
        h = buf;
        dx = get4(h);
@@ -460,7 +475,7 @@
        memset(&zr, 0, sizeof zr);
        zr.w = &zw;
        zr.io = b;
-       zr.buf = buf;
+       free(buf);
 
        memset(&zw, 0, sizeof zw);
        if(useadam7)
@@ -483,7 +498,6 @@
        if(err)
                sysfatal("inflatezlib %s\n", flateerr(err));
 
-       free(buf);
        free(zw.scan);
        free(zw.lastscan);
        return image;

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-Mee347a801f72833f6f766ff2
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[ori]

Quoth adr via 9fans <9fans@9fans.net>:
> Checking the sent mail I noticed that I forgot to remove mag...

Why not make getchunk allocate? Somethign like:

--- //.git/fs/object/e8259861da3a55c03491904e4d11c5c15b7577c5/tree/sys/src/cmd/jpg/readpng.c
+++ sys/src/cmd/jpg/readpng.c
@@ -94,7 +94,7 @@
 }
 
 static int
-getchunk(Biobuf *b, char *type, uchar *d, int m)
+getchunk(Biobuf *b, char *type, uchar **d)
 {
        uchar buf[8];
        ulong crc = 0, crc2;
@@ -103,11 +103,10 @@
        if(Bread(b, buf, 8) != 8)
                return -1;
        n = get4(buf);
+       *d = pngmalloc(n, 0);
        memmove(type, buf+4, 4);
        type[4] = 0;
-       if(n > m)
-               sysfatal("getchunk needed %d, had %d", n, m);
-       nr = Bread(b, d, n);
+       nr = Bread(b, *d, n);
        if(nr != n)
                sysfatal("getchunk read %d, expected %d", nr, n);
        crc = blockcrc(crctab, crc, type, 4);
@@ -131,7 +130,7 @@
        Again:
                z->p = z->buf;
                z->e = z->p;
-               n = getchunk(z->io, type, z->p, IDATSIZE);
+               n = getchunk(z->io, type, &z->p);
                if(n < 0 || strcmp(type, "IEND") == 0)
                        return -1;
                z->e = z->p + n;


------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-Md2aafc19d425830aec9c5ea9
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[ori]

Quoth adr via 9fans <9fans@9fans.net>:
> because it couldn't open some png files

Do you have an example?


------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-Mfc030872ffba9972709a61bd
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[adr via 9fans]

On Mon, 12 Jul 2021, ori@eigenstate.org wrote:
> Why not make getchunk allocate? Somethign like:
> 
> ---
> //.git/fs/object/e8259861da3a55c03491904e4d11c5c15b7577c5/tree/sys/src/cmd/jpg/readpng.c
> +++ sys/src/cmd/jpg/readpng.c
> @@ -94,7 +94,7 @@
> }
>
> static int
> -getchunk(Biobuf *b, char *type, uchar *d, int m)
> +getchunk(Biobuf *b, char *type, uchar **d)
> {
>        uchar buf[8];
>        ulong crc = 0, crc2;
> @@ -103,11 +103,10 @@
>        if(Bread(b, buf, 8) != 8)
>                return -1;
>        n = get4(buf);
> +       *d = pngmalloc(n, 0);
>        memmove(type, buf+4, 4);
>        type[4] = 0;
> -       if(n > m)
> -               sysfatal("getchunk needed %d, had %d", n, m);
> -       nr = Bread(b, d, n);
> +       nr = Bread(b, *d, n);
>        if(nr != n)
>                sysfatal("getchunk read %d, expected %d", nr, n);
>        crc = blockcrc(crctab, crc, type, 4);
> @@ -131,7 +130,7 @@
>        Again:
>                z->p = z->buf;
>                z->e = z->p;
> -               n = getchunk(z->io, type, z->p, IDATSIZE);
> +               n = getchunk(z->io, type, &z->p);
>                if(n < 0 || strcmp(type, "IEND") == 0)
>                        return -1;
>                z->e = z->p + n;
>
>

Hi Ori,

I thought that it would be a good idea to have a separate function
to just get the size without squeezing the fs through all the chunk,
it could be useful in the future. getchunk() could call it, but I
didn't see the point. I imagine that this is not a complete patch
but a suggestion, but anyway remember that you don't need z->buf
any more, and don't forget to free z->p for the next iteration.

By the way after revisiting readpng.c I noticed that I checked for
a negative value of the IHDR chunk length, better do like in the
original code:

[...]
        n = chunklen(b);
        if(n < 13)
                sysfatal("missing IHDR chunk");
[...]

Regards,
adr.

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-M36acff490ae45918672f52bd
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[adr via 9fans]

On Mon, 12 Jul 2021, ori@eigenstate.org wrote:
> Do you have an example?

I could upload it in some place but you don't need it, really. The
specification is clear, the length of a data chunk must be less
than 2^31 - 1, and the complete image data is represented by a
single zlib datastream that is stored in "some" number of IDAT
chunks.  That's all. A decoder can't make any other assumptions
about the size or the boundaries of the chunks, of course correct
me if I'm wrong.

Anyway if you are curious they were music scores extracted from a
pdf using mutool (mupdf).

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-Me409a3585b63dd2fc735d4b7
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[hiro]

it's always useful to have the testcase available, for others and for
possible future regressions

On 7/12/21, adr via 9fans <9fans@9fans.net> wrote:
> On Mon, 12 Jul 2021, ori@eigenstate.org wrote:
>> Do you have an example?
> 
> I could upload it in some place but you don't need it, really. The
> specification is clear, the length of a data chunk must be less
> than 2^31 - 1, and the complete image data is represented by a
> single zlib datastream that is stored in "some" number of IDAT
> chunks.  That's all. A decoder can't make any other assumptions
> about the size or the boundaries of the chunks, of course correct
> me if I'm wrong.
> 
> Anyway if you are curious they were music scores extracted from a
> pdf using mutool (mupdf).

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-Mfc53364915e09f1690f70f0e
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[adr via 9fans]

On Mon, 12 Jul 2021, hiro wrote:

> Date: Mon, 12 Jul 2021 20:04:23 +0200
> From: hiro <23hiro@gmail.com>
> Reply-To: 9fans <9fans@9fans.net>
> To: 9fans <9fans@9fans.net>
> Subject: Re: [9fans] pngread: alloc chunk's length
> 
> it's always useful to have the testcase available, for others and for
> possible future regressions

There is nothing to test, really, the length of an IDAT chunk
can't be fixed (At least you want to allocate more than 2GB...)
If a png file has a IDAT bigger than your constant, you are screwed.

You haven't noticed because the majority of encoders use small
chunks, but that doesn't mean that the code is right, is not. If
you want to test with a png file anyway, take a big image and use
some tool like
   http://optipng.sourceforge.net

The resulting file will be possibly smaller, but the data stream will
be encoded in one IDAT chunk to save space and reduce the overhead
of processing numerous IDAT chunks.

But you'll just get

getchunk needed xxxxxxxx, had 1000000

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-Mcaf942981401d70c27d13d7e
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[adr via 9fans]

On Mon, 12 Jul 2021, adr via 9fans wrote:
[...]
> the length of a data chunk must be less than 2^31 - 1
[...]
> can't be fixed (At least you want to allocate more than 2GB...)
[...]
Well... to be exact here, it can't be bigger than 2^31 - 1, so there
are "almost" 2GB.

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-Md3f857b8a239314394f2a9db
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[hiro]

are you saying this is a purely synthetic error, it doesn't happen in
the wild bec. these sizes are normally more sane?

On 7/13/21, adr via 9fans <9fans@9fans.net> wrote:
> On Mon, 12 Jul 2021, adr via 9fans wrote:
> [...]
>> the length of a data chunk must be less than 2^31 - 1
> [...]
>> can't be fixed (At least you want to allocate more than 2GB...)
> [...]
> Well... to be exact here, it can't be bigger than 2^31 - 1, so there
> are "almost" 2GB.

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-M00044430cb463550570c3bd5
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[adr via 9fans]

On Tue, 13 Jul 2021, hiro wrote:
> are you saying this is a purely synthetic error, it doesn't happen in
> the wild bec. these sizes are normally more sane?

No, no... you got it wrong. You have to follow the specification
of the format, unless you want to have surprises like this one.

The file you'll obtain with the example I gave you will be as
correct as any other png file.

Regards,
adr.

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-M1c74c38625f5a5b11071043e
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[hiro]

you seem to propose that if the png tells us to then we should
allocate 2GB per chunk, just bec. the spec allows it

even if the spec doesn't tell us a limit, we might want to have a limit.

On 7/13/21, adr via 9fans <9fans@9fans.net> wrote:
> On Tue, 13 Jul 2021, hiro wrote:
>> are you saying this is a purely synthetic error, it doesn't happen in
>> the wild bec. these sizes are normally more sane?
> 
> No, no... you got it wrong. You have to follow the specification
> of the format, unless you want to have surprises like this one.
> 
> The file you'll obtain with the example I gave you will be as
> correct as any other png file.
> 
> Regards,
> adr.

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-M76541e57925480ab6206e560
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[adr via 9fans]

On Tue, 13 Jul 2021, hiro wrote:
> you seem to propose that if the png tells us to then we should
> allocate 2GB per chunk, just bec. the spec allows it
>
> even if the spec doesn't tell us a limit, we might want to have a limit.

No. The system should constrain the allocation. If you have enough
memory you should be allowed to open a _correct_ png file.

Of course we could (I wont) discuss how to change the implementation
to allow systems without enough ram to process files with large chunks,
but I doubt it would be a problem in real life.

Regards,
adr.

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-M58cd1dfaefcf10ef70868139
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[adr via 9fans]

By the way, the lenght should be checked to not exceed 0x7FFFFFFF
so a corrupt chunk can be detected early.


--- /n/dump/2021/0627/sys/src/cmd/jpg/readpng.c Thu Jan 24 23:39:55 2013
+++ /sys/src/cmd/jpg/readpng.c  Tue Jul 13 11:16:50 2021
@@ -10,8 +10,6 @@

  enum
  {
-       IDATSIZE = 1000000,
-
        /* filtering algorithms */
        FilterNone =    0,      /* new[x][y] = buf[x][y] */
        FilterSub =     1,      /* new[x][y] = buf[x][y] + new[x-1][y] */ 
@@ -51,7 +49,6 @@
  struct ZlibR
  {
        Biobuf *io;             /* input buffer */
-       uchar *buf;             /* malloc'ed staging buffer */
        uchar *p;                       /* next byte to decompress */
        uchar *e;                       /* end of buffer */
        ZlibW *w;
@@ -94,19 +91,28 @@
  }

  static int
-getchunk(Biobuf *b, char *type, uchar *d, int m)
+chunklen(Biobuf *b)
+{
+       ulong n;
+
+       uchar buf[4];
+
+       if(Bread(b, buf, 4) != 4 || (n=get4(buf)) > 0x7FFFFFFF)
+               return -1;
+       return n;
+}
+
+static int
+getchunk(Biobuf *b, char *type, uchar *d, int n)
  {
-       uchar buf[8];
+       uchar buf[4];
        ulong crc = 0, crc2;
-       int n, nr;
+       int nr;

-       if(Bread(b, buf, 8) != 8)
+       if(Bread(b, buf, 4) != 4)
                return -1;
-       n = get4(buf);
-       memmove(type, buf+4, 4);
+       memmove(type, buf, 4);
        type[4] = 0;
-       if(n > m)
-               sysfatal("getchunk needed %d, had %d", n, m);
        nr = Bread(b, d, n);
        if(nr != n)
                sysfatal("getchunk read %d, expected %d", nr, n);
@@ -117,7 +123,7 @@
        crc2 = get4(buf);
        if(crc != crc2)
                sysfatal("getchunk crc failed");
-       return n;
+       return 0;
  }

  static int
@@ -129,25 +135,31 @@

        if(z->p >= z->e){
        Again:
-               z->p = z->buf;
+               n = chunklen(z->io);
+               if(n < 0)
+                       return -1;
+               z->p = pngmalloc(n, 0);
                z->e = z->p;
-               n = getchunk(z->io, type, z->p, IDATSIZE);
-               if(n < 0 || strcmp(type, "IEND") == 0)
+               getchunk(z->io, type, z->p, n);
+               if(strcmp(type, "IEND") == 0){
+                       free(z->p);
                        return -1;
+               }
                z->e = z->p + n;
                if(!strcmp(type,"PLTE")){
                        if(n < 3 || n > 3*256 || n%3)
                                sysfatal("invalid PLTE chunk len %d", n);
                        memcpy(z->w->palette, z->p, n);
                        z->w->palsize = 256;
+                       free(z->p);
                        goto Again;
                }
-               if(type[0] & PropertyBit)
+               if(type[0] & PropertyBit){
+                       free(z->p);
                        goto Again;  /* skip auxiliary chunks fornow */
-               if(strcmp(type,"IDAT")){
-                       sysfatal("unrecognized mandatory chunk %s", type);
-                       goto Again;
                }
+               if(strcmp(type,"IDAT"))
+                       sysfatal("unrecognized mandatory chunk %s", type);
        }
        return *z->p++;
  }
@@ -388,13 +400,18 @@
        ZlibR zr;
        ZlibW zw;

-       buf = pngmalloc(IDATSIZE, 0);
+       buf = pngmalloc(sizeof PNGmagic, 0);
        if(Bread(b, buf, sizeof PNGmagic) != sizeof PNGmagic ||
            memcmp(PNGmagic, buf, sizeof PNGmagic) != 0)
                sysfatal("bad PNGmagic");
+       free(buf);

-       n = getchunk(b, type, buf, IDATSIZE);
-       if(n < 13 || strcmp(type,"IHDR") != 0)
+       n = chunklen(b);
+       if(n < 13)
+               sysfatal("wrong IHDR chunk length");
+       buf = pngmalloc(n, 0);
+       getchunk(b, type, buf, n);
+       if(strcmp(type,"IHDR") != 0)
                sysfatal("missing IHDR chunk");
        h = buf;
        dx = get4(h);
@@ -460,7 +477,7 @@
        memset(&zr, 0, sizeof zr);
        zr.w = &zw;
        zr.io = b;
-       zr.buf = buf;
+       free(buf);

        memset(&zw, 0, sizeof zw);
        if(useadam7)
@@ -483,7 +500,6 @@
        if(err)
                sysfatal("inflatezlib %s\n", flateerr(err));

-       free(buf);
        free(zw.scan);
        free(zw.lastscan);
        return image;

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-M13a5098cf3af92b1671aa59c
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[ori]

Quoth adr via 9fans <9fans@9fans.net>:
> On Mon, 12 Jul 2021, ori@eigenstate.org wrote:
> > Do you have an example?
> 
> I could upload it in some place but you don't need it, really. The
> specification is clear

Yes, but your patch doesn't apply cleanly to 9front, and
it would be useful to check whether I fuck up porting it
manually.


------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-Mb081540e68f5b38f2e971028
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription

Re: [9fans] pngread: alloc chunk's length

[adr via 9fans]

On Wed, 14 Jul 2021, ori@eigenstate.org wrote:
> Yes, but your patch doesn't apply cleanly to 9front, and
> it would be useful to check whether I fuck up porting it
> manually.

Ok, but I told you already how to create one.

http://adr.freeshell.org/files/turner_ovid_banished_from_rome.png

------------------------------------------
9fans: 9fans
Permalink: https://9fans.topicbox.com/groups/9fans/T4a714ed14c50767a-Mc0426d011a67f1ff1400cc2b
Delivery options: https://9fans.topicbox.com/groups/9fans/subscription