8347707: Standardise the use of os::snprintf and os::snprintf_checked #26849

dholmes-ora · 2025-08-19T22:02:30Z

This is a proposal to standardize on the use of os::snprintf and os::snprintf_checked across the hotspot code base, and to disallow use of the C library variants. (It does not touch use of jio_printf at all.)

From: https://bugs.openjdk.org/browse/JDK-8347707

The platform snprintf/vsnprintf returns -1 on error, else if the buffer is large enough returns the number of bytes written (excluding the null byte), else (buffer is too small) the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available. Thus, a return value of size or more means that the output was truncated.

To provide a consistent approach to error handling and truncation management, we provide os::xxx wrapper functions as described below and forbid the use of the library ::vsnprintf and ::snprintf.

The potential errors are, generally speaking, not something we should encounter in our own well-written code:

encoding error: not applicable as we are not using extended character sets
invalid parameters (null buffers, specifying a limit > size of the buffer [Windows], things of this nature)
mal-formed formatting directives
overflow error (POSIX) if the required buffer size exceeds INT_MAX (as we return int).

As these should simply never occur, we handle the checks for -1 at the lowest-level (os::vsnprintf) with an assertion, and accompanying precondition assertions.

The potential clients of this API then fall into a number of camps:

Those who have sized their buffer correctly, don't need the return value for subsequent use, and for whom truncation (if it were possible) would be a programming error.

For these clients we have void os::snprintf_checked - which returns nothing and asserts on truncation.

Those who have sized their buffer correctly, but do need the return value for subsequent operations (e.g. chains of snprintf where you advance the buffer pointer based on previous writes), but again for whom truncation should never happen.

For these clients we have os::snprintf, but they have to add their own assertion for no truncation.

Those who present a buffer but know that truncation is a possibility, but don't need to do anything about it themselves, and for whom the return value is of no use.

These clients also use os::snprintf_checked. The truncation assertion can be useful for guiding buffer sizing decisions, but in product mode truncation is not an error.

Those who present a buffer but know that truncation is a possibility, and either need to handle it themselves, or else need to use the return value in subsequent operations.

These clients are also directed to use os::snprintf.

In summary we provide the following API:

