Replace sprintf with snprintf #13468

devreal · 2025-10-25T15:40:00Z

Mac OS' clang warns that printf is deprecated. Replace its use with snprintf. Also fixes some minor warnings found along the way (see commit messages).

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]>

Copilot

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 sprintf calls with snprintf including proper buffer size parameters
Removed unused acoll_module variables from barrier functions
Initialized remote_cid64 variable to prevent use of uninitialized memory
Corrected format specifiers from %ld to %llu for size_t types

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.

Copilot · 2025-10-25T15:40:30Z

ompi/mca/hook/comm_method/hook_comm_method_fns.c

            }

-            str = malloc(per + 32 + nleaderranks * 2 + 1);
+            bufsize = nleaderranks * (per + 1) + 1;


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.

Suggested change

bufsize = nleaderranks * (per + 1) + 1;

bufsize = 8 + (nleaderranks * (per + 1)) + 1;

This comment is incorrect. It accounts for the initial string by starting the loop below from 4 instead of 0. What I don't understand in this code is why it assumes there are always at least 4 leaderranks ?

We're in a branch where we have a minimum number of leaderranks: https://github.com/open-mpi/ompi/pull/13468/files#diff-4bcdf923dce180dbcc719b3472dd10527a4c285021962e918f5365a75eac81f6L756

I think copilot is right that I messed up the size computation (technically, that was copilot and I didn't pay enough attention ^^)

There are two usages of the allocated string. The first one requires8 + ((nleaderranks - 4) / 4) * 8 + 1 chars and the second one 2 * nleaderranks + 1 chars. You are allocating 8 + (nleaderranks * (per + 1)) + 1 with per being set to the number of digits in nleaderranks or 2 whichever is largest).

ompi/mca/hook/comm_method/hook_comm_method_fns.c

ompi/communicator/comm_cid.c

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]>

devreal · 2025-10-25T23:16:40Z

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]>

bwbarrett

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.

ompi/debuggers/ompi_common_dll_defs.h

bosilca · 2025-10-27T13:32:47Z

ompi/mca/hook/comm_method/hook_comm_method_fns.c

            }

-            str = malloc(per + 32 + nleaderranks * 2 + 1);
+            bufsize = nleaderranks * (per + 1) + 1;


This comment is incorrect. It accounts for the initial string by starting the loop below from 4 instead of 0. What I don't understand in this code is why it assumes there are always at least 4 leaderranks ?

bosilca · 2025-10-27T13:36:35Z

ompi/mca/hook/comm_method/hook_comm_method_fns.c

            for (i=4; i<nleaderranks; i+=4) {
-                sprintf(p, "%d", i);
+                snprintf(p, bufsize - (p - str), "%d", i);
                for (j=(int)strlen(p); j<8; ++j) {


Trying to understand the memory allocation above I got totally confused by this piece of code. Why is j looping up to 8 instead of up to per ? A similar code exists around line 650 and it clear the string up to per for each leaderrank.

devreal added 4 commits October 25, 2025 11:35

Replace sprintf with snprintf

01f60b8

Mac OS clang warns that sprintf is deprecated. Replace it with snprintf. Signed-off-by: Joseph Schuchart <[email protected]>

acoll: remove unused module variables

fa2d6e0

Signed-off-by: Joseph Schuchart <[email protected]>

comm_cid: initialize remote_cid64

bfee6d7

Clang warns about possible uninitialized use. Signed-off-by: Joseph Schuchart <[email protected]>

comm_cid: fix printf warnings

296713c

Signed-off-by: Joseph Schuchart <[email protected]>

devreal requested review from Sergei-Lebedev, Copilot and hppritcha October 25, 2025 15:40

devreal added the Target: main label Oct 25, 2025

Copilot AI reviewed Oct 25, 2025

View reviewed changes

bwbarrett reviewed Oct 25, 2025

View reviewed changes

ompi/mca/hook/comm_method/hook_comm_method_fns.c Outdated Show resolved Hide resolved

ompi/communicator/comm_cid.c Outdated Show resolved Hide resolved

devreal added 2 commits October 25, 2025 19:10

reduce-local: add parans around min/max macro

2fbeaa7

The expression `max_k < min(a, b)` may not produce what we think it does. Signed-off-by: Joseph Schuchart <[email protected]>

Make _ucount int instead of size_t in mpi_image_info

67ccb23

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]>

devreal added 2 commits October 25, 2025 19:18

Use PRIu64 for uint64_t

f298795

Signed-off-by: Joseph Schuchart <[email protected]>

Name buffer size in ompi_report_comm_methods

c9ba524

Signed-off-by: Joseph Schuchart <[email protected]>

bwbarrett approved these changes Oct 25, 2025

View reviewed changes

bosilca reviewed Oct 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Replace sprintf with snprintf #13468

Replace sprintf with snprintf #13468

devreal commented Oct 25, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Oct 25, 2025

Uh oh!

bosilca Oct 27, 2025

Uh oh!

devreal Oct 27, 2025

Uh oh!

bosilca Oct 27, 2025

Uh oh!

Uh oh!

Uh oh!

devreal commented Oct 25, 2025

Uh oh!

bwbarrett left a comment

Uh oh!

Uh oh!

bosilca Oct 27, 2025

Uh oh!

bosilca Oct 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	bufsize = nleaderranks * (per + 1) + 1;
	bufsize = 8 + (nleaderranks * (per + 1)) + 1;

Uh oh!

Replace sprintf with snprintf #13468

Are you sure you want to change the base?

Replace sprintf with snprintf #13468

Conversation

devreal commented Oct 25, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 25, 2025

Choose a reason for hiding this comment

Uh oh!

bosilca Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

devreal Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

bosilca Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

devreal commented Oct 25, 2025

Uh oh!

bwbarrett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bosilca Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

bosilca Oct 27, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants