Replaced str... with strn... methods #37690
Open
+30
−22
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Review changes with |
kkasperczyk-no
force-pushed
the
snprintf_fixes
branch
4 times, most recently
from
8365b5d to
10698f7
Compare
|
PR #37690: Size comparison from 7dccded to 10698f7 Full report (17 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, qpg, stm32, tizen)
|
kkasperczyk-no
force-pushed
the
snprintf_fixes
branch
from
10698f7 to
56ebe19
Compare
|
PR #37690: Size comparison from 24d61c6 to 56ebe19 Full report (3 builds for cc32xx, stm32)
|
kkasperczyk-no
force-pushed
the
snprintf_fixes
branch
from
56ebe19 to
9143f2f
Compare
| @@ -102,15 +102,16 @@ static void HandleNodeBrowse(void * context, DnssdService * services, size_t ser | |||
| auto & ipAddress = services[i].mAddress; | |||
|
|
|||
| // mType(service name) exactly matches with operational service name | |||
| bool isOperationalBrowse = strcmp(services[i].mType, kOperationalServiceName) == 0; | |||
| bool isOperationalBrowse = strncmp(services[i].mType, kOperationalServiceName, sizeof(kOperationalServiceName)) == 0; | |||
There was a problem hiding this comment.
When you do strncmp(buffer, const, length), length should be the size of the buffer, not the size of the constant. Otherwise, strncmp(buffer, const, length) == 0 becomes "Is const a prefix of buffer?", which may change the behavior.
Overall, it seems adding the following helper in CHIPMemString.h may be less error prone than trying to use the right constant manually:
template <size_t N>
inline bool StringEquals(char (&str1)[N], const char* str2)
{
return strncmp(str1, str2, N) == 0;
}
There was a problem hiding this comment.
becomes "Is const a prefix of buffer?
Ah right, good catch! So isn't it just enough to use kDnssdTypeMaxSize + 1 instead?
There was a problem hiding this comment.
Yes, it should be enough in this case. I only proposed a more generic solution as it's hard for a reviewer to catch these kinds of errors (one has to check what the actual buffer size is).
src/platform/Zephyr/ZephyrConfig.cpp
Outdated
| return WriteConfigValueStr(key, str, str ? strlen(str) : 0); | ||
| /* The max value size is smaller than the sector size by several ATE size, but the API to get the exact size is not available. | ||
| * The backend implementation validates the detailed size, so we don't need to do that here. */ | ||
| return WriteConfigValueStr(key, str, str ? strnlen(str, sSectorSize) : 0); |
There was a problem hiding this comment.
Honestly, this code doesn't really protect from out of bounds reads as the sector size will most of the time be an order of magnitude bigger than the value provided. The API contract is that str must be null-terminated and we have no other choice than to trust it is.
There was a problem hiding this comment.
I see, thanks for the input
There was a problem hiding this comment.
I will put a comment there, as we are going to keep getting this usage as potentially error-prone from the coverity. This way we will remember it's safe to ignore it.
There was a problem hiding this comment.
It seems you could just suppress the warning using coverity annotation: https://doclazy.wordpress.com/2011/07/14/coverity-suppressing-false-positives-with-cod/
kkasperczyk-no
force-pushed
the
snprintf_fixes
branch
from
9143f2f to
e4d6dd4
Compare
Static code analysis shown that there are several places that str.. methods are used, what may lead to the bad memory access. Replaced all of them with corresponding strn... methods.
kkasperczyk-no
force-pushed
the
snprintf_fixes
branch
from
e4d6dd4 to
42953c2
Compare
|
PR #37690: Size comparison from 48adf65 to 42953c2 Full report (84 builds for bl602, bl702, bl702l, cc13x4_26x4, cc32xx, cyw30739, efr32, esp32, linux, nrfconnect, nxp, psoc6, qpg, stm32, telink, tizen)
|
bzbarsky-apple
requested changes
Feb 21, 2025
| @@ -695,7 +695,7 @@ CHIP_ERROR Server::SendUserDirectedCommissioningRequest(Transport::PeerAddress c | |||
| CHIP_ERROR err; | |||
|
|
|||
| // only populate fields left blank by the client | |||
| if (strlen(id.GetInstanceName()) == 0) | |||
| if (strnlen(id.GetInstanceName(), Dnssd::Commission::kInstanceNameMaxLength + 1) == 0) | |||
There was a problem hiding this comment.
We don't care about the actual length, right? We just care whether *id.GetInstanceName() == '\0'? Does it make sense to change to that test?
| @@ -738,7 +738,7 @@ CHIP_ERROR Server::SendUserDirectedCommissioningRequest(Transport::PeerAddress c | |||
| } | |||
| } | |||
|
|
|||
| if (strlen(id.GetDeviceName()) == 0) | |||
| if (strnlen(id.GetDeviceName(), Dnssd::kKeyDeviceNameMaxLength + 1) == 0) | |||
There was a problem hiding this comment.
Again, this does not care about actual length....
| @@ -84,7 +84,7 @@ CHIP_ERROR InterfaceId::GetInterfaceName(char * nameBuf, size_t nameBufSize) con | |||
| } | |||
| CHIP_ERROR InterfaceId::InterfaceNameToId(const char * intfName, InterfaceId & interface) | |||
| { | |||
| if (strlen(intfName) < 3) | |||
| if (strnlen(intfName, kMaxIfNameLength) < 3) | |||
There was a problem hiding this comment.
This change ... does not seem to make anything safer. If there is no null-terminator before kMaxIfNameLength then nothing would change about this test and then the strtoul below will happily walk off the end of the data just like strlen would have. If there is a null-terminator before kMaxIfNameLength, then strlen would not be a problem either.
| @@ -210,7 +210,7 @@ CHIP_ERROR InterfaceId::GetInterfaceName(char * nameBuf, size_t nameBufSize) con | |||
|
|
|||
| CHIP_ERROR InterfaceId::InterfaceNameToId(const char * intfName, InterfaceId & interface) | |||
| { | |||
| if (strlen(intfName) < 3) | |||
| if (strnlen(intfName, InterfaceId::kMaxIfNameLength) < 3) | |||
There was a problem hiding this comment.
Again, this does not seem to actually improve safety, given the strtoul below.
| @@ -445,7 +445,7 @@ CHIP_ERROR InterfaceId::GetInterfaceName(char * nameBuf, size_t nameBufSize) con | |||
| { | |||
| return CHIP_ERROR_POSIX(errno); | |||
| } | |||
| size_t nameLength = strlen(intfName); | |||
| size_t nameLength = strnlen(intfName, IF_NAMESIZE); | |||
There was a problem hiding this comment.
Again, the CopyString will just walk off the end of the data, no? Worse yet, this can break the length check below and turn what would have been an out-of-bounds read into an out-of-bounds write, which is much worse.
I'm stopping here. The basic premise that strnlen is safer than strlen is only true in some cases, and so far it's not true in a bunch of cases in this PR. Making it safer requires being very careful with the data flow.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Static code analysis shown that there are several places that str.. methods are used, what may lead to the bad memory access. Replaced all of them with corresponding strn... methods.
Testing
Tested in CI.