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
