glibc-2.35: spurious "buffer overrun" for strncpy#1215
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a buffer overrun warning in glibc-2.35 by correcting the length calculation in a strncpy call. The issue occurs specifically when the port parameter is "0".
- Corrects the buffer size calculation in
strncpyto prevent potential buffer overrun - Changes from
sizeof(port_str)+1-strlen(port)tosizeof(port_str)-strlen(port)
src/dmtcp_coordinator.cpp
Outdated
| char buf[16] = " "; | ||
| char port_str[16] =":"; | ||
| strncpy(port_str+1, port, sizeof(port_str)+1-strlen(port)); | ||
| strncpy(port_str+1, port, sizeof(port_str)-strlen(port)); |
There was a problem hiding this comment.
The length calculation is incorrect. It should be sizeof(port_str)-1-1 (or sizeof(port_str)-2) to account for both the ':' character at position 0 and the null terminator. The current calculation uses strlen(port) which is the length of the source string, not the available space in the destination buffer.
| strncpy(port_str+1, port, sizeof(port_str)-strlen(port)); | |
| strncpy(port_str+1, port, sizeof(port_str)-2); |
There was a problem hiding this comment.
@aayushi363 , I have a question. How were you compiling this? From my reading, we should not have seen this, unless our g++ compilation was explicitly using -D_FORTIFY_SOURCE=XXX was being explicitly used by us. If so, where did this flag come from in compiling DMTCP?
There was a problem hiding this comment.
Neither we are using this flag explicitly while compiling nor it is mentioned in any of the make files.
There was a problem hiding this comment.
I think I like the comment of Copilot. Have you tried that change? If that works with port == 0, then let's use that one. As it indicates, strlen(port) could be just 1, and the correct constant to subtract here really should be 2.
There was a problem hiding this comment.
I agree, tying it to strlen(port) isn’t reliable, and using sizeof(port_str) - 2 is safer. I tested it, and the change works.
This occurs when port=="0"
aayushi363
force-pushed
the
dev/glibc-buffer-overrun
branch
from
a21d80c to
b1a395f
Compare
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaced dynamic strncpy length computation for port suffixing with a fixed bound (sizeof(port_str) - 2) when copying into port_str+1. This standardizes the number of copied characters, reduces boundary overrun risk, and preserves existing control flow and usage of the resulting short command line. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/dmtcp_coordinator.cpp (2)
1598-1602: Prefer snprintf for clarity and explicit NUL-termination (minor).strncpy here relies on the earlier zero-initialization of port_str for NUL-termination. Using snprintf makes the intent obvious and guarantees termination without depending on initialization details.
Apply this diff:
- strncpy(port_str+1, port, sizeof(port_str)-2); + // Ensure ':' + port fits within port_str (16 bytes incl. NUL) + // Truncate port to at most 14 chars if longer. + snprintf(port_str, sizeof(port_str), ":%.*s", + (int)sizeof(port_str) - 2, port);
1601-1605: Optionally check short_name() return to avoid setting a blank comm name (very unlikely here).short_name() can return NULL when the suffix is too long for the 16-byte comm buffer. Not an issue for real port strings, but a defensive check avoids PR_SET_NAME being set to spaces if the suffix logic changes later.
Apply this diff:
- short_name(buf, argv0, sizeof buf, port_str); + if (!short_name(buf, argv0, sizeof buf, port_str)) { + // Fallback: leave buf as-is or set a minimal identifier + // e.g., strncpy(buf, "dmtcp", sizeof(buf)-1); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/dmtcp_coordinator.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
src/dmtcp_coordinator.cpp (1)
1600-1601: Correct bound for strncpy; fixes the fortify overrun and handles port == "0".Using sizeof(port_str) - 2 is the right bound when writing at port_str + 1: it leaves room for the leading ':' and the trailing NUL. Given the zero-initialization from the ":" initializer, the buffer remains NUL-terminated even when truncation occurs. This should eliminate the glibc-2.35 _FORTIFY_SOURCE warning.
This occurs when port=="0"
Summary by CodeRabbit