8357299: Graphics copyArea doesn't copy any pixels when there is overflow #25340

prsadhuk · 2025-05-21T03:17:34Z

Graphics copyArea overflow check bails out of copying pixels if there is overflow.
The spec says ""If a portion of the source rectangle lies outside the bounds of the component, or is obscured by another window or component, {@code copyArea} will be unable to copy the associated pixels"

which suggests that we should always copy the parts inside the bounds and never the parts outside the bounds
but it seems currently, in the case of overflow it no longer copies any pixels, including the parts that are inside.
So, the fix clips the copyarea region to clip bounds so it will only affect pixels within the valid bounds, and any pixels outside will be ignored.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8357299: Graphics copyArea doesn't copy any pixels when there is overflow (Bug - P3)

Reviewers

Alisen Chung (@alisenchung - Committer)
Alexander Zuev (@azuev-java - Reviewer)
Phil Race (@prrace - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/25340/head:pull/25340
$ git checkout pull/25340

Update a local copy of the PR:
$ git checkout pull/25340
$ git pull https://git.openjdk.org/jdk.git pull/25340/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25340

View PR using the GUI difftool:
$ git pr show -t 25340

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/25340.diff

Using Webrev

Link to Webrev Comment

…flow

bridgekeeper · 2025-05-21T03:18:25Z

👋 Welcome back psadhukhan! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2025-05-21T03:18:28Z

@prsadhuk This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8357299: Graphics copyArea doesn't copy any pixels when there is overflow

Reviewed-by: achung, kizune, prr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 614 new commits pushed to the master branch:

e509997: 8357275: Locale.Builder.setLanguageTag should mention conversions made on language tag
e306367: 8357693: AOTCodeCompressedOopsTest.java failed with -XX:+UseLargePages
f318868: 8348328: Update IANA Language Subtag Registry to Version 2025-05-15
... and 611 more: https://git.openjdk.org/jdk/compare/428334b616e1c436fbc2b8d71e41a0b154e9da12...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2025-05-21T03:18:58Z

@prsadhuk The following label will be automatically applied to this pull request:

client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2025-05-21T03:22:21Z

Webrevs

00: Full (2731bea5)

mrserb · 2025-05-21T07:05:26Z

src/java.desktop/share/native/libawt/java2d/loops/Blit.c


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


isn't the MaskBlit_MaskBlit use the same pattern?

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?

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.

alisenchung

Tested locally on macOS and test fails before the change and passes after the change.

azuev-java

Looks good.

prrace · 2025-05-23T20:59:41Z

src/java.desktop/share/native/libawt/java2d/loops/Blit.c

-    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)


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);

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

@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..

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.

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..

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

mrserb · 2025-05-29T23:29:37Z

src/java.desktop/share/native/libawt/java2d/loops/Blit.c

-    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)


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.

mrserb · 2025-05-29T23:31:12Z

src/java.desktop/share/native/libawt/java2d/loops/Blit.c

+    dstInfo.bounds.y2 = UNSAFE_TO_ADD(dsty, height)
+                        ? clipInfo.bounds.y2 : (dsty + height);
    if (UNSAFE_TO_SUB(srcx, dstx) ||
        UNSAFE_TO_SUB(srcy, dsty)) {


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?

prsadhuk · 2025-05-30T02:06:42Z

/integrate

openjdk · 2025-05-30T02:07:00Z

Going to push as commit 64503c7.
Since your change was applied there have been 619 commits pushed to the master branch:

a05f9de: 8358017: Various enhancements of jpackage test helpers
648c337: 8356648: runtime/Thread/AsyncExceptionTest.java fails with +StressCompiledExceptionHandlers
cb8eea4: 8356095: AArch64: Obsolete -XX:+NearCPool option
... and 616 more: https://git.openjdk.org/jdk/compare/428334b616e1c436fbc2b8d71e41a0b154e9da12...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-05-30T02:07:07Z

@prsadhuk Pushed as commit 64503c7.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

8357299: Graphics copyArea doesn't copy any pixels when there is over…

2731bea

…flow

openjdk bot added the rfr Pull request is ready for review label May 21, 2025

openjdk bot added the client client-libs-dev@openjdk.org label May 21, 2025

mrserb reviewed May 21, 2025

View reviewed changes

alisenchung approved these changes May 22, 2025

View reviewed changes

azuev-java approved these changes May 23, 2025

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label May 23, 2025

prrace reviewed May 23, 2025

View reviewed changes

honkar-jdk mentioned this pull request May 27, 2025

JDK-8357082 : Stabilize and add debug logs to CopyAreaOOB.java #25279

Closed

3 tasks

prrace approved these changes May 29, 2025

View reviewed changes

mrserb reviewed May 29, 2025

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label May 30, 2025

openjdk bot closed this May 30, 2025

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels May 30, 2025

prsadhuk deleted the JDK-8357299 branch May 30, 2025 02:07

mrserb mentioned this pull request May 30, 2025

8358107: Rollback JDK-8357299 changeset #25537

Closed

3 tasks

8357299: Graphics copyArea doesn't copy any pixels when there is overflow #25340

8357299: Graphics copyArea doesn't copy any pixels when there is overflow #25340

Uh oh!

Conversation

prsadhuk commented May 21, 2025 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented May 21, 2025

Uh oh!

openjdk bot commented May 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented May 21, 2025

Uh oh!

mlbridge bot commented May 21, 2025

Webrevs

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alisenchung left a comment

Choose a reason for hiding this comment

Uh oh!

azuev-java left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsadhuk May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrserb May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrserb May 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prsadhuk commented May 30, 2025

Uh oh!

openjdk bot commented May 30, 2025

Uh oh!

openjdk bot commented May 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

prsadhuk commented May 21, 2025 •

edited by openjdk bot

Loading

openjdk bot commented May 21, 2025 •

edited

Loading

prsadhuk May 26, 2025 •

edited

Loading

mrserb May 30, 2025 •

edited

Loading

mrserb May 29, 2025 •

edited

Loading