[OpenJDK 2D-Dev] Please review patch for 7105461

[Jim Graham]

Hi Clemens,

For fillRect...

If x or y are > MAX_SHORT then you should probably just reject the 
operation immediately, otherwise you end up with a bunch of math that 
should end up doing a NOP, but it is tenuous if it actually succeeds in 
doing so.

Also, if x,y are < MIN_SHORT then when you clamp them to MIN_SHORT they 
may be > x2,y2 and so when you clamp the dimensions you will end up 
clamping a massively negative number to Ushort.  I don't have the code 
off hand to see how the method deals with that, but it is probably not 
100% kosher in this case (not sure if the subtraction might end up 
looking like a huge positive number).  Thus, it is probably better to 
reject the op if x2,y2 are < MIN_SHORT as well rather than rely on the 
processing.

Also, rejecting the call here rather than relying on the outcome of the 
math avoids adding NOP requests to the X protocol stream...

			...jim

On 10/7/2012 6:41 AM, Clemens Eisserer wrote:
> Hi Jim,
>
> Thanks for your patience, sorry this bugfix review consumed so much time.
> Please find the latest webrev at
> http://cr.openjdk.java.net/~ceisserer/7105461/webrev.04/
>
> The following changes were incooperated:
>
> - Grab AWT lock consistently before entering the try/catch block.
>
> - Clamp X/Y to Short.MAX_VALUE, and width/height to unsigned short,
> X11 drawables may be larger than Short.MAX_VALUE which would cause
> failure for rectangles with x/y outside [0, 32767].
> I didn't take this into account the first time.
>
> - dimAdd now used for x2/y2
>
> - Protect against integer overflow in drawLine too, by using clipAdd.
>
>> ... emcompassesXYXY and
>> ecompassesXYWH will do that for you, the latter even uses dimAdd instead of
>> clipAdd which points out that you probably wanted dimAdd for the x2,y2 in
>> the first place - it will result in an empty rectangle if the dimension is
>> <=0 which is appropriate for fillRect, and it will do it slightly faster
>> than clipAdd I think.
> Thanks for the pointer, I use dimAdd now directly.
>
>> Also, it might be worth a comment that this isn't clipping code so much as
>> making sure the values stay in range for the X protocol...
> Done.
>
> Thanks, Clemens
>