[cairo] Potential patches (need review help)
Bill Spitzak
spitzak at gmail.com
Tue Sep 23 15:03:09 PDT 2014
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.
More information about the cairo
mailing list