-
Notifications
You must be signed in to change notification settings - Fork 928
Replace sprintf with snprintf #13468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Mac OS clang warns that sprintf is deprecated. Replace it with snprintf. Signed-off-by: Joseph Schuchart <[email protected]>
Signed-off-by: Joseph Schuchart <[email protected]>
Clang warns about possible uninitialized use. Signed-off-by: Joseph Schuchart <[email protected]>
Signed-off-by: Joseph Schuchart <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses deprecation warnings from macOS clang by replacing all sprintf calls with the safer snprintf function throughout the codebase. The changes add buffer size checking to prevent potential buffer overflows. Additionally, the PR fixes minor warnings including unused variables, an uninitialized variable, and incorrect format specifiers for size_t types.
Key changes:
- Replaced all
sprintfcalls withsnprintfincluding proper buffer size parameters - Removed unused
acoll_modulevariables from barrier functions - Initialized
remote_cid64variable to prevent use of uninitialized memory - Corrected format specifiers from
%ldto%lluforsize_ttypes
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/support/support.h | Updated test assertion macro to use snprintf |
| test/simple/parallel_w8.c | Replaced sprintf with snprintf for MPI processor name fallback |
| test/simple/parallel_w64.c | Replaced sprintf with snprintf for MPI processor name fallback |
| test/simple/parallel_r8.c | Replaced sprintf with snprintf for MPI processor name fallback |
| test/simple/parallel_r64.c | Replaced sprintf with snprintf for MPI processor name fallback |
| test/simple/no-disconnect.c | Replaced sprintf with snprintf for buffer preparation |
| test/simple/crisscross.c | Replaced sprintf with snprintf for MPI processor name fallback |
| test/datatype/position.c | Updated hex dump function to use snprintf with proper bounds checking |
| opal/util/timings.h | Replaced sprintf with snprintf in timing macros |
| opal/mca/btl/tcp/btl_tcp_component.c | Updated parameter name formatting to use snprintf |
| opal/mca/btl/smcuda/btl_smcuda_component.c | Replaced sprintf with snprintf for FIFO path construction |
| opal/mca/btl/smcuda/btl_smcuda.c | Replaced sprintf with snprintf for FIFO path construction |
| ompi/mca/sharedfp/lockedfile/sharedfp_lockedfile.c | Updated filename construction to use snprintf |
| ompi/mca/hook/comm_method/hook_comm_method_fns.c | Extensively updated string formatting with snprintf and proper buffer size tracking |
| ompi/mca/common/monitoring/common_monitoring_coll.c | Updated collective monitoring to use snprintf with buffer size tracking |
| ompi/mca/coll/ucc/coll_ucc_module.c | Replaced sprintf with snprintf for UCC configuration strings |
| ompi/mca/coll/ftagree/coll_ftagree_earlyreturning.c | Updated debug printing and message formatting to use snprintf |
| ompi/mca/coll/acoll/coll_acoll_barrier.c | Removed unused acoll_module variables |
| ompi/instance/instance.c | Replaced sprintf with snprintf for error message construction |
| ompi/communicator/comm_cid.c | Fixed format specifiers and initialized variable, updated error messages to use snprintf |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| str = malloc(per + 32 + nleaderranks * 2 + 1); | ||
| bufsize = nleaderranks * (per + 1) + 1; |
Copilot
AI
Oct 25, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The buffer size calculation doesn't account for the initial header string '0 1 2 3 ' which is 8 characters. This could lead to a buffer overflow when writing to str. The calculation should be bufsize = 8 + (nleaderranks * per) + 1; to properly include the header.
| bufsize = nleaderranks * (per + 1) + 1; | |
| bufsize = 8 + (nleaderranks * (per + 1)) + 1; |
The expression `max_k < min(a, b)` may not produce what we think it does. Signed-off-by: Joseph Schuchart <[email protected]>
The struct stores offsets in a struct so there is no reason why this should be size_t. The compiler warns about a comparison `<0` for size_t in `ompi_field_offset`. Signed-off-by: Joseph Schuchart <[email protected]>
|
I keep adding minor things to this PR that I found on the way. I can take them out into a separate PR if that is cleaner. |
Signed-off-by: Joseph Schuchart <[email protected]>
Signed-off-by: Joseph Schuchart <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please squash the two cid printf commits. Otherwise, looks good to me. Also, I think this set is fine in size, but I wouldn't make it much bigger.
Mac OS' clang warns that
printfis deprecated. Replace its use withsnprintf. Also fixes some minor warnings found along the way (see commit messages).