Re: Potential patches (need review help)

On 09/23/2014 01:16 PM, Bryce Harrington wrote:

> https://bugs.freedesktop.org/show_bug.cgi?id=63461
>
> Another platform endianness bug.  The original patch looked simple
> enough, but it's been superseded by two new patches that add NULL/0
> checks.  Do these two patches cover all the cases that the first patch
> did?  Or is there a simpler way to handle this without peppering these
> checks everywhere?

The problem here is the conversion code often was written something like 
this:

     do {
       convert(*ptr++);
     } while --length;

This would fail if the length was zero, instead converting MAXUNSIGNED+1 
locations. Generally this would eventually hit an unallocated page and 
cause a segmentation violation.

This led to two different fixes. First one actually checked the length:

     if (length > 0) do {
       convert(*ptr++);
     } while --length;

Somebody else found the bug but noticed that ptr was zero (since it is 
probably common that if length is zero ptr is zero), resulting in this 
different patch:

     if (ptr) do {
       convert(*ptr++);
     } while --length;

I think we can assume that if the ptr is zero, the length is zero. 
Otherwise all kinds of other things will crash, such as

   memcpy(dest, ptr, length*sizeof(*ptr));

The opposite is not true, if length is zero ptr might be non-zero (for 
instance malloc(0) is non-zero on many systems).

This means the second one is entirely redundant and both patches should 
not be applied. Only the length should be checked.

However I really think changing the code to only have a single 
conditional is a better solution:

     while (length--) {
       convert(*ptr++);
     }

I suppose someone is worried that this adds one extra test of length, 
but since the fix is to add a test anyway that is moot. Or they are 
concerned that postfix is slower than prefix, but I really don't think 
that is true for any modern CPUs or compilers for integers.
-- 
cairo mailing list
[email protected]
http://lists.cairographics.org/mailman/listinfo/cairo