[cairo] Review of recent negative-stride changes

[Chris Wilson]

On Wed, 2008-03-05 at 14:01 -0800, Carl Worth wrote:
> 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.

Whilst we here, is there any desire for generating alpha-only images
from grayscale PNGs. At present such images are expanded to RGB, which
IMHO breaks the principle of least surprise - but the behaviour may now
be considered part of the API, having done so through many releases.

> > 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.

The background here is that as I wanted to use
cairo_format_stride_for_width() inside cairo-png.c I first checked how
other places within the cairo codebase dealt with similar needs to call
cairo_image_surface_create_for_data() with multiple formats. I spotted
that in cairo-glitz-surface.c, there was a call to
cairo_image_surface_create_for_data() with a negative stride and a quick
grep through pixman revealed that it too in part checked for negative
strides. On reflection, pixman does not handle negative strides (the
instance I spotted looks like an anomaly) and the call in glitz is a
carefully crafted abuse of cairo's API. 

> 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.)

Reading the documentation, it would be useful just to quickly say the
image data is in scanline order (and even explain "scanline order"!) as
not all graphic libraries use the same coordinate system.

> > 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?

Just a couple of niggles. The first is malloc(0) has the potential to
return NULL which may upset some apps and also the user may be filling
the buffer before passing to cairo_image_surface_create_for_data. Before
either of which the programmer should check for an error from
cairo_format_stride_for_width(), so from that point of view I think it
is immaterial whether the error case is -1 or 0. -1, I think, has the
benefit of clearly being an invalid stride.

The second issue is that cairo_format_stride_for_width() will currently
assert on an invalid format - which may or may not be a bad thing!

I would bolster the check inside cairo_image_surface_create_for_data()
with a stride is less than cairo_format_stride_for_width as well, and
also consider checking for negative widths and heights.

> 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.

Noted. I concentrated on ensuring that each was an easily revertible
micro-commit, and even had intended to raise the API issue on list. On
that front, I should endeavour to keep questionable topic branches so
that controversial commits do not end up in the middle of a series of
simple fixes. With the negative stride, I didn't find anything to
explicitly prohibit such strides and having found an example within
cairo (and seeing it passed by the guards in
cairo_image_surface_create_for_data) I thought it was tacitly supported!
Oops.
--
Chris Wilson