On Wed, 20 Aug 2025 22:18:28 GMT, Dean Long <dl...@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... > > src/hotspot/os/aix/os_aix.cpp line 1055: > >> 1053: if (ebuf != nullptr && ebuflen > 0) { >> 1054: os::snprintf_checked(ebuf, ebuflen - 1, "%s, LIBPATH=%s, >> LD_LIBRARY_PATH=%s : %s", >> 1055: filename, ::getenv("LIBPATH"), >> ::getenv("LD_LIBRARY_PATH"), error_report); > > Suggestion: > > (void) os::snprintf(ebuf, ebuflen - 1, "%s, LIBPATH=%s, > LD_LIBRARY_PATH=%s : %s", > filename, ::getenv("LIBPATH"), > ::getenv("LD_LIBRARY_PATH"), error_report); > > This could easily truncate, based on LIBPATH and LD_LIBRARY_PATH. Yes but again if that happens during testing we need to know and fix the buffer size to get the non-truncated values. > src/hotspot/os/aix/os_aix.cpp line 1097: > >> 1095: struct utsname name; >> 1096: uname(&name); >> 1097: os::snprintf_checked(buf, buflen, "%s %s", name.release, >> name.version); > > Suggestion: > > (void) os::snprintf(buf, buflen, "%s %s", name.release, name.version); See previous replies. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26849#discussion_r2289995061 PR Review Comment: https://git.openjdk.org/jdk/pull/26849#discussion_r2289996137
