llvm dev - Jan 2012 - [LLVMdev] load widening conflicts with AddressSanitizer

Hi Kostya,
> [resurrecting an old mail thread about AddressSanitizer false positive
caused by
> load widening]
>
> Once the Attribute::AddressSafety is set by clang (a separate patch),
fixing
> this bug may look as simple as this:
I don't get the point of an attribute.  There's plenty of code out there
that does wide loads like this directly (without them being created by the
optimizers) since, just like the optimizers, they know it is safe and a win.
The attribute won't help them.  It looks like a way of just hiding the real
problem, which seems to be that address sanitizer is overly strict.

Ciao, Duncan.
>
> --- lib/Analysis/MemoryDependenceAnalysis.cpp   (revision 148708)
> +++ lib/Analysis/MemoryDependenceAnalysis.cpp   (working copy)
> @@ -323,6 +323,14 @@
>           !TD.fitsInLegalInteger(NewLoadByteSize*8))
>         return 0;
> +    if
(LI->getParent()->getParent()->hasFnAttr(Attribute::AddressSafety)
&&
> +        LIOffs+NewLoadByteSize > MemLocEnd) {
> +      // We will be reading past the location accessed by the original
program.
> +      // While this is safe in a regular build, Address Safety analysys
tools
> +      // may start reporting false warnings. So, do't do widening.
> +      return 0;
> +    }
> +
>       // If a load of this width would include all of MemLoc, then we
succeed.
>       if (LIOffs+NewLoadByteSize >= MemLocEnd)
>         return NewLoadByteSize;
>
>
> Other suggestions, objections?
>
> Thanks,
> --kcc
>
>
>
>
>
> On Wed, Dec 28, 2011 at 1:12 PM, Criswell, John T <criswell at
illinois.edu
> <mailto:criswell at illinois.edu>> wrote:
>
>
>     *From:* Kostya Serebryany [kcc at google.com <mailto:kcc at
google.com>]
>     *Sent:* Wednesday, December 28, 2011 2:46 PM
>     *To:* Criswell, John T
>
>     *Cc:* llvmdev at cs.uiuc.edu <mailto:llvmdev at cs.uiuc.edu>
>     *Subject:* Re: [LLVMdev] load widening conflicts with AddressSanitizer
>
>
>
>     On Wed, Dec 28, 2011 at 12:40 PM, Criswell, John T <criswell at
illinois.edu
>     <mailto:criswell at illinois.edu>> wrote:
>
>         Dear All,
>
>         I think adding metadata and expecting transforms to repect it is a
bad
>         idea.  It is just too easy for someone who does not know about the
>         metadata to add a transform that ignores it.
>
>         As for SAFECode, I think we have one of several options for
handling
>         load-widening.  The most obvious one is to have a pass that just
boosts
>         the allocation size of any alloca with an align 16 attribute;
>
>
>     This may lead to real bugs being lost (false negatives).
>
>     I believe it would remove memory unsafe behavior from the program (so
the
>     program is still functionally incorrect, but it would not violate
memory
>     safety, making memory safety attacks impossible).
>
>
>
>         this pass would only be scheduled to execute when the other
SAFECode
>         instrumentation passes are scheduled to execute.  Another option
would
>         be to just disable the load-widening transform or to use a
specialized
>         version that only widens when the allocation size does not cause a
problem.
>
>
>     To disable load widening you need to pass some flag to the load
widening phase.
>     Passing it through metadata is one of the possible solutions.
>
>     Or we can disable load widening from the clang driver, but then we will
need
>     a flag for that (do we have it now?)
>
>     If you have modified the clang driver with a -fasn option, then you
could
>     also modify the driver so that it does not run the load-widening pass
when
>     the -fasn option is given on the command-line.
>
>     This assumes, of course, that the load-widening transform is not part
of
>     some other transform (like instcombine).  If it is part of another
>     transform, it would be split out into its own transform (separation of
>     concerns and all that).
>
>     -- John T.
>
>
>     --kcc
>
>
>         -- John T.
>
>         *From:* llvmdev-bounces at cs.uiuc.edu <mailto:llvmdev-bounces
at cs.uiuc.edu>
>         [llvmdev-bounces at cs.uiuc.edu <mailto:llvmdev-bounces at
cs.uiuc.edu>] on
>         behalf of Kostya Serebryany [kcc at google.com <mailto:kcc at
google.com>]
>         *Sent:* Tuesday, December 27, 2011 12:57 PM
>         *To:* Chris Lattner
>         *Cc:* llvmdev at cs.uiuc.edu <mailto:llvmdev at cs.uiuc.edu>
>         *Subject:* Re: [LLVMdev] load widening conflicts with
AddressSanitizer
>
>
>
>         On Mon, Dec 19, 2011 at 4:27 PM, Chris Lattner <clattner at
apple.com
>         <mailto:clattner at apple.com>> wrote:
>
>
>             On Dec 17, 2011, at 7:40 AM, Rafael Ávila de Espíndola wrote:
>
>              > On 16/12/11 08:46 PM, Chris Lattner wrote:
>              >> I'm not opposed to disabling this transformation
when asan is
>             on, we just need a clean way to express this in the IR.
>              >
>              > Could clang be aware of asan being on and introduce a
"please don't
>              > widen" metadata on local variable accesses?
>
>             Yes, "we just need a clean way to express this in the
IR."
>
>             LLVM can't have a global "bool ASANIsOn;" that
the optimizers listen to.
>
>
>
>         A global is bad.
>         What about a metadata attached to a Function saying that
transformations
>         which will read out of bounds (even "safely") are
illegal?
>         asan and SAFEcode will add this metadata, optimizers will listen to
it.
>
>         Any other suggestion?
>
>
>         --kcc
>
>
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

