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. > ... src/hotspot/os/aix/attachListener_aix.cpp line 423: > 421: log_trace(attach)("Failed to find attach file: %s, trying > alternate", fn); > 422: os::snprintf_checked(fn, sizeof(fn), "%s/.attach_pid%d", > 423: 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? 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. 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); src/hotspot/os/aix/porting_aix.cpp line 939: > 937: // retrieve the path to the currently running executable binary > 938: // to open it > 939: os::snprintf_checked(buffer, 100, "/proc/%ld/object/a.out", > (long)getpid()); Suggestion: os::snprintf_checked(buffer, sizeof(buffer), "/proc/%ld/object/a.out", (long)getpid()); src/hotspot/os/aix/porting_aix.cpp line 1157: > 1155: } > 1156: if (ebuf != nullptr && ebuflen > 0) { > 1157: os::snprintf_checked(ebuf, ebuflen - 1, "%s", error_report); Suggestion: (void) os::snprintf(ebuf, ebuflen - 1, "%s", error_report); src/hotspot/share/gc/shared/satbMarkQueue.cpp line 324: > 322: > 323: virtual void do_thread(Thread* t) { > 324: os::snprintf_checked(_buffer, SATB_PRINTER_BUFFER_SIZE, "Thread: > %s", t->name()); Suggestion: (void) os::snprintf(_buffer, SATB_PRINTER_BUFFER_SIZE, "Thread: %s", t->name()); Can this be a JavaThread with an arbitrarily long name()? src/hotspot/share/utilities/virtualizationSupport.cpp line 76: > 74: if (sg_error == VMGUESTLIB_ERROR_SUCCESS) { > 75: has_host_information = true; > 76: os::snprintf_checked(host_information, sizeof(host_information), > "%s", result_info); Are these two guaranteed not to overflow/truncate? ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/26849#discussion_r2289418676 PR Review Comment: https://git.openjdk.org/jdk/pull/26849#discussion_r2289425021 PR Review Comment: https://git.openjdk.org/jdk/pull/26849#discussion_r2289426575 PR Review Comment: https://git.openjdk.org/jdk/pull/26849#discussion_r2289429123 PR Review Comment: https://git.openjdk.org/jdk/pull/26849#discussion_r2289430247 PR Review Comment: https://git.openjdk.org/jdk/pull/26849#discussion_r2289397586 PR Review Comment: https://git.openjdk.org/jdk/pull/26849#discussion_r2289406269
