SUMMARY Valgrind reports: Source and destination overlap in strncat(0x4c33045, 0x4c33040, 5) which is, given the argument values, an impossible situation: strncat() will not take more than 5 bytes from the location pointed to by second argument, 0x4c33040, that is, the range 0x4c33040-0x4c33044 (if not fewer, should a '\0' be encountered within), but the destination address is out of that range (yet indeed right adjacent to the source area). But there's no error here. STEPS TO REPRODUCE #include <stdio.h> #include <stdlib.h> #include <string.h> int main(int argc, char* argv[]) { size_t len = strlen(argv[1]); char* buf = (char*) malloc(2 * len + 1); memcpy(buf, argv[1], len + 1); strncat(buf + len, buf, len); printf("%s\n", buf); free(buf); return 0; } OBSERVED RESULT $ gcc -Wall -o vbug vbug.c $ ./vbug hello hellohello $ valgrind ./vbug hello ==10915== Memcheck, a memory error detector ==10915== Copyright (C) 2002-2015, and GNU GPL'd, by Julian Seward et al. ==10915== Using Valgrind-3.11.0 and LibVEX; rerun with -h for copyright info ==10915== Command: ./vbug hello ==10915== ==10915== Source and destination overlap in strncat(0x4c33045, 0x4c33040, 5) ==10915== at 0x4A08A76: strncat (vg_replace_strmem.c:344) ==10915== by 0x4006D0: main (in /home/lavr/vbug) ==10915== hellohello ==10915== ==10915== HEAP SUMMARY: ==10915== in use at exit: 0 bytes in 0 blocks ==10915== total heap usage: 1 allocs, 1 frees, 11 bytes allocated ==10915== ==10915== All heap blocks were freed -- no leaks are possible ==10915== ==10915== For counts of detected and suppressed errors, rerun with: -v ==10915== ERROR SUMMARY: 1 errors from 1 contexts (suppressed: 6 from 5) EXPECTED RESULT No errors SOFTWARE/OS VERSIONS Linux: uname -a Linux hostname 2.6.32-754.6.3.el6.x86_64 #1 SMP Tue Oct 9 17:27:49 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
Indeed a problem.
I do think this is somewhat questionable. You could see it as if strncat copies over the trailing zero terminator from the source to the destination. In which case there is overlap. If you insist that zero terminator isn't being copied, but a new zero terminator is added to the destination after n chars of the original have been copied, then the following should fix it: diff --git a/shared/vg_replace_strmem.c b/shared/vg_replace_strmem.c index 79e640189..8dd5b6368 100644 --- a/shared/vg_replace_strmem.c +++ b/shared/vg_replace_strmem.c @@ -370,7 +370,7 @@ static inline void my_exit ( int x ) if (is_overlap(dst_orig, \ src_orig, \ (Addr)dst-(Addr)dst_orig+1, \ - (Addr)src-(Addr)src_orig+1)) \ + n)) \ RECORD_OVERLAP_ERROR("strncat", dst_orig, src_orig, n); \ \ return dst_orig; \ Note that we also intercept strlcat. But there you give the size of the dst buffer, which should include space for the terminator. So there this kind of concat would always be considered an overlap.
My point was that strncat() will never even encounter the terminating '\0' byte in the source string because its counter (5) will be depleted to 0 before then (or, in a generic case, the '\0' would have been encountered in the source string earlier -- causing strncat() to return). Which means that technically the pointers used in such an operation will have never crossed the paths of each other (in this case, the source pointer will never be used to extract anything that goes into the area where the destination pointer was operating). If strncat() was used in the opposite direction (vs. what's my test case's doing), that is, trying to double the string by prepending the original string, then the warning would be warranted, because the first position of the source string could have been indeed overwritten: strncat(buf, buf + len, len); And obviously the result would have been wrong as well -- no string doubled on the program output. I still consider that warning as excessive in the reported case.
(In reply to AL from comment #3) > My point was that strncat() will never even encounter the terminating '\0' > byte in the source string because its counter (5) will be depleted to 0 > before then (or, in a generic case, the '\0' would have been encountered in > the source string earlier -- causing strncat() to return). Which means that > technically the pointers used in such an operation will have never crossed > the paths of each other (in this case, the source pointer will never be used > to extract anything that goes into the area where the destination pointer > was operating). Technically you are right. But I would worry about the string terminator overlap. An implementation might copy 8 bytes at a time if len == 7 (or any n * 8 - 1) . Which would have overlap. It probably would still be fine. There would probably be an explicit zero terminating byte that fixed up any overlap confusion. But it does feel you are just on the edge of what is "allowed". > I still consider that warning as excessive in the reported case. You might well be right. And I wouldn't complain if the overlap check was adjusted to just account for the maximum n chars in src. But the code as given does make warning flags go off in other tools as well. e.g. gcc 13 (didn't test with other versions) will also insist there is a possible 1 byte overlap: $ gcc -O2 -Wall -g -o vbug vbug.c vbug.c: In function ‘main’: vbug.c:10:5: warning: ‘strncat’ accessing between 1 and 9223372036854775805 bytes at offsets [0, 9223372036854775805] and 0 may overlap 1 byte at offset [0, 9223372036854775807] [-Wrestrict] 10 | strncat(buf + len, buf, len); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
I think that this is incorrect. The C standard says "The initial character of s2 overwrites the null character at the end of s1." In this case we're complaining about just that. I can't see any
I can't see anything in the standard that defines overlap. The difference between str(n)cat and functions like memcpy/memmove is that one character gets lost in the process - one of the terminating nulls.
I don't get any errors with either clang or gcc asan on FreeBSD or Linux.
> $ gcc -O2 -Wall -g -o vbug vbug.c > vbug.c: In function ‘main’: > vbug.c:10:5: warning: ‘strncat’ accessing between 1 and 9223372036854775805 bytes at offsets [0, 9223372036854775805] and 0 may overlap 1 byte at offset [0, 9223372036854775807] [-Wrestrict] Starting with some later versions, GCC have gone to some great extreme with both their warnings and the usefulness of such, which is borderline absurdish, at times -- just try to take a look at the bounds above... > An implementation might copy 8 bytes at a time if len == 7 Okay, let's say this is the case, then the code still works correctly, overwriting the ending '\0' byte in dst (which also happens to be shared with src) -- but that's what was going to happen, anyways.
> src_orig, \ > (Addr)dst-(Addr)dst_orig+1, \ > - (Addr)src-(Addr)src_orig+1)) \ > + n)) \ > RECORD_OVERLAP_ERROR("strncat", dst_orig, src_orig, n); \ We need to keep the calculated lengths - either or both sting can be shorter than n.
commit 8eb1df5752f2acba13f78d33b93ec5d1e954b4ff (HEAD -> master, origin/master, origin/HEAD) Author: Paul Floyd <pjfloyd@wanadoo.fr> Date: Thu Nov 9 22:19:43 2023 +0100 Bug 401284 - False positive "Source and destination overlap in strncat" Need to also look at strlcat and plain strcat.
(In reply to Paul Floyd from comment #9) > > src_orig, \ > > (Addr)dst-(Addr)dst_orig+1, \ > > - (Addr)src-(Addr)src_orig+1)) \ > > + n)) \ > > RECORD_OVERLAP_ERROR("strncat", dst_orig, src_orig, n); \ > > We need to keep the calculated lengths - either or both sting can be shorter > than n. ah, yes. Thanks for double checking and pushing this fix through. I think what you pushed is correct. But I think in general "overlap" in these string functions is not well defined because it isn't always clear of the NUL terminator is part of the string/operation or not (like in this case). And for code that does what this example code does the mem/byte functions are more clear. BTW. The gcc 13 does produce a pretty clear warning for this code if we help it see the exact length: #include <stdio.h> #include <stdlib.h> #include <string.h> int main(int argc, char* argv[]) { size_t len = strlen(argv[1]); if (len == 7) { char* buf = (char*) malloc(2 * len + 1); memcpy(buf, argv[1], len + 1); strncat(buf + len, buf, len); printf("%s\n", buf); free(buf); } return 0; } $ gcc -g -O2 -Wall -o vbug vbug.c vbug.c: In function ‘main’: vbug.c:11:5: warning: ‘strncat’ output truncated before terminating nul copying 7 bytes from a string of the same length [-Wstringop-truncation] 11 | strncat(buf + len, buf, len); | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
This is one of those things where the more I think about it the more I get confused, at least to start with. I think I can summarize this as dst can point to the nul terminator of src as long as n is less than or equal to the length of src.
>BTW. The gcc 13 does produce a pretty clear warning for this code if we help it see the exact length >vbug.c:11:5: warning: ‘strncat’ output truncated before terminating nul copying 7 bytes from a string of the same length [-Wstringop-truncation] > 11 | strncat(buf + len, buf, len); TBH, I don't understand what GCC is blabbering about here. Is there somehow a problem with what strncat() can handle or how can it be used? Like I said, The usefulness or adequacy of some of the GCC warnings exceed the acceptable noise levels, lately. They went haywire with those things, without much thinking of how disruptive and distracting they can get...