Created attachment 341456 [details] [review]
potential patch

Created attachment 341885 [details] [review]
tests: Add test for bug 775693

Fix committed with a few style fixes.

Attachment 341885 [details] pushed as 3724a73 - tests: Add test for bug 775693

amazing! Very good work.
FTR: https://git.gnome.org/browse/gdk-pixbuf/commit/?id=9ae4723ec3fa631354e3d201c5435a7385c33d45 is the fix.

The fix seems wrong for the case of copy_pixels true and
encoding != GDK_PIXDATA_ENCODING_RLE. In this case we have:

...
const size_t data_size = pixdata->height * pixdata->rowstride;
data_limit = data + data_size;
...
if (data + (pixdata->rowstride * pixdata->height) < data_limit)
  memcpy(<args>)
else
  <fail>

None of the variables are modifed in the second ellipsis, so
I don't see how the condition for proceeding with the memcpy
can be satisfied: we should have

data + (pixdata->rowstride * pixdata->height) == data_limit

not less than.

(In reply to Allin Cottrell from comment #5)
> The fix seems wrong for the case of copy_pixels true and
> encoding != GDK_PIXDATA_ENCODING_RLE. In this case we have:
> 
> ...
> const size_t data_size = pixdata->height * pixdata->rowstride;
> data_limit = data + data_size;
> ...
> if (data + (pixdata->rowstride * pixdata->height) < data_limit)
>   memcpy(<args>)
> else
>   <fail>
> 
> None of the variables are modifed in the second ellipsis, so
> I don't see how the condition for proceeding with the memcpy
> can be satisfied: we should have
> 
> data + (pixdata->rowstride * pixdata->height) == data_limit
> 
> not less than.

Yeah, the fix is incorrect. It needs to check the ->length, if set.

Created attachment 342067 [details] [review]
tests: Additional test for bug 775693

Created attachment 342068 [details] [review]
tests: Fix bug in pixdata test

In 3724a739 we added a test for the deserializing code, but we shouldn't
be free'ing the contents straight away, as gdk_pixdata_deserialize()
does not copy the data, but expects it to stay around until we've copied
the data to the pixbuf (with gdk_pixbuf_from_pixdata() and copy_pixels
== TRUE), or we're done with the pixbuf.

Created attachment 342069 [details] [review]
pixdata: Add debug when decoding pixdata

Created attachment 342070 [details] [review]
Revert "pixdata: Prevent buffer overflow by checking for bounds before memcpy"

The fix is incorrect.

This reverts commit 9ae4723ec3fa631354e3d201c5435a7385c33d45.

Created attachment 342071 [details] [review]
pixdata: Avoid copying more data than available

The problem we were encountering in bug 775693 wasn't that we were
running past the end of the memory we just allocated, but that the
length of the data we were given couldn't possibly match the dimensions
of the image.

This fixes running past the end of the given pixdata when the length is
provided inside the structure.

Created attachment 342072 [details] [review]
pixdata: Add "-r" as an alias for "--rle"

Created attachment 342073 [details] [review]
pixdata: Add "--rle" to the --help output

Created attachment 342074 [details] [review]
tests: Add more pixdata tests

Those ones should succeed, and we test both uncompressed and
RLE-compressed data.

Created attachment 342075 [details] [review]
pixdata: Check for RLE pixdata length

Avoid copying data from past the end of the pixdata when processing
RLE-encoded pixdata.

Attachment 342067 [details] pushed as daaae1b - tests: Additional test for bug 775693
Attachment 342068 [details] pushed as 28fbde8 - tests: Fix bug in pixdata test
Attachment 342069 [details] pushed as 7f075ba - pixdata: Add debug when decoding pixdata
Attachment 342070 [details] pushed as 6ddb160 - Revert "pixdata: Prevent buffer overflow by checking for bounds before memcpy"
Attachment 342071 [details] pushed as 73ce596 - pixdata: Avoid copying more data than available
Attachment 342072 [details] pushed as 2396b4d - pixdata: Add "-r" as an alias for "--rle"
Attachment 342073 [details] pushed as 4f41e7c - pixdata: Add "--rle" to the --help output
Attachment 342074 [details] pushed as a829106 - tests: Add more pixdata tests
Attachment 342075 [details] pushed as 10c8111 - pixdata: Check for RLE pixdata length

507    while (image_buffer < image_limit &&
508 	   (rle_buffer_limit != NULL || rle_buffer > rle_buffer_limit))


I'm being curious: Is the "rle_buffer > rle_buffer_limit" limit check correct?

rle_buffer_limit = pixdata->pixel_data + pixdata->length - GDK_PIXDATA_HEADER_LENGTH;

I think rle_buffer_limit describes the byte *after* the buffer, i.e. where we should not be able to read from or write into.  So a typical check is the one that existed before, e.g. "buffer_p < buffer_limit". When checking the "other way around", as I think is done with the patch, it may be that the check needs to test for 
"rle_buffer >= rle_buffer_limit", i.e. whether the current pointer has already hit the limit and not exceeded it.

But: I don't know the code at all and I might be missing some things that are obvious to people with more knowledge.

(In reply to Tobias Mueller from comment #17)
> 507    while (image_buffer < image_limit &&
> 508 	   (rle_buffer_limit != NULL || rle_buffer > rle_buffer_limit))
> 
> 
> I'm being curious: Is the "rle_buffer > rle_buffer_limit" limit check
> correct?
> 
> rle_buffer_limit = pixdata->pixel_data + pixdata->length -
> GDK_PIXDATA_HEADER_LENGTH;
> 
> I think rle_buffer_limit describes the byte *after* the buffer,

No, it describes the last byte of the buffer. data_address + length should be the offset for the last byte of data.

At this point, it would be best to file new bugs referencing those older ones if you want to have answers.