On Tue, Jan 24, 2012 at 1:23 AM, Duncan Sands <baldrick at free.fr> wrote:
> Hi Kostya,
>
> > [resurrecting an old mail thread about AddressSanitizer false positive
> caused by
> > load widening]
> >
> > Once the Attribute::AddressSafety is set by clang (a separate patch),
> fixing
> > this bug may look as simple as this:
>
> Hi Duncan,
> I don't get the point of an attribute.  There's plenty of code out
there
> that does wide loads like this directly (without them being created by the
> optimizers) since,

You mean, the source code that e.g. loads 8 bytes, where up to 7 bytes
might be out of array bounds?
Yes, we've seen quite a bit of such code.

First, such code often appears in libc, mostly in hand-written assembly
(e.g. strlen), and valgrind/memcheck has a lot of trouble dealing with it
(it basically has to intercept all such functions, which does not work when
such functions are inlined, so valgrind does not properly work with
O2-compiled binaries).
asan does not care about it (yet) because it does not instrument libc.

Second, we also seen such hacks in regular C/C++ code (usually, in codecs
or compression code).
Strictly speaking -- all these cases are bugs, according to either C or C++
standard, and asan does not impose more restrictions than the standard.
Note, that the hacks like these hurt not only address safety checkers like
asan/memcheck/drmemory/SAFEcode/etc, but also race detectors like
tsan/helgrind/drd/etc.

We still have lots of code with these intentional OOB accesses and we want
to test it.
In most cases I've met so far, the developers decided to actually fix the
bugs according to the C++ standard and require the memory allocation to
have up to 7 extra bytes.
I do expect that sometimes this is impossible or undesirable.
Then the solution would be to use __attribute__((address_safety)) to avoid
instrumenting the tricky pieces of code.

--kcc


just like the optimizers, they know it is safe and a
win.
> The attribute won't help them.  It looks like a way of just hiding the
real
> problem, which seems to be that address sanitizer is overly strict.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120124/40f6fbc5/attachment.html>

