8299635: Hotspot update for deprecated sprintf in Xcode 14 #11935

XueleiFan · 2023-01-11T06:26:18Z

The sprintf is deprecated in Xcode 14 because of security concerns. The issue was addressed in JDK-8296812 for hotspot impl, and JDK-8299378 for building, but the test case was not covered. The failure was reported in PR 11793, while running tier1 testing.

This patch is trying to find the use of sprintf in hotspot iml and test cases.

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-8299635: Hotspot update for deprecated sprintf in Xcode 14

Reviewers

David Holmes (@dholmes-ora - Reviewer)
Mikael Vidstedt (@vidmik - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 11935

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

Using diff file

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

bridgekeeper · 2023-01-11T06:27:28Z

👋 Welcome back xuelei! 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 · 2023-01-11T06:31:26Z

@XueleiFan The following labels will be automatically applied to this pull request:

hotspot
jmx
serviceability

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

mlbridge · 2023-01-11T21:03:11Z

Webrevs

vidmik · 2023-01-11T21:16:51Z

This PR does not address all the remaining sprintf:s in hotspot, and with it now explicitly forbidden the build will fail:

src/hotspot/os/linux/attachListener_linux.cpp: In member function 'virtual void LinuxAttachOperation::complete(jint, bufferedStream*)':
src/hotspot/os/linux/attachListener_linux.cpp:414:10: error: call to 'sprintf' declared with attribute warning: use os::snprintf [-Werror=attribute-warning]
  414 |   sprintf(msg, "%d\n", result);
      |   ~~~~~~~^~~~~~~~~~~~~~~~~~~~~

I count ~30 sprintf:s that need updating.

I'm also curious: some of the sprintfs are C2 (src/hotspot/share/opto) - are your builds including C2? If so, why are you not running into the issue for those files?

XueleiFan · 2023-01-11T21:29:01Z

This PR does not address all the remaining sprintf:s in hotspot, and with it now explicitly forbidden the build will fail:

This is a question to me as well. I noticed there are still some use of sprintf, but the building passed on MacOS and Linux. I was wondering if the following update really work (if the '...' parameter works for the forbidden?), or something else matters.

FORBID_C_FUNCTION(int sprintf(char*, const char*, ...), "use os::snprintf");

I count ~30 sprintf:s that need updating.

I'm also curious: some of the sprintfs are C2 (src/hotspot/share/opto) - are your builds including C2? If so, why are you not running into the issue for those files?

I'm new to hotspot. Do you know how could I enable C2? Thanks!

XueleiFan · 2023-01-12T07:25:07Z

I'm also curious: some of the sprintfs are C2 (src/hotspot/share/opto) - are your builds including C2? If so, why are you not running into the issue for those files?

I'm new to hotspot. Do you know how could I enable C2? Thanks!

Never mind, I got it from configuration help message (use --with-jvm-features=compiler2).

sspitsyn

This looks good.
Thank you for fixing it.
Serguei

sspitsyn · 2023-01-13T01:56:23Z

There are more uses of sprintf in some serviceability folders:

src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c:  sprintf(fname, "/proc/%d/status", pid);
src/jdk.hotspot.agent/linux/native/libsaproc/ps_proc.c:  sprintf(fname, "/proc/%d/maps", ph->pid);
src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c:  s = filename + sprintf (filename, "%s/.build-id/", debug_file_directory);
src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c:      s += sprintf (s, "%02x", *data++);
src/jdk.hotspot.agent/linux/native/libsaproc/symtab.c:    s += sprintf (s, "%02x", *data++);
src/jdk.hotspot.agent/windows/native/libsaproc/sawindbg.cpp:        sprintf(errmsg, "%s (hr: 0x%08X)", str, hr); \

src/jdk.management/share/native/libmanagement_ext/management_ext.c:    sprintf(errmsg, "errno: %d error: %s\n", errno, msg);
src/java.management/share/native/libmanagement/VMManagementImpl.c:    sprintf(buf, "%d.%d", major, minor);
src/java.management/share/native/libmanagement/management.c:    sprintf(errmsg, "errno: %d error: %s\n", errno, msg);

src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c:        sprintf(msg, "handshake failed - received >%s< - expected >%s<", b, hello);
src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c:        sprintf(buf, "%d", portNum);
src/jdk.jdwp.agent/share/native/libdt_socket/socketTransport.c:                sprintf(ebuf, "ERROR: Peer not allowed to connect: %s\n",
src/jdk.jdwp.agent/windows/native/libdt_socket/socket_md.c:        sprintf(buf, "winsock error %d", error);
src/jdk.jdwp.agent/windows/native/libjdwp/linker_md.c:        sprintf(holder, "%s.dll", fname);

openjdk · 2023-01-13T04:09:30Z

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

8299635: Hotspot update for deprecated sprintf in Xcode 14

Reviewed-by: dholmes, mikael

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 183 new commits pushed to the master branch:

fbe5ab0: 8300830: Remove redundant assertion in src/hotspot/share/runtime/javaCalls.cpp
1339461: 8300981: Build failure on 32-bit platforms after JDK-8281213
81d523d: Merge
60b8a98: 8300591: @SuppressWarnings option "lossy-conversions" missing from jdk.compiler module javadoc
a3ed7e9: 8300623: Lambda deserialization regression involving Enum method reference
cf46004: 8300272: Improve readability of the test JarWithOneNonDisabledDigestAlg
b678e70: 8300942: JDK-8299684 breaks x86 build
3be5758: 8300769: Remove G1CollectionSet::_inc_bytes_used_before
859ca75: 8300862: Remove some G1 collection set remembered set debugging code
048705c: 8300910: Remove metaprogramming/integralConstant.hpp
... and 173 more: https://git.openjdk.org/jdk/compare/4be94e435002d9d6cb594a393e9e35d650a9a0c9...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.

sspitsyn · 2023-01-13T08:00:55Z

@XueleiFan Could you, please, do not integrate until more cases with the same problem are fixed?

openjdk · 2023-01-13T08:01:04Z

@sspitsyn Only the author (@XueleiFan) is allowed to issue the reviewer command.

sspitsyn

More cases with the same issue were found.

XueleiFan · 2023-01-13T08:42:54Z

@XueleiFan Could you, please, do not integrate until more cases with the same problem are fixed?

Sure. That's on my plan. I am trying to figure out how to get these files included in the building. My build passed on MacOS and Linux, even with C2 enabled.

The update on test may be different from src update. I may divide the fix into two PRs, if I figure out how to build the missing uses of sprintf:s.

Thank for for the feedback. I appreciated it.

vidmik · 2023-01-14T00:41:28Z

Thank you for making these additional changes! I'm still seeing some issue building on linux-x64, for example:

src/hotspot/share/opto/regalloc.hpp:130:17: note: candidate: 'virtual char* PhaseRegAlloc::dump_register(const Node*, char*, size_t) const'
  130 |   virtual char *dump_register( const Node *n, char *buf, size_t buf_size) const = 0;
      |                 ^~~~~~~~~~~~~

XueleiFan · 2023-01-14T18:48:56Z

There are more uses of sprintf in some serviceability folders:

Yes. There are at least 57 src files that use sprintf function in components other than hotspot, as far as I can see. This PR is pretty big now. I would like to clean them up in an other PR so that it is easier to review.

src/hotspot/os/windows/os_windows.cpp

src/hotspot/share/utilities/globalDefinitions.hpp

dholmes-ora · 2023-01-17T01:36:43Z

I would suggest constraining this PR to src/hotspot and test/hotspot and deal with the JDK serviceability files in a different PR (and there may be other JDK files impacted too).

XueleiFan · 2023-01-18T19:35:08Z

I would suggest constraining this PR to src/hotspot and test/hotspot and deal with the JDK serviceability files in a different PR (and there may be other JDK files impacted too).

I agreed. This PR is mainly focus on the hotspot, except two test files for jdk management. The test clean up is done with this PR, but there are still a lot (57+) use in other src component. One or more PR will be filed for the remaining clean up.

src/hotspot/share/utilities/globalDefinitions.hpp

dholmes-ora · 2023-01-19T07:34:13Z

This PR is mainly focus on the hotspot, except two test files for jdk management.

I would suggest leaving those two management files for the serviceability PR.

vidmik · 2023-01-19T21:06:56Z

I was (still) puzzled by the remaining sprintf:s in the JDK and the fact that the build completed successfully even if not all of them had been address/updated, so I investigated it a bit and came to the conclusion that the Xcode/clang warning (which we turn into an error with -Werror) only seems to be generated for C++ files, not for C files.

For many reasons we may still want to fix the remaining occurrences as well, but for the immediate case of the new Xcode version it seems to be sufficient to fix the C++ files.

dholmes-ora

Nothing further from me. I think this is okay as-is now. Thanks.

XueleiFan · 2023-01-20T18:19:01Z

@sspitsyn I'm going to integrate the changeset. Did you have further comment or need more time?

XueleiFan · 2023-01-20T18:20:10Z

Nothing further from me. I think this is okay as-is now. Thanks.

@dholmes-ora Thank you for the review!

dholmes-ora · 2023-01-22T22:15:46Z

@XueleiFan You need two reviews before integrating a Hotspot change - thanks.

XueleiFan · 2023-01-24T15:30:46Z

@XueleiFan You need two reviews before integrating a Hotspot change - thanks.

May I get one more reviewer for this PR? Thanks!

vidmik

Looks good, thank you for doing this!

XueleiFan · 2023-01-25T15:40:45Z

Looks good, thank you for doing this!

@vidmik Thank you!

XueleiFan · 2023-01-25T15:41:51Z

/integrate

openjdk · 2023-01-25T15:43:02Z

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

f279c75: 8300805: Update autoconf build-aux files with latest from 2022-09-17
a23ff63: 8301086: jdk/internal/util/ByteArray/ReadWriteValues.java fails with CompilationError
61775c8: 8300997: Add curl support to createJMHBundle.sh
8a47429: 8295944: Move the Http2TestServer and related classes into a package of its own
c8ad600: 8301004: httpclient: Add more debug to HttpResponseInputStream
74e1a8b: 8300236: Use VarHandle access in Data(Input | Output)Stream classes
a5d8e12: 8300244: Replace NULL with nullptr in share/interpreter/
71107f4: 8300651: Replace NULL with nullptr in share/runtime/
3c61d5a: 8300659: Refactor TestMemoryAwareness to use WhiteBox api for host values
b2071f7: 8300657: Remove null filtering in CLD oop handle area
... and 186 more: https://git.openjdk.org/jdk/compare/4be94e435002d9d6cb594a393e9e35d650a9a0c9...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-01-25T15:43:16Z

@XueleiFan Pushed as commit e80b5ea.

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

8299635: More test issues for deprecated sprintf in Xcode 14

a036a07

openjdk bot added serviceability serviceability-dev@openjdk.org jmx jmx-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Jan 11, 2023

typo correction

1f7706f

XueleiFan marked this pull request as ready for review January 11, 2023 20:56

openjdk bot added the rfr Pull request is ready for review label Jan 11, 2023

sspitsyn approved these changes Jan 13, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 13, 2023

sspitsyn suggested changes Jan 13, 2023

View reviewed changes

openjdk bot removed the ready Pull request is ready to be integrated label Jan 13, 2023

more in src/hotspot

62a6be8

XueleiFan marked this pull request as draft January 14, 2023 03:19

openjdk bot removed the rfr Pull request is ready for review label Jan 14, 2023

Xuelei Fan added 2 commits January 14, 2023 11:33

missed update for debug mode

98c7d3d

correction for ppc

076f8b5

Merge

143887e

XueleiFan changed the title ~~8299635: More test issues for deprecated sprintf in Xcode 14~~ 8299635: Hotspot update for deprecated sprintf in Xcode 14 Jan 14, 2023

XueleiFan marked this pull request as ready for review January 14, 2023 19:38

openjdk bot added the rfr Pull request is ready for review label Jan 14, 2023

dholmes-ora reviewed Jan 17, 2023

View reviewed changes

src/hotspot/os/windows/os_windows.cpp Outdated Show resolved Hide resolved

dholmes-ora reviewed Jan 17, 2023

View reviewed changes

src/hotspot/share/utilities/globalDefinitions.hpp Show resolved Hide resolved

Xuelei Fan added 2 commits January 19, 2023 03:36

Remove FORBID_C_FUNCTION update

99d5c61

align splited lines

62589ae

dholmes-ora reviewed Jan 19, 2023

View reviewed changes

src/hotspot/share/utilities/globalDefinitions.hpp Outdated Show resolved Hide resolved

XueleiFan added 2 commits January 19, 2023 10:48

remove serviceability update

ca6959a

remove serviceability update and add back forbid of sprintf

ec0bb48

dholmes-ora approved these changes Jan 20, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jan 20, 2023

vidmik approved these changes Jan 25, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jan 25, 2023

openjdk bot closed this Jan 25, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 25, 2023

XueleiFan deleted the JDK-8299635 branch January 25, 2023 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8299635: Hotspot update for deprecated sprintf in Xcode 14 #11935

8299635: Hotspot update for deprecated sprintf in Xcode 14 #11935

XueleiFan commented Jan 11, 2023 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jan 11, 2023

openjdk bot commented Jan 11, 2023

mlbridge bot commented Jan 11, 2023 •

edited

Loading

vidmik commented Jan 11, 2023

XueleiFan commented Jan 11, 2023

XueleiFan commented Jan 12, 2023

sspitsyn left a comment

sspitsyn commented Jan 13, 2023 •

edited

Loading

openjdk bot commented Jan 13, 2023 •

edited

Loading

sspitsyn commented Jan 13, 2023 •

edited

Loading

openjdk bot commented Jan 13, 2023

sspitsyn left a comment

XueleiFan commented Jan 13, 2023

vidmik commented Jan 14, 2023

XueleiFan commented Jan 14, 2023 •

edited

Loading

dholmes-ora commented Jan 17, 2023

XueleiFan commented Jan 18, 2023

dholmes-ora commented Jan 19, 2023

vidmik commented Jan 19, 2023

dholmes-ora left a comment

XueleiFan commented Jan 20, 2023

XueleiFan commented Jan 20, 2023

dholmes-ora commented Jan 22, 2023

XueleiFan commented Jan 24, 2023

vidmik left a comment

XueleiFan commented Jan 25, 2023 •

edited

Loading

XueleiFan commented Jan 25, 2023

openjdk bot commented Jan 25, 2023

openjdk bot commented Jan 25, 2023

8299635: Hotspot update for deprecated sprintf in Xcode 14 #11935

8299635: Hotspot update for deprecated sprintf in Xcode 14 #11935

Conversation

XueleiFan commented Jan 11, 2023 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jan 11, 2023

openjdk bot commented Jan 11, 2023

mlbridge bot commented Jan 11, 2023 • edited Loading

Webrevs

vidmik commented Jan 11, 2023

XueleiFan commented Jan 11, 2023

XueleiFan commented Jan 12, 2023

sspitsyn left a comment

Choose a reason for hiding this comment

sspitsyn commented Jan 13, 2023 • edited Loading

openjdk bot commented Jan 13, 2023 • edited Loading

sspitsyn commented Jan 13, 2023 • edited Loading

openjdk bot commented Jan 13, 2023

sspitsyn left a comment

Choose a reason for hiding this comment

XueleiFan commented Jan 13, 2023

vidmik commented Jan 14, 2023

XueleiFan commented Jan 14, 2023 • edited Loading

dholmes-ora commented Jan 17, 2023

XueleiFan commented Jan 18, 2023

dholmes-ora commented Jan 19, 2023

vidmik commented Jan 19, 2023

dholmes-ora left a comment

Choose a reason for hiding this comment

XueleiFan commented Jan 20, 2023

XueleiFan commented Jan 20, 2023

dholmes-ora commented Jan 22, 2023

XueleiFan commented Jan 24, 2023

vidmik left a comment

Choose a reason for hiding this comment

XueleiFan commented Jan 25, 2023 • edited Loading

XueleiFan commented Jan 25, 2023

openjdk bot commented Jan 25, 2023

openjdk bot commented Jan 25, 2023

XueleiFan commented Jan 11, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 11, 2023 •

edited

Loading

sspitsyn commented Jan 13, 2023 •

edited

Loading

openjdk bot commented Jan 13, 2023 •

edited

Loading

sspitsyn commented Jan 13, 2023 •

edited

Loading

XueleiFan commented Jan 14, 2023 •

edited

Loading

XueleiFan commented Jan 25, 2023 •

edited

Loading