0

I have code which looks like this:

char* newChar = new char[strlen(inputCharArray)+1];
if (NULL == newChar) {
    return;
}
strncpy(newChar, inputCharArray, strlen(inputCharArray));
newChar[strlen(inputCharArray)] = '\0';

This seems for me like totally valid code. But GCC 9.2.1 is complaining about it, with the following warning:

warning: 'char* strncpy(char*, const char*, size_t)' specified bound depends on the length of the source argument [-Wstringop-overflow=]

user438383
  • 5,716
  • 8
  • 28
  • 43
Sir2B
  • 1,029
  • 1
  • 10
  • 17
  • 2
    Off-topic: C++ -> you may safely forget about the null pointer check; `new` throws if allocation fails (unless you explicitly switch off by compiler settings or alike)... – Aconcagua Jul 21 '22 at 16:09
  • Looks valid to me, too, maybe a hint for non-optimal code, though, as in given case `memcpy` would be the better choice... Actually I'd rather go with more C++-like `std::copy` or even better forget about character arrays entirely in favour of `std::string`, which reduces to a one-liner without necessity to care for memory management either (`delete[]`ing the array again!): `std::string newChar(inputCharArray);` (though the name doesn't appear appropriate to me any more...). – Aconcagua Jul 21 '22 at 16:14
  • The intent of `strncpy()` is that the third (length) argument is related to the actual length of the first (output) argument, regardless of the second (source). You've written it so the length argument is overtly related to the length of the source argument. The compiler is presumably not doing deeper analysis based on you having made the two lengths equal (which can be justified for a compiler, since the `new` and `strncpy()` call might be in different functions/source files - and the compiler is not required to detect that you've made the lengths equal). – Peter Jul 21 '22 at 16:32
  • It sees the code copying the string without the zero-terminator. Sensible thing to do is to use strcpy(), you made sure it always fits. – Hans Passant Jul 21 '22 at 20:45
  • @Aconcagua modern c++ and exceptions are not used in the project (embedded c and only little bit c++) – Sir2B Jul 22 '22 at 06:12
  • Well, embedded and dynamic memory allocation (`new`!) are not really a good combination anyway... Won't judge, but still would rather avoid myself. Wondering why you'd call `strlen` three times anyway – hopefully the compiler is smart enough to recognise and optimise away, but I wouldn't rely on that. Both `memcpy` and `strcpy` are better choice anyway, as each of does only one of those two checks `strncpy` needs to do (it cannot know if the string is shorter than given length thus needs to check for null terminator *additionally*). – Aconcagua Jul 22 '22 at 08:30
  • In between these two `memcpy` likely is even a bit more efficient as being able to copy larger chunks at once without having to inspect individual bytes for a null terminator. – Aconcagua Jul 22 '22 at 08:31
  • @Aconcagua Don't worry. The memory allocation is only done during startup. And we have overriden the new operator. This is not part of any cyclic operations, so cpu time is not as important. – Sir2B Jul 22 '22 at 10:45
  • I think strcpy is he way I go. But then the static code analyzer complains, to use strncpy. But that's my problem. – Sir2B Jul 22 '22 at 10:47
  • Static analysers are a (great) helping tool, but not the Holy Grail either, they can err, too. Just make sure you *document* why you did differently and anything should be fine. `strcpy` would include the null-terminator so you wouldn't need to set explicitly, by the way... – Aconcagua Jul 25 '22 at 07:08
  • '*so cpu time is not as important'* – on MCU maybe even more important: memory fragmentation ;) – Aconcagua Jul 25 '22 at 07:11

0 Answers0