Hi Kostya,
> On Tue, Jan 24, 2012 at 1:23 AM, Duncan Sands <baldrick at free.fr
> <mailto:baldrick at free.fr>> wrote:
>
>     Hi Kostya,
>
>      > [resurrecting an old mail thread about AddressSanitizer false
positive
>     caused by
>      > load widening]
>      >
>      > Once the Attribute::AddressSafety is set by clang (a separate
patch), fixing
>      > this bug may look as simple as this:
>
> Hi Duncan,
>
>     I don't get the point of an attribute.  There's plenty of code
out there
>     that does wide loads like this directly (without them being created by
the
>     optimizers) since,
>
>
> You mean, the source code that e.g. loads 8 bytes, where up to 7 bytes
might be
> out of array bounds?
yes.
> Yes, we've seen quite a bit of such code.
I'm not surprised.
> First, such code often appears in libc, mostly in hand-written assembly
(e.g.
> strlen), and valgrind/memcheck has a lot of trouble dealing with it
> (it basically has to intercept all such functions, which does not work when
such
> functions are inlined, so valgrind does not properly work with O2-compiled
> binaries).
> asan does not care about it (yet) because it does not instrument libc.
>
> Second, we also seen such hacks in regular C/C++ code (usually, in codecs
or
> compression code).
> Strictly speaking -- all these cases are bugs, according to either C or C++
> standard, and asan does not impose more restrictions than the standard.
> Note, that the hacks like these hurt not only address safety checkers like
> asan/memcheck/drmemory/SAFEcode/etc, but also race detectors like
> tsan/helgrind/drd/etc.
>
> We still have lots of code with these intentional OOB accesses and we want
to
> test it.
> In most cases I've met so far, the developers decided to actually fix
the bugs
> according to the C++ standard and require the memory allocation to have up
to 7
> extra bytes.
As far as I can see the C and C++ standards are not relevant.  ASAN works on
LLVM IR, not on C or C++.  Lots of different languages have LLVM frontends.  I
personally turn Ada and Fortran into LLVM IR all the time for example.  Clearly
the C standard is not relevant to LLVM IR coming from such languages.  What
matters is how LLVM IR is defined.  As far as I know this construct is perfectly
valid in LLVM IR.
> I do expect that sometimes this is impossible or undesirable.
> Then the solution would be to use __attribute__((address_safety)) to avoid
> instrumenting the tricky pieces of code.
Unfortunately there is in general no way of attaching such attributes in many
languages.

Ciao, Duncan.
>
> --kcc
>
>
>     just like the optimizers, they know it is safe and a win.
>     The attribute won't help them.  It looks like a way of just hiding
the real
>     problem, which seems to be that address sanitizer is overly strict.
>

On Tue, Jan 24, 2012 at 10:23:06AM +0100, Duncan Sands
wrote:
> Hi Kostya,
> 
> > [resurrecting an old mail thread about AddressSanitizer false positive
caused by
> > load widening]
> >
> > Once the Attribute::AddressSafety is set by clang (a separate patch),
fixing
> > this bug may look as simple as this:
> 
> I don't get the point of an attribute.  There's plenty of code out
there
> that does wide loads like this directly (without them being created by the
> optimizers) since, just like the optimizers, they know it is safe and a
win.
> The attribute won't help them.  It looks like a way of just hiding the
real
> problem, which seems to be that address sanitizer is overly strict.
The approach taken by valgrind is to provide a preprocessor macro, so a
validation build can disable such optional performance hacks.

Joerg

On Tue, Jan 24, 2012 at 10:04 AM, Joerg Sonnenberger <
joerg at britannica.bec.de> wrote:
> On Tue, Jan 24, 2012 at 10:23:06AM +0100, Duncan Sands wrote:
> > Hi Kostya,
> >
> > > [resurrecting an old mail thread about AddressSanitizer false
positive
> caused by
> > > load widening]
> > >
> > > Once the Attribute::AddressSafety is set by clang (a separate
patch),
> fixing
> > > this bug may look as simple as this:
> >
> > I don't get the point of an attribute.  There's plenty of code
out there
> > that does wide loads like this directly (without them being created by
> the
> > optimizers) since, just like the optimizers, they know it is safe and
a
> win.
> > The attribute won't help them.  It looks like a way of just hiding
the
> real
> > problem, which seems to be that address sanitizer is overly strict.
>
> The approach taken by valgrind is to provide a preprocessor macro, so a
> validation build can disable such optional performance hacks.
>
True (to some extent it is equivalent to proposed __attribute__).
The difference is that valgrind's RUNNING_ON_VALGRIND (
http://valgrind.org/docs/manual/manual-core-adv.html#manual-core-adv.clientreq
)
is a dynamically executed code, while with asan we need something static.

--kcc


>
> Joerg
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20120124/d883dbee/attachment.html>