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

[David Holmes]

Seems there is no real interest in a pre-discussion so I will make the 
PR non-draft.

David

On 25/07/2025 11:54 am, David Holmes 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.
> 
> 4. 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 dn'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
> 
> There is a draft PR comprising of multiple dependent commits so that you 
> can view things in stages. There has been a suggestion to integrate this 
> in a staged way under different JBS issues to make reviewing easier. 
> There are 46 modified files. The bulk of changes replace calls to 
> snprintf/os::snprintf with calls to os::snprintf_checked.
> 
> https://github.com/openjdk/jdk/pull/26470
> 
> Comments welcomed.
> 
> Thanks,
> David
>