8308071: [REDO] update for deprecated sprintf for src/utils by XueleiFan · Pull Request #13995 · openjdk/jdk

XueleiFan · 2023-05-15T18:46:00Z

Hi,

This is a redo of JDK-8307855, where issues were found after integration.

The sprintf is deprecated in Xcode 14, and Microsoft Virtual Studio, because of security concerns. The issue was addressed in JDK-8296812 for building failure, and JDK-8299378/JDK-8299635/JDK-8301132 for testing issues . This is a break-down update for sprintf uses in the src/utils directory.

Thanks,
Xuelei

Progress

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

Issue

JDK-8308071: [REDO] update for deprecated sprintf for src/utils (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 13995

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2023-05-15T18:47:34Z

👋 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-05-15T18:49:59Z

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

build
hotspot-compiler

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-05-15T20:11:45Z

Webrevs

XueleiFan · 2023-05-15T20:13:33Z

/reviewers 2

openjdk · 2023-05-15T20:13:57Z

@XueleiFan
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

vidmik · 2023-05-15T21:47:19Z

src/utils/hsdis/binutils/hsdis-binutils.c

-  if (delays)  sprintf(p += strlen(p), " delay='%d'", delays);
+  size_t used_size = strlen(close);
+  char* p = buf + used_size;
+  bufsize -= used_size;


May not happen in practice, but if used_size is larger than bufsize this will wrap to a very large value. Perhaps the strcpy above should also be an snprintf, and the return value handled the same way as for the subsequent snprintf calls?

I think it is safe as the buf size has been checked at around line 230. However, it may make the code easier to read if replacing strcpy with snprintf. The patch was updated accordingly.

This and all uses of snprintf in this change are incorrect. If the output is truncated, snprintf returns the
number of characters that would have been written if there had been enough space. That is, the result
may be larger than bufsize.

This and all uses of snprintf in this change are incorrect. If the output is truncated, snprintf returns the number of characters that would have been written if there had been enough space. That is, the result may be larger than bufsize.

The correctness of this change depends on the fact that the buffer has sufficient capacity, which has been checked at line 230. I agreed that this is not a typical use of snprintf that the returned value is not checked. I will make an update to check the returned value of snprintf.

OK, I missed that. (The relevant code doesn't show up in the default github diff. I really ought to know better
than to use that view for reviewing.) Even having been pointed to the code, I had to do some counting and
such to convince myself that it was safe. A bit of commentary might save some time for the next reader.

kimbarrett · 2023-05-17T03:26:45Z

src/utils/hsdis/binutils/hsdis-binutils.c

-  if (delays)  sprintf(p += strlen(p), " delay='%d'", delays);
+  size_t used_size = strlen(close);
+  char* p = buf + used_size;
+  bufsize -= used_size;


This and all uses of snprintf in this change are incorrect. If the output is truncated, snprintf returns the
number of characters that would have been written if there had been enough space. That is, the result
may be larger than bufsize.

kimbarrett · 2023-05-17T08:48:37Z

src/utils/hsdis/binutils/hsdis-binutils.c

-  if (dsize)   sprintf(p += strlen(p), " dsize='%d'", dsize);
-  if (delays)  sprintf(p += strlen(p), " delay='%d'", delays);
+  size_t used_size = snprintf(buf, bufsize, "%s", close);
+  if ((used_size < 0) || (used_size >= bufsize)) {


(used_size < 0) is tautologically false, since used_size is a size_t, so unsigned. I'm somewhat surprised
this doesn't trigger a warning from some compiler.

Updated to use int to replace size_t.. Thank you for the catching.

bufsize is size_t, so that's a comparison between signed and unsigned values, which I think some compilers
will warn about. Maybe the preceding check for negative is getting rid of that? But will that still occur in
a slowdebug build, or will the lack of optimization lead to a warning?

As always, this comment helps a lot. Thank you!

Updated to cast int to size_t explicitly.

@kimbarrett Did you have a chance to have another look? Please let me know if you prefer to the update that the returned value of snprintf() is not checked because the memory size has been checked previously.

kimbarrett · 2023-05-17T08:55:42Z

src/utils/hsdis/binutils/hsdis-binutils.c

-  if (delays)  sprintf(p += strlen(p), " delay='%d'", delays);
+  size_t used_size = strlen(close);
+  char* p = buf + used_size;
+  bufsize -= used_size;


OK, I missed that. (The relevant code doesn't show up in the default github diff. I really ought to know better
than to use that view for reviewing.) Even having been pointed to the code, I had to do some counting and
such to convince myself that it was safe. A bit of commentary might save some time for the next reader.

bridgekeeper · 2023-06-27T19:47:08Z

@XueleiFan This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2023-07-26T00:32:42Z

@XueleiFan This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

8308071: [REDO] update for deprecated sprintf for src/utils

9ac9570

openjdk bot added build build-dev@openjdk.org hotspot-compiler hotspot-compiler-dev@openjdk.org labels May 15, 2023

XueleiFan marked this pull request as ready for review May 15, 2023 20:06

openjdk bot added the rfr Pull request is ready for review label May 15, 2023

vidmik reviewed May 15, 2023

View reviewed changes

replace strcpy with snprintf

1f833d5

kimbarrett suggested changes May 17, 2023

View reviewed changes

check returned value of snprintf

dd6ddbc

kimbarrett suggested changes May 17, 2023

View reviewed changes

XueleiFan added 2 commits May 17, 2023 07:50

size_t to int

244278a

compare between int and size_t

ccef71e

bridgekeeper bot closed this Jul 26, 2023

Conversation

XueleiFan commented May 15, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented May 15, 2023

Uh oh!

openjdk bot commented May 15, 2023

Uh oh!

mlbridge bot commented May 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

XueleiFan commented May 15, 2023

Uh oh!

openjdk bot commented May 15, 2023

Uh oh!

vidmik May 15, 2023 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bridgekeeper bot commented Jun 27, 2023

Uh oh!

bridgekeeper bot commented Jul 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

XueleiFan commented May 15, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented May 15, 2023 •

edited

Loading

vidmik May 15, 2023 •

edited

Loading