8330936: [ubsan] exclude function BilinearInterp and ShapeSINextSpan in libawt java2d from ubsan checks

[MBaesken]

In java2d coding there are a few overflows (those are shown when running jtreg tests with ubsan enabled binaries)
jtreg test java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java shows

jdk/src/java.desktop/share/native/libawt/java2d/loops/TransformHelper.c:683:16: runtime error: signed integer overflow: 1651910497 + 660764199 cannot be represented in type 'int'
#0 0x7efe59e6ece8 in BilinearInterp src/java.desktop/share/native/libawt/java2d/loops/TransformHelper.c:683
#1 0x7efe59e75e21 in Java_sun_java2d_loops_TransformHelper_Transform src/java.desktop/share/native/libawt/java2d/loops/TransformHelper.c:499
#2 0x7efe9b8dee7b ()

java/awt/BasicStroke/DashStrokeTest.java shows

src/java.desktop/share/native/libawt/java2d/pipe/ShapeSpanIterator.c:1366:21: runtime error: signed integer overflow: 1282539 + 2118518271 cannot be represented in type 'int'
#0 0x7fb97d7daf21 in ShapeSINextSpan src/java.desktop/share/native/libawt/java2d/pipe/ShapeSpanIterator.c:1366
#1 0x7fb97d62fa7e in AnyIntSetSpans src/java.desktop/share/native/libawt/java2d/loops/AnyInt.c:75
#2 0x7fb97d6a8816 in Java_sun_java2d_loops_FillSpans_FillSpans src/java.desktop/share/native/libawt/java2d/loops/FillSpans.c:92
#3 0x7fba12d07e7b ()

There is currently no need seen to adjust this coding, so exclude the methods from ubsan checking to avoid unneeded warnings.

---

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-8330936: [ubsan] exclude function BilinearInterp and ShapeSINextSpan in libawt java2d from ubsan checks (Sub-task - P3)

Reviewers

• Christoph Langer (@RealCLanger - Reviewer)
• Phil Race (@prrace - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 23255

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

Using diff file

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

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mbaesken! 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]

@MBaesken 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:

8330936: [ubsan] exclude function BilinearInterp and ShapeSINextSpan in libawt java2d from ubsan checks

Reviewed-by: clanger, 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 508 new commits pushed to the master branch:

• 11a37c8: 8351165: Remove unused includes from vmStructs
• c3b4819: 8351074: Disallow null prefix and suffix in DecimalFormat
• 6012e8d: 8350808: Small typos in JShell method SnippetEvent.toString()
• 661bd5b: 8343478: Remove unnecessary @SuppressWarnings annotations (core-libs)
• 61d9ab9: 8350854: Include thread counts in safepoint logging
• ea9e3cf: 8281511: java/net/ipv6tests/UdpTest.java fails with checkTime failed
• caaf409: 8350546: Several java/net/InetAddress tests fails UnknownHostException
• 062b7c7: 8351115: Test AOTClassLinkingVMOptions.java fails after JDK-8348322
• a88e8cd: 8350952: Remove some non present files from OPT_SPEED_SRC list
• de29ef3: 8343191: Cgroup v1 subsystem fails to set subsystem path
• ... and 498 more: https://git.openjdk.org/jdk/compare/6032f6ea04a5f0d52e0a375df764cb15273c8836...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]

@MBaesken 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]

Webrevs

• 00: Full (3c85e821)

[RealCLanger]

The change looks ok and trivial. However I find nothing about an investigation what would need to be changed to eliminate the UB warning in code.

[MBaesken]

I moved the ubsan 'exclusion' to a subissue and opened parent issue https://bugs.openjdk.org/browse/JDK-8348966 for the 2 overflows .

[RealCLanger]

LGTM now with the updated bug synopsis. But somebody from client-libs has to give their blessings as well.

[mrserb]

how it is possible to repro these warnings?

[MBaesken]

how it is possible to repro these warnings?

Configure with '--enable-ubsan' added (on Linux; ubsan package needs to be installed on your distro). After the build, run the mentioned jtreg tests (java/awt/Scrollbar/AquaLFScrollbarTest/ScrollBarBorderTest.java and java/awt/BasicStroke/DashStrokeTest.java, if you run jtreg jdk :tier4 there might be a few other tests showing the issues too).

[MBaesken]

Any more comments on this ? If not I would integrate it soon.

[MBaesken]

Hi @mrserb any comments? Are you fine with the change ?

[prrace]

I looked at the native code, and it seems to me that overflow is actually by design, when working with fractional pixel positions and color components.
So I think this points out that whilst ubsan findings might be an interesting data point, it isn't an automatic "go update the code".

The mechanism being used to silence the warnings might be palatable in very small quantities, but I sincerely hope it is not the start of a pattern. If it is, then something less intrusive to the source code would be better.
For example, something that is part of the build system. This would become a "must" if there's a 3rd party library involved.
Eg, when we find a compiler warning against 3rd party code, we just selectively disable the warning, and do not change the imported sources.

[MBaesken]

So I think this points out that whilst ubsan findings might be an interesting data point, it isn't an automatic "go update the code".

I agree, the findings have to be checked and not always this leads to a source change.

[MBaesken]

/integrate

[openjdk]

Going to push as commit 40f150d.
Since your change was applied there have been 514 commits pushed to the master branch:

• 649ef77: 8323158: HotSpot Style Guide should specify more include ordering
• 5c552a9: 8349358: [JMH] Cannot access class jdk.internal.vm.ContinuationScope
• e82031e: 8350756: C2 SuperWord Multiversioning: remove useless slow loop when the fast loop disappears
• 3626ac3: 8204868: java/util/zip/ZipFile/TestCleaner.java still fails with "cleaner failed to clean zipfile."
• 4bb3d81: 8351138: Running subset of gtests gets error printing result information
• 107ee87: 8346954: [JMH] jdk.incubator.vector.MaskedLogicOpts fails due to IndexOutOfBoundsException
• 11a37c8: 8351165: Remove unused includes from vmStructs
• c3b4819: 8351074: Disallow null prefix and suffix in DecimalFormat
• 6012e8d: 8350808: Small typos in JShell method SnippetEvent.toString()
• 661bd5b: 8343478: Remove unnecessary @SuppressWarnings annotations (core-libs)
• ... and 504 more: https://git.openjdk.org/jdk/compare/6032f6ea04a5f0d52e0a375df764cb15273c8836...master

Your commit was automatically rebased without conflicts.

[openjdk]

@MBaesken Pushed as commit 40f150d.

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