[Bug sanitizer/109446] New: Possible destination array overflow without diagnosis in memcpy

[Bug sanitizer/109446] New: Possible destination array overflow without diagnosis in memcpy

[mohamed.selim at dxc dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446

            Bug ID: 109446
           Summary: Possible destination array overflow without diagnosis
                    in memcpy
           Product: gcc
           Version: 8.4.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: mohamed.selim at dxc dot com
                CC: dodji at gcc dot gnu.org, dvyukov at gcc dot gnu.org,
                    jakub at gcc dot gnu.org, kcc at gcc dot gnu.org, marxin at gcc dot gnu.org
  Target Milestone: ---

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;
}

[Bug sanitizer/109446] Possible destination array overflow without diagnosis in memcpy

[mohamed.selim at dxc dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446

--- Comment #1 from Mohamed <mohamed.selim at dxc dot com> ---
correction to scenario II should pass by value as follows
//void test(Bar b) // scenario II

[Bug sanitizer/109446] Possible destination array overflow without diagnosis in memcpy

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446

Xi Ruoyao <xry111 at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |xry111 at gcc dot gnu.org
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-04-11
             Status|UNCONFIRMED                 |NEW

--- Comment #2 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
Should we disable __builtin_memcpy etc. with -fsanitize=address?

[Bug sanitizer/109446] Possible destination array overflow without diagnosis in memcpy

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446

--- Comment #3 from Martin Liška <marxin at gcc dot gnu.org> ---
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.

[Bug sanitizer/109446] Possible destination array overflow without diagnosis in memcpy

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446

--- Comment #4 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(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

[Bug sanitizer/109446] Possible destination array overflow without diagnosis in memcpy

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446

Martin Liška <marxin at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |rguenth at gcc dot gnu.org

--- Comment #5 from Martin Liška <marxin at gcc dot gnu.org> ---
> 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?

[Bug sanitizer/109446] Possible destination array overflow without diagnosis in memcpy

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(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?

[Bug sanitizer/109446] Possible destination array overflow without diagnosis in memcpy

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(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).

[Bug sanitizer/109446] Possible destination array overflow without diagnosis in memcpy

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
(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).

[Bug sanitizer/109446] Possible destination array overflow without diagnosis in memcpy

[marxin at gcc dot gnu.org]

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.

[Bug sanitizer/109446] Possible destination array overflow without diagnosis in memcpy

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446

--- Comment #10 from Andrew Pinski <pinskia 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).

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.

[Bug sanitizer/109446] Possible destination array overflow without diagnosis in memcpy

[marxin at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446

--- Comment #11 from Martin Liška <marxin at gcc dot gnu.org> ---
> 
> 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.

[Bug sanitizer/109446] Possible destination array overflow without diagnosis in memcpy

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446

--- Comment #12 from rguenther at suse dot de <rguenther at suse dot de> ---
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.

[Bug sanitizer/109446] Possible destination array overflow without diagnosis in memcpy

[xry111 at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109446

--- Comment #13 from Xi Ruoyao <xry111 at gcc dot gnu.org> ---
(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.