ACE_UNIX_Addr supports abstract path in Linux. #870
Conversation
likema
force-pushed
the
linux-unix-addr-abstract-path
branch
from
87ccca6 to
294c373
Compare
likema
force-pushed
the
linux-unix-addr-abstract-path
branch
from
294c373 to
cf8f8f2
Compare
|
Any test? |
It is already in |
|
But does it contain a test for an abstract path? |
I mean that I committed a test for it: ACE_TAO/ACE/tests/UNIX_Addr_Test.cpp Lines 58 to 74 in cf8f8f2 |
|
Ok, missed that addition. Can you merge master into your branch so that all recent CI builds run? |
likema
force-pushed
the
linux-unix-addr-abstract-path
branch
from
cf8f8f2 to
9ae9996
Compare
Done |
WalkthroughThe changes update the handling of UNIX domain socket addresses in ACE for Linux abstract socket paths. New conditional logic verifies whether a path is abstract (prefixed with '@'), adjusts the character and length accordingly, and uses the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant S as string_to_addr
participant L as Linux Check
C->>S: Call string_to_addr(input)
S->>L: Check if ACE_LINUX defined
alt Linux system
L->>S: Verify if sun_path[0] == '@'
alt Abstract path detected
S->>S: Replace '@' with '\0'
S->>S: Calculate length (len)
end
else
S->>S: Use standard path processing
end
S->>S: Call set_size(len)
S-->>C: Return address configuration
sequenceDiagram
participant C as Caller
participant E as operator== (ACE_UNIX_Addr)
participant D as Decision Block
C->>E: Compare two UNIX_Addr objects
E->>D: Check if both sun_paths are empty (Linux)
alt Both empty
D->>E: Set offset to skip first character
E->>E: Perform comparison using adjusted index
else
D->>E: Use full-length comparison
end
E-->>C: Return comparison result
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 review |
✅ Actions performedReview triggered.
|
![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
🔭 Outside diff range comments (2)
ACE/ace/UNIX_Addr.cpp (2)
38-56: Add input validation in string_to_addr.The current implementation lacks validation of the input path length, which could lead to buffer overflow.
Add length validation:
int ACE_UNIX_Addr::string_to_addr (const char addr[]) { + if (!addr) + return -1; + + size_t const input_len = ACE_OS::strlen(addr); + if (input_len >= sizeof(this->unix_addr_.sun_path)) + return -1; + ACE_OS::strsncpy (this->unix_addr_.sun_path, addr, sizeof this->unix_addr_.sun_path); size_t const len = ACE_OS::strlen (this->unix_addr_.sun_path); #if defined (ACE_LINUX) if (*this->unix_addr_.sun_path == '@') // abstract path { *this->unix_addr_.sun_path = 0; } #endif /* ACE_LINUX */
128-147: Improve error handling in set method.The current implementation doesn't validate input parameters and might access invalid memory.
Add parameter validation:
int ACE_UNIX_Addr::set (const sockaddr_un *un, int len) { + if (!un || len <= 0) + return -1; + (void) ACE_OS::memset ((void *) &this->unix_addr_, 0, sizeof this->unix_addr_); this->unix_addr_.sun_family = AF_UNIX; #if defined (ACE_LINUX) int const n = ACE_MIN (len - int (sizeof this->unix_addr_ - sizeof (this->unix_addr_.sun_path)), int (sizeof (this->unix_addr_.sun_path))); if (n > 0) { memcpy (this->unix_addr_.sun_path, un->sun_path, n); } #else ACE_OS::strcpy (this->unix_addr_.sun_path, un->sun_path); #endif /* ACE_LINUX */ this->base_set (AF_UNIX, len); return 0; }
🧹 Nitpick comments (1)
ACE/tests/UNIX_Addr_Test.cpp (1)
58-74: Add more test cases for abstract paths.While the current tests cover basic functionality, consider adding these test cases:
- Negative tests with invalid abstract paths
- Edge cases with maximum path length
- Tests for comparison operator with abstract paths
Add these test cases:
#if defined(ACE_LINUX) + // Test maximum length abstract path + char max_path[108] = "@"; // sizeof(sun_path) is typically 108 + ACE_OS::memset(max_path + 1, 'a', sizeof(max_path) - 2); + max_path[sizeof(max_path) - 1] = '\0'; + addr.set(max_path); + ACE_TEST_ASSERT(addr.addr_to_string(buf, sizeof(buf)) == 0); + ACE_TEST_ASSERT(strcmp(max_path, ACE_TEXT_ALWAYS_CHAR(buf)) == 0); + + // Test comparison of abstract paths + ACE_UNIX_Addr addr2("@/tmp/ace.test"); + ACE_TEST_ASSERT(addr2 == addr2); + ACE_TEST_ASSERT(!(addr2 != addr2)); + + // Test invalid abstract paths + ACE_TEST_ASSERT(addr.set("@") == 0); // Empty abstract path + ACE_TEST_ASSERT(addr.set("@@/tmp/test") == 0); // Multiple @ prefixes #endif // ACE_LINUX
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ACE/ace/UNIX_Addr.cpp(5 hunks)ACE/ace/UNIX_Addr.inl(1 hunks)ACE/tests/UNIX_Addr_Test.cpp(1 hunks)
🔇 Additional comments (1)
ACE/ace/UNIX_Addr.inl (1)
27-40:⚠️ Potential issueVerify comparison length and add bounds checking.
The current implementation has potential issues:
- The length calculation
sizeof (this->unix_addr_.sun_path) - 1might be incorrect for abstract paths where the full length should be used.- No validation of array bounds before accessing
sun_path[0].Consider this safer implementation:
ACE_INLINE bool ACE_UNIX_Addr::operator == (const ACE_UNIX_Addr &sap) const { size_t i = 0; + size_t len = sizeof(this->unix_addr_.sun_path); #if defined (ACE_LINUX) - if (!*this->unix_addr_.sun_path && !*sap.unix_addr_.sun_path) + if (len > 0 && !this->unix_addr_.sun_path[0] && !sap.unix_addr_.sun_path[0]) { i = 1; + // For abstract paths, compare the full length + len = sizeof(this->unix_addr_.sun_path); } #endif /* ACE_LINUX */ return ACE_OS::strncmp (this->unix_addr_.sun_path + i, sap.unix_addr_.sun_path + i, - sizeof (this->unix_addr_.sun_path) - 1) == 0; + len - i) == 0; }
| int | ||
| ACE_UNIX_Addr::addr_to_string (ACE_TCHAR s[], size_t len) const | ||
| { | ||
| ACE_OS::strsncpy (s, | ||
| ACE_TEXT_CHAR_TO_TCHAR (this->unix_addr_.sun_path), | ||
| len); | ||
| size_t i = 0; | ||
| #if defined (ACE_LINUX) | ||
| if (len && !*this->unix_addr_.sun_path && this->unix_addr_.sun_path[1]) | ||
| { | ||
| s[0] = '@'; | ||
| i = 1; | ||
| } | ||
| #endif /* ACE_LINUX */ | ||
| ACE_OS::strsncpy (s + i, | ||
| ACE_TEXT_CHAR_TO_TCHAR (this->unix_addr_.sun_path + i), | ||
| len - i); | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
Add bounds checking in addr_to_string.
The current implementation lacks validation of the output buffer size and index bounds.
Add bounds checking:
int
ACE_UNIX_Addr::addr_to_string (ACE_TCHAR s[], size_t len) const
{
+ if (!s || !len)
+ return -1;
+
size_t i = 0;
#if defined (ACE_LINUX)
if (len && !*this->unix_addr_.sun_path && this->unix_addr_.sun_path[1])
{
s[0] = '@';
i = 1;
+ if (i >= len)
+ return -1;
}
#endif /* ACE_LINUX */
ACE_OS::strsncpy (s + i,
ACE_TEXT_CHAR_TO_TCHAR (this->unix_addr_.sun_path + i),
len - i);
return 0;
}📝 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.
| int | |
| ACE_UNIX_Addr::addr_to_string (ACE_TCHAR s[], size_t len) const | |
| { | |
| ACE_OS::strsncpy (s, | |
| ACE_TEXT_CHAR_TO_TCHAR (this->unix_addr_.sun_path), | |
| len); | |
| size_t i = 0; | |
| #if defined (ACE_LINUX) | |
| if (len && !*this->unix_addr_.sun_path && this->unix_addr_.sun_path[1]) | |
| { | |
| s[0] = '@'; | |
| i = 1; | |
| } | |
| #endif /* ACE_LINUX */ | |
| ACE_OS::strsncpy (s + i, | |
| ACE_TEXT_CHAR_TO_TCHAR (this->unix_addr_.sun_path + i), | |
| len - i); | |
| return 0; | |
| } | |
| int | |
| ACE_UNIX_Addr::addr_to_string (ACE_TCHAR s[], size_t len) const | |
| { | |
| if (!s || !len) | |
| return -1; | |
| size_t i = 0; | |
| #if defined (ACE_LINUX) | |
| if (len && !*this->unix_addr_.sun_path && this->unix_addr_.sun_path[1]) | |
| { | |
| s[0] = '@'; | |
| i = 1; | |
| if (i >= len) | |
| return -1; | |
| } | |
| #endif /* ACE_LINUX */ | |
| ACE_OS::strsncpy (s + i, | |
| ACE_TEXT_CHAR_TO_TCHAR (this->unix_addr_.sun_path + i), | |
| len - i); | |
| return 0; | |
| } |
In Linux, UNIX domain socket supports abstract path like
@/tmp/foobarSummary by CodeRabbit
New Features
Tests