Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 9 additions & 12 deletions src/java.desktop/share/native/libawt/java2d/loops/Blit.c
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2000, 2024, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2000, 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -74,19 +74,16 @@ Java_sun_java2d_loops_Blit_Blit

srcInfo.bounds.x1 = srcx;
srcInfo.bounds.y1 = srcy;
if (UNSAFE_TO_ADD(srcx, width) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't the MaskBlit_MaskBlit use the same pattern?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But that doesn't cause any issue to the testcase given and existing testcase..Do you have any testcase which doesnt work because of MaskBlit pattern?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not tried to create a test for MaskBlit, only for a simple Blit. I will take a look at the possibility of reproducing it.

UNSAFE_TO_ADD(srcy, height) ||
UNSAFE_TO_ADD(dstx, width) ||
UNSAFE_TO_ADD(dsty, height)) {
return;
}

srcInfo.bounds.x2 = srcx + width;
srcInfo.bounds.y2 = srcy + height;
srcInfo.bounds.x2 = UNSAFE_TO_ADD(srcx, width)
? clipInfo.bounds.x2 : (srcx + width);
srcInfo.bounds.y2 = UNSAFE_TO_ADD(srcy, height)
? clipInfo.bounds.y2 : (srcy + height);
dstInfo.bounds.x1 = dstx;
dstInfo.bounds.y1 = dsty;
dstInfo.bounds.x2 = dstx + width;
dstInfo.bounds.y2 = dsty + height;
dstInfo.bounds.x2 = UNSAFE_TO_ADD(dstx, width)
? clipInfo.bounds.x2 : (dstx + width);
dstInfo.bounds.y2 = UNSAFE_TO_ADD(dsty, height)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why wouldn't you always want to limit it to the clip ?
I mean shouldn't it be like this ?
dstInfo.bounds.x2 = UNSAFE_TO_ADD(dstx, width) ? clipInfo.bounds.x2 : (((dstx + width) > clipInfo.bounds.x2) ? clipInfo.bounds.x2 : (dstx + width));
or maybe a bit more readable as
dstInfo.bounds.x2 = ((UNSAFE_TO_ADD(dstx, width) || ((dstx + width) > clipInfo.bounds.x2)) ? clipInfo.bounds.x2 : (dstx + width);

Copy link
Contributor Author

@prsadhuk prsadhuk May 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is because down below it anyway calls
SurfaceData_IntersectBounds(&dstInfo.bounds, &clipInfo.bounds);
so it should clip to clipInfo.bounds there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prrace Since it is going to clip dstInfo.bounds to clipInfo.bounds in SurfaceData_IntersectBounds, I believe is not necessary to do the duplicate clip here.
Let me know if you think otherwise..

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is because down below it anyway calls SurfaceData_IntersectBounds(&dstInfo.bounds, &clipInfo.bounds);
so it should clip to clipInfo.bounds there.

In addition to intersecting dst with clip, you're also intersecting src with clip, which seems incorrect. A better approach might be to compute the drawable width based on the non-overflowing range for both src and dst, and then proceed with the original logic.

BTW I have not checked MaskBlit_MaskBlit yet it might require a similar update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have not found any problem with that part of code so far..We have addressed the issue that was raised.
MaskBlit might require similar update but since it was not proven to be problematic (we waited for your feedback) and since RDP1 is approaching, we will integrate this and you can come back with POC for us to check..

Copy link
Member

@mrserb mrserb May 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have not found any problem with that part of code so far..

This is incorrect because the clip bounds are relative to the destination (dst), so you cannot intersect them directly with the source (src) surface. The usage of UNSAFE_TO_SUB below is also wrong, we cannot just exists.

filed https://bugs.openjdk.org/browse/JDK-8358103 and https://bugs.openjdk.org/browse/JDK-8358107

? clipInfo.bounds.y2 : (dsty + height);
if (UNSAFE_TO_SUB(srcx, dstx) ||
UNSAFE_TO_SUB(srcy, dsty)) {
Copy link
Member

@mrserb mrserb May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also should check this part of the code, is it correct to simply exit here, or should we instead calculate and use the maximum distance we can handle?

return;
Expand Down
60 changes: 60 additions & 0 deletions test/jdk/java/awt/Graphics/BrokenBoundsClip.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/

/*
* @test
* @bug 8357299
* @summary Verifies if Graphics copyArea doesn't copy any pixels
* when there is overflow
* @run main BrokenBoundsClip
*/

import java.awt.Color;
import java.awt.Graphics2D;
import java.awt.image.BufferedImage;

import static java.awt.image.BufferedImage.TYPE_INT_RGB;

public final class BrokenBoundsClip {

public static final int SIZE = 100;

public static void main(String[] args) {
BufferedImage bi = new BufferedImage(SIZE, SIZE, TYPE_INT_RGB);

Graphics2D g2d = bi.createGraphics();
g2d.setColor(Color.RED);
g2d.fillRect(SIZE / 2, SIZE / 2, SIZE / 2, SIZE / 2);

g2d.copyArea(bi.getWidth() / 2, bi.getHeight() / 2,
Integer.MAX_VALUE , Integer.MAX_VALUE ,
-bi.getWidth() / 2, -bi.getHeight() / 2);
int actual = bi.getRGB(0, 0);
int expected = Color.RED.getRGB();
if (actual != expected) {
System.err.println("Actual: " + Integer.toHexString(actual));
System.err.println("Expected: " + Integer.toHexString(expected));
throw new RuntimeException("Wrong color");
}
}
}