Possible overflow of destination array using std::memcpy, the behavior doesn't trigger any diagnostic by the sanitizer in scenario I, while in scenario II the behavior triggers the sanitizer diagnosis. In the test the overflow is about 40 bytes, by overflow 24 bytes array with 64 bytes src string literals. I've also tried to use alignas(64) to align class bar on 64 bytes i.e. class alignas(64) Bar { ... }; But it didn't trigger the sanitizer diagnosis. #include <iostream> #include <array> #include <cstring> const char txt[] = "123456789-123456789-123456789-123456789-123456789-123456789-123"; class Bar { public: constexpr Bar() : m1{}, m2{}, m3{}, m4{}, dst{} {} std::uint16_t m1; std::uint16_t m2; std::uint16_t m3; std::uint16_t m4; std::int8_t dst[24]; }; void test(Bar& b) // scenario II //void test(Bar& b) // scenario II { std::cout << "staring memcpy.\n"; std::cout << "size of bytes to be copied: " << sizeof(txt) <<"\n"; std::cout << "dst array size: " << sizeof(b.dst) << "\n"; std::cout << "overflow size: " << sizeof(txt) - sizeof(b.dst) << "\n"; std::memcpy(b.dst, txt, sizeof(txt)); } class client { public: void func() { test(b); } private: Bar b{}; }; //g++-8 -o vtest vmain.cpp -std=c++14 -fsanitize=address -fsanitize=undefined -static-libasan -static-libubsan int main() { client c{}; c.func(); std::cout << "size of Bar: " << sizeof(Bar) << "\n"; std::cout << "align of Bar: " << alignof(Bar) << "\n"; return 0; }
correction to scenario II should pass by value as follows //void test(Bar b) // scenario II
Should we disable __builtin_memcpy etc. with -fsanitize=address?
The problem here is that we normally preserve memcpy calls and then __interceptor_memcpy is used from the run-time library. However, in this case the second argument of memcpy is a known constant and we convert it to: MEM <unsigned char[64]> [(char * {ref-all})_7] = MEM <unsigned char[64]> [(char * {ref-all})&txt]; for such an assignment we only check the beginning and the end of the chunk and we miss the overflow.
(In reply to Martin Liška from comment #3) > The problem here is that we normally preserve memcpy calls and then > __interceptor_memcpy is used from the run-time library. However, in this > case the second argument of memcpy is a known constant and we convert it to: > MEM <unsigned char[64]> [(char * {ref-all})_7] = MEM <unsigned char[64]> > [(char * {ref-all})&txt]; > > for such an assignment we only check the beginning and the end of the chunk > and we miss the overflow. It seems Clang disables this optimization and convert memcpy to __asan_memcpy calls if -fsanitize=address used: https://godbolt.org/z/dcfadoMYY
> It seems Clang disables this optimization and convert memcpy to > __asan_memcpy calls if -fsanitize=address used: > > https://godbolt.org/z/dcfadoMYY Yep! Richi, do you know a place where we lower this?
(In reply to Martin Liška from comment #5) > > It seems Clang disables this optimization and convert memcpy to > > __asan_memcpy calls if -fsanitize=address used: > > > > https://godbolt.org/z/dcfadoMYY > > Yep! Richi, do you know a place where we lower this? gimple_fold_builtin_memory_op not sure if we should prevent all of those transforms. But the question is why ASAN doesn't instrument the generated aggregate copy? Maybe because in C/C++ you cannot write an aggregate array copy?
(In reply to Richard Biener from comment #6) > not sure if we should prevent all of those transforms. But the question is > why ASAN doesn't instrument the generated aggregate copy? Maybe because > in C/C++ you cannot write an aggregate array copy? We do instrument those. But only instrument them by checking the first and last byte of the copy, not all bytes in between (because that would be for inline checking too large - we'd need to emit inline a loop over those bytes).
(In reply to Jakub Jelinek from comment #7) > (In reply to Richard Biener from comment #6) > > not sure if we should prevent all of those transforms. But the question is > > why ASAN doesn't instrument the generated aggregate copy? Maybe because > > in C/C++ you cannot write an aggregate array copy? > > We do instrument those. But only instrument them by checking the first and > last byte > of the copy, not all bytes in between (because that would be for inline > checking too large - we'd need to emit inline a loop over those bytes). OK, that's lack of an appropriate API then? But still the first and last byte should be sufficient to detect the problem (but maybe I'm missing something here).
(In reply to Richard Biener from comment #8) > (In reply to Jakub Jelinek from comment #7) > > (In reply to Richard Biener from comment #6) > > > not sure if we should prevent all of those transforms. But the question is > > > why ASAN doesn't instrument the generated aggregate copy? Maybe because > > > in C/C++ you cannot write an aggregate array copy? > > > > We do instrument those. But only instrument them by checking the first and > > last byte > > of the copy, not all bytes in between (because that would be for inline > > checking too large - we'd need to emit inline a loop over those bytes). > > OK, that's lack of an appropriate API then? But still the first and last > byte should be sufficient to detect the problem (but maybe I'm missing > something here). No, because the last byte is out of redzone: =>0x7ffff5300000: f1 f1 f1 f1 00 00 00[f3]f3 f3 f3 f3 00 00 00 00 the 'f3' redzone is covering 5*8 bytes after the data type only.
(In reply to Richard Biener from comment #8) > (In reply to Jakub Jelinek from comment #7) > > (In reply to Richard Biener from comment #6) > > > not sure if we should prevent all of those transforms. But the question is > > > why ASAN doesn't instrument the generated aggregate copy? Maybe because > > > in C/C++ you cannot write an aggregate array copy? > > > > We do instrument those. But only instrument them by checking the first and > > last byte > > of the copy, not all bytes in between (because that would be for inline > > checking too large - we'd need to emit inline a loop over those bytes). > > OK, that's lack of an appropriate API then? But still the first and last > byte should be sufficient to detect the problem (but maybe I'm missing > something here). Maybe it just happens the end to be on the stack of the inner most function so it just happens that it is an variable address still.
> > Maybe it just happens the end to be on the stack of the inner most function > so it just happens that it is an variable address still. No, that's not the case, see my previous comment.
On Wed, 12 Apr 2023, marxin at gcc dot gnu.org wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446 > > --- Comment #9 from Martin Li?ka <marxin at gcc dot gnu.org> --- > (In reply to Richard Biener from comment #8) > > (In reply to Jakub Jelinek from comment #7) > > > (In reply to Richard Biener from comment #6) > > > > not sure if we should prevent all of those transforms. But the question is > > > > why ASAN doesn't instrument the generated aggregate copy? Maybe because > > > > in C/C++ you cannot write an aggregate array copy? > > > > > > We do instrument those. But only instrument them by checking the first and > > > last byte > > > of the copy, not all bytes in between (because that would be for inline > > > checking too large - we'd need to emit inline a loop over those bytes). > > > > OK, that's lack of an appropriate API then? But still the first and last > > byte should be sufficient to detect the problem (but maybe I'm missing > > something here). > > No, because the last byte is out of redzone: > > =>0x7ffff5300000: f1 f1 f1 f1 00 00 00[f3]f3 f3 f3 f3 00 00 00 00 > > the 'f3' redzone is covering 5*8 bytes after the data type only. OK, so it's lack of an API then. The alternative would be to do sth similar to stack-checking - instrument every 5*8 byte, possibly up to some limit. Or, as you probably suggest, avoid folding memcpy with size larger than 5*8 byte inline.
(In reply to rguenther@suse.de from comment #12) > On Wed, 12 Apr 2023, marxin at gcc dot gnu.org wrote: > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446 > > > > --- Comment #9 from Martin Li?ka <marxin at gcc dot gnu.org> --- > > (In reply to Richard Biener from comment #8) > > > (In reply to Jakub Jelinek from comment #7) > > > > (In reply to Richard Biener from comment #6) > > > > > not sure if we should prevent all of those transforms. But the question is > > > > > why ASAN doesn't instrument the generated aggregate copy? Maybe because > > > > > in C/C++ you cannot write an aggregate array copy? > > > > > > > > We do instrument those. But only instrument them by checking the first and > > > > last byte > > > > of the copy, not all bytes in between (because that would be for inline > > > > checking too large - we'd need to emit inline a loop over those bytes). > > > > > > OK, that's lack of an appropriate API then? But still the first and last > > > byte should be sufficient to detect the problem (but maybe I'm missing > > > something here). > > > > No, because the last byte is out of redzone: > > > > =>0x7ffff5300000: f1 f1 f1 f1 00 00 00[f3]f3 f3 f3 f3 00 00 00 00 > > > > the 'f3' redzone is covering 5*8 bytes after the data type only. > > OK, so it's lack of an API then. The alternative would be to > do sth similar to stack-checking - instrument every 5*8 byte, > possibly up to some limit. Or, as you probably suggest, avoid > folding memcpy with size larger than 5*8 byte inline. I guess doing so will need to change the cpymemM expand of all targets.