883624 - Pointer arithmetic in Blur.cpp may wrap on underflow causing illegal offsets (2^32-n) on 64bit archs

Status: RESOLVED INVALID

(Core :: Graphics, defect)

Comment 1

[Martin Husemann]

Attachment: Force arithmetic to ptrdiff_t to avoid wrapping to 2^32 on underflow

Comment 2

[Jeff Muizelaar [:jrmuizel]]

I don't(In reply to Martin Husemann from comment #1)
> Created attachment 763218 [details] [diff] [review]
> Force arithmetic to ptrdiff_t to avoid wrapping to 2^32 on underflow

I don't understand why this is only a problem on 64bit archs.

Comment 3

[Martin Husemann]

I haven't fully done the math, but assumed it would wrap twice and end up with the right pointer on 32bit archs. E.g. we start with an offset of 60 byte into the image and the first callculation gives -58 offset from that, which wraps to 0xffffffc6, which then is added to the pointer and wraps again, resulting in the address 58 before the start of the 60 byte offset pointer (as intended).

If ptrdiff_t is not portable enough (grrr MS VC++), there may be other ways to express this, maybe keeping the arithmetic int and not pre-adjusting the pointer into the image, but explicitly adding the offset within this calculation.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

(In reply to Martin Husemann from comment #3)
> I haven't fully done the math, but assumed it would wrap twice and end up
> with the right pointer on 32bit archs. E.g. we start with an offset of 60
> byte into the image and the first callculation gives -58 offset from that,
> which wraps to 0xffffffc6, which then is added to the pointer and wraps
> again, resulting in the address 58 before the start of the 60 byte offset
> pointer (as intended).
> 
> If ptrdiff_t is not portable enough (grrr MS VC++), there may be other ways
> to express this, maybe keeping the arithmetic int and not pre-adjusting the
> pointer into the image, but explicitly adding the offset within this
> calculation.

y, aTopLobe,  stride32bit and aLeftLobe are all signed integers, so the result
will be a signed integer and shouldn't underflow or overflow. This is added to the pointer which should also be fine. Thus I still don't understand what the problem is.

Comment 5

[Martin Husemann]

I can follow your argument and indeed do not see why int promotion to unsigned should happen here. However, I saw the crash life in gdb and can reproduce it - maybe gdb lied to me, will crash it again and dig deeper (but that may take a few days, haven't got a working debug version handy right now).

Comment 6

[Martin Husemann]

I can not reproduce it any more - whatever happened back then when I hit it (on an older branch).
Sorry for the noise.