MEM_Connector: Fix CID 1442918 #854
Conversation
likema
force-pushed
the
cid-1442918
branch
3 times, most recently
from
319ec28 to
5240636
Compare
|
@jwillemsen What about it? Although it is detected by Coverity, it is a bug indeed. But it is hard to reproduce it. |
|
See https://www.remedy.nl for our services, reviewing issues takes time and we are looking for sponsoring for that. |
WalkthroughThe changes in the Changes
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 1
🧹 Nitpick comments (4)
ACE/ace/MEM_Connector.cpp (4)
104-107: LGTM! Consider enhancing the error message.Good improvement to check for exact byte count instead of just -1. This catches partial reads that could lead to using an incomplete strategy value.
Consider making the error message more descriptive by including the actual bytes received:
- ACE_TEXT ("ACE_MEM_Connector::connect error receiving strategy\n")), + ACE_TEXT ("ACE_MEM_Connector::connect error receiving strategy: expected %d bytes\n"), + sizeof (ACE_INT16)),
117-120: LGTM! Consider enhancing the error message.Good improvement to check for exact byte count instead of just -1. This catches partial writes that could lead to strategy mismatch between client and server.
Consider making the error message more descriptive by including the actual bytes sent:
- ACE_TEXT ("ACE_MEM_Connector::connect error sending strategy\n")), + ACE_TEXT ("ACE_MEM_Connector::connect error sending strategy: expected %d bytes\n"), + sizeof (ACE_INT16)),
122-127: LGTM! Consider enhancing the error message.Good improvements:
- Explicit initialization of buf_len to 0 ensures deterministic behavior.
- Checking for exact byte count catches partial reads that could lead to buffer overflow or underflow.
Consider making the error message more descriptive by including the actual bytes received:
- ACE_TEXT ("ACE_MEM_Connector::connect error receiving shm filename length\n")), + ACE_TEXT ("ACE_MEM_Connector::connect error receiving shm filename length: expected %d bytes\n"), + sizeof (buf_len)),
Line range hint
104-129: Architectural improvement: Robust IPC error handling.The changes improve the robustness of the IPC protocol by ensuring complete data transmission. By checking for exact byte counts instead of just error conditions (-1), the code now detects partial reads/writes that could lead to protocol misalignment between client and server. This is particularly important for shared memory setup where protocol errors could lead to memory corruption.
The fact that the bug is hard to reproduce suggests it might occur due to timing-dependent partial reads/writes, which these changes now properly handle.
| if (ACE::recv (new_handle, buf, buf_len) != buf_len) | ||
| ACELIB_ERROR_RETURN ((LM_DEBUG, | ||
| ACE_TEXT ("ACE_MEM_Connector::connect error receiving shm filename.\n")), | ||
| -1); |
There was a problem hiding this comment.
Add bounds checking to prevent buffer overflow.
While checking for exact byte count is good, there's a potential buffer overflow risk if buf_len is larger than MAXPATHLEN.
Add bounds checking before the receive operation:
+ if (buf_len > MAXPATHLEN)
+ ACELIB_ERROR_RETURN ((LM_DEBUG,
+ ACE_TEXT ("ACE_MEM_Connector::connect error: shm filename length %d exceeds maximum %d\n"),
+ buf_len, MAXPATHLEN),
+ -1);
+
if (ACE::recv (new_handle, buf, buf_len) != buf_len)
ACELIB_ERROR_RETURN ((LM_DEBUG,
- ACE_TEXT ("ACE_MEM_Connector::connect error receiving shm filename.\n")),
+ ACE_TEXT ("ACE_MEM_Connector::connect error receiving shm filename: expected %d bytes\n"),
+ buf_len),
-1);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (ACE::recv (new_handle, buf, buf_len) != buf_len) | |
| ACELIB_ERROR_RETURN ((LM_DEBUG, | |
| ACE_TEXT ("ACE_MEM_Connector::connect error receiving shm filename.\n")), | |
| -1); | |
| if (buf_len > MAXPATHLEN) | |
| ACELIB_ERROR_RETURN ((LM_DEBUG, | |
| ACE_TEXT ("ACE_MEM_Connector::connect error: shm filename length %d exceeds maximum %d\n"), | |
| buf_len, MAXPATHLEN), | |
| -1); | |
| if (ACE::recv (new_handle, buf, buf_len) != buf_len) | |
| ACELIB_ERROR_RETURN ((LM_DEBUG, | |
| ACE_TEXT ("ACE_MEM_Connector::connect error receiving shm filename: expected %d bytes\n"), | |
| buf_len), | |
| -1); |
Summary by CodeRabbit