On Tue, 19 Aug 2025 22:02:30 GMT, David Holmes <dhol...@openjdk.org> wrote:
> 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: > > 1. 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. > > 2. 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. > > 3. 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. > ... 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 line 4801: > 4799: char buf [16]; // according to glibc manpage, 16 chars incl. '/0' > 4800: (void) os::snprintf(buf, sizeof(buf), "%s", name); > 4801: buf[sizeof(buf) - 1] = '\0'; pre-existing. Adding null termination at the end hasn't been needed for a while. There are probably others like this that can be deferred to following cleanup. src/hotspot/os/posix/attachListener_posix.cpp line 351: > 349: int n = os::snprintf(fn, UNIX_PATH_MAX, "%s/.java_pid%d", > 350: os::get_temp_directory(), > os::current_process_id()); > 351: assert(n < (int)UNIX_PATH_MAX, "java_pid file name buffer overflow"); I don't see any other uses of `n` besides this assert. Maybe this should be using `os::snprintf_checked`? src/hotspot/share/opto/idealGraphPrinter.cpp line 644: > 642: // Only use up to 4 chars and fall back to a generic "I" to keep > it short. > 643: int written_chars = os::snprintf(buffer, sizeof(buffer), "%d", > value); > 644: if (written_chars > 0 && written_chars <= 4) { I don't understand the addition of `written_chars > 0` here and line 658? src/hotspot/share/runtime/os.hpp line 810: > 808: // Performs vsnprintf and asserts the result is non-negative (so there > was not > 809: // an encoding error or any other kind of usage error). > 810: [[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. ------------- PR Review: https://git.openjdk.org/jdk/pull/26849#pullrequestreview-3138069291 PR Review Comment: https://git.openjdk.org/jdk/pull/26849#discussion_r2289177595 PR Review Comment: https://git.openjdk.org/jdk/pull/26849#discussion_r2289182600 PR Review Comment: https://git.openjdk.org/jdk/pull/26849#discussion_r2289195737 PR Review Comment: https://git.openjdk.org/jdk/pull/26849#discussion_r2289201763
