[cairo] Review of recent negative-stride changes

[Carl Worth]

On Wed,  5 Mar 2008 06:38:20 -0800 (PST), Chris Wilson wrote:
> commit 11a2444ec875aaaed12c1f1cfed5eb8e139c306d
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Fri Jan 18 14:53:50 2008 +0000
>
>     [cairo-png] Support generating CAIRO_FORMAT_RGB24 from PNGs.
>
>     If the PNG does not have an alpha channel, then create an opaque
>     image.

Excellent change, Chris, thank you! I know people have previously
expressed a desire for this, so it's nice to see it done.

> commit 06b375aee999220ce294c22fa50a3040c19d5492
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Fri Feb 29 11:55:01 2008 +0000
>
>     [cairo-png] Use cairo_format_stride_for_width()
>
>     Use cairo_format_stride_for_width() instead of assuming the pixel size
>     and manually calculating the row stride. This should make it easier to
>     support loading multiple image formats in future.
...
> -    pixel_size = 4;
> -    data = _cairo_malloc_abc (png_height, png_width, pixel_size);
> +    stride = cairo_format_stride_for_width (CAIRO_FORMAT_ARGB32, png_width);
> +    if (stride < 0) {
> +	surface = _cairo_surface_create_in_error (_cairo_error (CAIRO_STATUS_INVALID_STRIDE));
> +	goto BAIL;
> +    }
> +
> +    data = _cairo_malloc_ab (png_height, stride);

I just want to quote that for now, because I think it's the
justification for something that appears later in this message,
(earlier in the commit history).

> commit b181f40949a855c957dc6e7a1033981a2ed7d05a
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Thu Feb 28 16:04:08 2008 +0000
>
>     [test/a8-mask] Check negative strides as well.
>
>     Check that we also allow surfaces to be created using a negative stride.

Uhm... is this even something we want to enshrine in the test
suite. I certainly don't expect cairo_image_surface_create_for_data to
work with a negative stride. And it's not currently documented that
way.

Is there some justification for supporting this? If so, then let's
change the documentation and ensure the implementation matches. But
we're a little late in 1.5.x for a change like that.

And if not, then let's not make the test suite enforce this. I'd like
to revert this change for now.

Meanwhile, what we do document in cairo_image_surface_create_for_data
is that the stride come from cairo_format_stride_for_width. So
supporting negative stride here makes no sense together with the
change to use -1 as an error return value from that function. (More on
that below.)

> commit b6eb1c5c92321849661198facd53510366050d45
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Fri Feb 29 11:49:14 2008 +0000
>
>     [cairo-image-surface] Harden cairo_format_stride_for_width().
>
>     Check the user supplied values for validity and potential overflow,
>     returning -1 in such cases, and update the documentation to warn of the
>     new error return.

Yuck.

I don't like this one at all. I don't like overloading variables like
"stride" to have magic values like -1 to mean something like "illegal
format or excessively large width". That's really ugly.

It also has the potential to introduce buffer overflows in a case like
this:

	unsigned int stride;

	stride = cairo_format_stride_for_width (illegal_format_value, width);

	data = malloc (stride * height);

Now, we know that the user should be more careful about multiplying
values to be passed to malloc, but we also know that even we have made
mistakes like that before. And passing the -1 value back seems
particularly rude here.

Meanwhile, an illegal format will be caught be
cairo_image_surface_create_for_data anyway, so I don't really see the
advantage to catching this error earlier in
cairo_format_stride_for_width.

However, I do understand the original rationale for hardening
here. The code change above that replaces _cairo_malloc_abc with
cairo_surface_stride_for_width and _cairo_malloc_ab effectively
eliminates one of our multiplication-overflow checks, (by hiding it
inside cairo_format_stride_for_width).

So how about we do the following:

	1. In the case of any error inside
           cairo_format_stride_for_width, (invalid format or overflow
           when computing stride), we return 0.

	2. Inside cairo_image_surface_create_for_data we make it an
           error if the stride is less than the width.

Those are nice clean semantics that should catch all the error cases,
still allow 0 as a legitimate width, and never provide that magic and
unkind value of -1 to the user.

Seem reasonable?

The rest of the patch series was a bunch of _cairo_error cleanups that
seem quite reasonable, (I'm impressed at how many of these you can
find, Chris!). Though, next time it wouldn't hurt to have them
separate from a semantic change like this negative stride stuff.

Thanks again for all you do. I hope you're continuing to have fun with
cairo,

-Carl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.cairographics.org/archives/cairo/attachments/20080305/fe89f442/attachment.pgp