[[nodiscard]] int os::vsnprintf is the building block for the other methods, it:
- asserts on precondition failures
- asserts on error
- guarantees null-termination in the case of unexpected errors (as the standards are still unclear on that point
- is declared [[nodiscard[]] so that callers cannot ignore the return value (they can explicitly cast to void to indicate they don't need it)
void os::snprintf_checked
- calls `os::vnsprintf`` so asserts on errors
- asserts on truncation
[[nodiscard]] int os::snprintf
- calls os::vnsprintf so asserts on errors

In terms of the effects on the existing code we:

Change callers of ::snprintf/os::snprintf that ignore the return value and ensure the buffer is large enough to use os::snprintf_checked
- those that allow truncation to happen must use os::snprintf.
Change all callers of ::snprintf/os::snprintf that use the return value to use os::snprintf, plus any additional assertions needed
Change the 9 callers of os::snprintf_checked that do use the return value, to use os::snprintf with their own assertions added
Callers of os::vnsprintf are adjusted as needed

The PR is comprising multiple dependent commits so that you can view things in stages. There are 46 modified files. The bulk of the changes replace calls to snprintf/os::snprintf with calls to os::snprintf_checked.

Note the use of [[nodiscard]] is permitted but not yet documented as such in the style-guide.

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-8347707: Standardise the use of os::snprintf and os::snprintf_checked (Enhancement - P4)

Reviewers

Kim Barrett (@kimbarrett - Reviewer)
Fredrik Bredberg (@fbredber - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 26849

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

Using diff file

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

Using Webrev

Link to Webrev Comment

Make os::vsnprintf responsible for error checking.

Change callers that need the return value to use os::snprintf, and add truncation checks.

…ld not cause a problem during normal execution in debug testing.

bridgekeeper · 2025-08-19T22:03:10Z

👋 Welcome back dholmes! 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-08-19T22:03:50Z

@dholmes-ora 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:

8347707: Standardise the use of os::snprintf and os::snprintf_checked

Reviewed-by: kbarrett, fbredberg

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

3641c32: 8365994: ZGC: Incorrect type signature in ZMappedCache comparator
2ae3ea2: 8366035: Simplify CPUTimeCounters::publish_gc_total_cpu_time
68abf76: 8366105: Update link to the external RuleBasedBreakIterator documentation
... and 84 more: https://git.openjdk.org/jdk/compare/3bbaa772b0bb94694940156ec0ce421f87f02be7...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-08-19T22:04:48Z

@dholmes-ora The following labels will be automatically applied to this pull request:

graal
hotspot
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 · 2025-08-19T22:09:19Z

Webrevs

01: Full - Incremental (38003a22)
00: Full (2de6ac98)

kimbarrett

I noticed there are a number of calls with buflen - 1. Probably from the bad
old days when some snprintf variants didn't guarantee null termination. I
didn't see forced null terminations for the few I spot-checked though, and
it's not obvious in some cases that the final byte is certain to be null going
in. Nothing to be done about that here, but there might be some following
cleanup work called for.

src/hotspot/os/linux/os_linux.cpp

src/hotspot/os/posix/attachListener_posix.cpp

kimbarrett · 2025-08-20T20:12:35Z

src/hotspot/share/opto/idealGraphPrinter.cpp

-        int written_chars = os::snprintf_checked(buffer, sizeof(buffer), "%d", value);
-        if (written_chars <= 4) {
+        int written_chars = os::snprintf(buffer, sizeof(buffer), "%d", value);
+        if (written_chars > 0 && written_chars <= 4) {


I don't understand the addition of written_chars > 0 here and line 658?

Well in theory, in a release build, os::snprintf could return -1 and I didn't want any static analysis tools pointing this out to me later.

kimbarrett · 2025-08-20T20:15:25Z

src/hotspot/share/runtime/os.hpp

-  static int snprintf_checked(char* buf, size_t len, const char* fmt, ...) ATTRIBUTE_PRINTF(3, 4);
+  // Performs vsnprintf and asserts the result is non-negative (so there was not
+  // an encoding error or any other kind of usage error).
+  [[nodiscard]] static int vsnprintf(char* buf, size_t len, const char* fmt, va_list args) ATTRIBUTE_PRINTF(3, 0);


Consider moving the ATTRIBUTE_PRINTF to the front so all the attributes are together?
And maybe a line break between the attributes and the signature, just to avoid pushing the
signature way over to the right.

I have done that and placed each attribute on its own line (we should have a style guide entry for this :) ). But I note that all the other ATTRIBUTE_PRINTF's are placed after the function.

What you've done here matches what I did when adding [[noreturn]] to report_xxx functions
in debug.hpp. So I'm good with that. The style guide already has this guidance:
https://github.com/openjdk/jdk/blame/02fe095d29994bec28c85beb6bf2a69b0f49b206/doc/hotspot-style.md#L1120-L1122
There's just lots of old, non-conforming code. :)
I forgot about that when I said "Consider moving", and should have pointed to the style guide.
Whether and where there should be line breaks is something the style guide leaves to authors and reviewers.

dean-long · 2025-08-20T21:20:50Z

Could we map snprintf to os::snprintf_checked with a macro or some kind of namespace magic? It would reduce the number of files changed and catch any new cases of snprintf that might get accidentally added in the future.

src/hotspot/cpu/aarch64/vm_version_aarch64.cpp

src/hotspot/share/gc/shared/satbMarkQueue.cpp

dean-long · 2025-08-20T22:05:10Z

src/hotspot/share/utilities/virtualizationSupport.cpp

    if (sg_error == VMGUESTLIB_ERROR_SUCCESS) {
      has_host_information = true;
-      os::snprintf(host_information, sizeof(host_information), "%s", result_info);
+      os::snprintf_checked(host_information, sizeof(host_information), "%s", result_info);


Are these two guaranteed not to overflow/truncate?

The issue is not whether they can overflow, but if they do is it something we want to detect during testing so we can take action - e.g. by increasing the buffer size. This is very subjective, but my initial position in most cases has been yes.

dean-long · 2025-08-20T22:13:31Z

src/hotspot/os/aix/attachListener_aix.cpp

-    snprintf(fn, sizeof(fn), "%s/.attach_pid%d",
-             os::get_temp_directory(), os::current_process_id());
+    os::snprintf_checked(fn, sizeof(fn), "%s/.attach_pid%d",
+                         os::get_temp_directory(), os::current_process_id());


This could fail if os::get_temp_directory() returns an extremely long path. How about doing a truncation check like at line 354?

Nevermind, same thing. We would need to fix a lot of code if os::get_temp_directory() returned a pathologically long string.

I've changed line 352 as per Kim's comment above because snprintf followed by an assert for truncation is what snprintf_checked does.

Again the question to ask is: if we hit this during testing do we think it indicates we need to increase the buffer size. Again I am initially in the yes camp.

src/hotspot/os/aix/os_aix.cpp

src/hotspot/os/aix/porting_aix.cpp

dholmes-ora · 2025-08-21T05:57:51Z

Thanks for looking at this @dean-long !

Could we map snprintf to os::snprintf_checked with a macro or some kind of namespace magic? It would reduce the number of files changed and catch any new cases of snprintf that might get accidentally added in the future.

Raw snprintf is now prohibited so no future accidental additions are possible. I don't think hiding the fact we are not using the library snprintf is a good thing either.

kimbarrett

Looks good.

dholmes-ora · 2025-08-25T07:07:00Z

Thanks for the Review Kim!

fbredber

A good looking PR, and a good step in the right direction. Nice!

But as @kimbarrett I also noticed there are a number of calls with buflen - 1, which in my case consumed too mush of the review time. So it would be nice to see a follow up that deals with code like this.

dholmes-ora · 2025-08-27T06:47:45Z

Thanks for the review @fbredber . I have added your cleanup suggestion to JDK-8365896 as well.

dholmes-ora · 2025-08-31T21:33:42Z

/integrate

openjdk · 2025-08-31T21:34:19Z

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

bdc3981: 8361521: BogusFocusableWindowState.java fails with StackOverflowError on Linux
9339a6a: 8361593: Commented dead code in JDK-8342868 can be removed
12e6a0b: 8366208: Unexpected exception in sun.java2d.cmm.lcms.LCMSImageLayout
... and 147 more: https://git.openjdk.org/jdk/compare/3bbaa772b0bb94694940156ec0ce421f87f02be7...master

Your commit was automatically rebased without conflicts.

openjdk · 2025-08-31T21:34:25Z

@dholmes-ora Pushed as commit 80ab094.

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

dholmes-ora · 2025-08-31T21:35:23Z

Note, before integrating I merged locally with master, checked the incoming changes and re-ran tier 1-3 builds.

dholmes-ora added 8 commits July 23, 2025 04:34

Mark os::vsnprintf as nodiscard and update the callsites.

6fdfe1e

Make os::vsnprintf responsible for error checking.

Change os::snprintf_checked to be void function.

c63d4bb

Change callers that need the return value to use os::snprintf, and add truncation checks.

Change raw snprintf to os::snprintf_checked, whereever truncation wou…

f7fe658

…ld not cause a problem during normal execution in debug testing.

Change return-value using snprintf to explicit os::snprintf

e2f51fc

Forbid C library snprintf

9c2ffb7

Convert os::snprintf() to os::snprintf_checked() where appropriate.

615a59d

Make os::snprintf "nodiscard" and adjust final callsites

c10b6d8

Merge

2de6ac9

openjdk bot added graal graal-dev@openjdk.org serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org rfr Pull request is ready for review labels Aug 19, 2025

kimbarrett reviewed Aug 20, 2025

View reviewed changes