[Bug sanitizer/59600] New: no_sanitize_address mishandled when function is inlined

[Bug sanitizer/59600] New: no_sanitize_address mishandled when function is inlined

[eggert at gnu dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59600

            Bug ID: 59600
           Summary: no_sanitize_address mishandled when function is
                    inlined
           Product: gcc
           Version: 4.8.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: sanitizer
          Assignee: unassigned at gcc dot gnu.org
          Reporter: eggert at gnu dot org
                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

Created attachment 31514
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31514&action=edit
Compile and link with '-fsanitize=address -O2' and run to illustrate the bug

A function declared with __attribute__ ((no_sanitize_address)) will attempt to
sanitize its addresses if the function happens to be inlined in another
function that lacks the attribute.  This will cause the code to dump core even
when it's not supposed to.  I discovered this problem when trying to use
address sanitization in Emacs.  To reproduce the problem on an x86-64 platform,
compile the attached program with 'gcc -fsanitize=address -O2'.  It will dump
core because the commented address is sanitized even though it's in a
no_sanitize_address function.  Compiling with -DTHIS_WORKS_AROUND_THE_BUG works
around the bug by adding a noinline attribute to the function in question, but
user code shouldn't have to do that.

[Bug sanitizer/59600] no_sanitize_address mishandled when function is inlined

[y.gribov at samsung dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59600

Yury Gribov <y.gribov at samsung dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |y.gribov at samsung dot com

--- Comment #1 from Yury Gribov <y.gribov at samsung dot com> ---
Created attachment 31515
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31515&action=edit
Draft patch

Fails for me as well.

Given that Asan runs long after inliner this behavior is expected. Perhaps it
makes sense to prohibit inline for unsanitized functions? We'll loose some
performance but no_sanitize_address semantics would be more transparent for
users.

Here's a crude patch which seems to fix repro and also show no regressions for
`make check-c RUNTESTFLAGS=asan.exp'.

[Bug sanitizer/59600] no_sanitize_address mishandled when function is inlined

[kcc at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59600

--- Comment #2 from Kostya Serebryany <kcc at gcc dot gnu.org> ---
We had this problem in clang before 
http://llvm.org/viewvc/llvm-project?view=revision&revision=187967

[Bug sanitizer/59600] no_sanitize_address mishandled when function is inlined

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59600

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Yury Gribov from comment #1)
> Created attachment 31515 [details]
> Draft patch


Why can't you just set DECL_UNINLINABLE on the function decl in
handle_no_sanitize_undefined_attribute in c-common.c just like how
handle_noinline_attribute handles it?

[Bug sanitizer/59600] no_sanitize_address mishandled when function is inlined

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59600

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #3)
> (In reply to Yury Gribov from comment #1)
> > Created attachment 31515 [details]
> > Draft patch
> 
> 
> Why can't you just set DECL_UNINLINABLE on the function decl in
> handle_no_sanitize_undefined_attribute in c-common.c just like how
> handle_noinline_attribute handles it?

Or better yet handle the case where the attributes diff inside
function_attribute_inlinable_p so you don't lose that much optimizations only
when the attributes differ.

[Bug sanitizer/59600] no_sanitize_address mishandled when function is inlined

[y.gribov at samsung dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59600

Yury Gribov <y.gribov at samsung dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #31515|0                           |1
        is obsolete|                            |

--- Comment #5 from Yury Gribov <y.gribov at samsung dot com> ---
Created attachment 31516
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31516&action=edit
New patch based on Andrew's review

(In reply to Andrew Pinski from comment #3)
> Or better yet handle the case where the attributes diff
> inside function_attribute_inlinable_p
> so you don't lose that much optimizations
> only when the attributes differ.

Yup and this would also work for non-C-family frontends.

How about this patch then?

[Bug sanitizer/59600] no_sanitize_address mishandled when function is inlined

[pinskia at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59600

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Comment on attachment 31516
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31516
New patch based on Andrew's review

No this wrong in two ways. Move the check to before the check of the target
attribute table. And second you should compare the current function attribute
to fndecl attribute.

[Bug sanitizer/59600] no_sanitize_address mishandled when function is inlined

[y.gribov at samsung dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59600

--- Comment #8 from Yury Gribov <y.gribov at samsung dot com> ---
Created attachment 31522
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=31522&action=edit
Patch which inlines based on caller/callee attribute match

(In reply to Yury Gribov from comment #7)
> > And second you should compare the current function
> > attribute to fndecl attribute.
> 
> You mean compare caller's and callee's attributes? Makes a lot of sense but
> I don't think this info is available at the time
> function_attribute_inlinable_p is called:
> 
> ...
> 
> Perhaps this check should be moved to e.g. expand_call_inline?

I'm giving this a try with this new patch. Does it look reasonable for you?

[Bug sanitizer/59600] no_sanitize_address mishandled when function is inlined

[ygribov at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59600

--- Comment #9 from ygribov at gcc dot gnu.org ---
Author: ygribov
Date: Wed Feb  5 05:22:29 2014
New Revision: 207497

URL: http://gcc.gnu.org/viewcvs?rev=207497&root=gcc&view=rev
Log:
    PR sanitizer/59600

gcc/
    * cif-code.def (ATTRIBUTE_MISMATCH): New CIF code.
    * ipa-inline.c (report_inline_failed_reason): Handle mismatched
    sanitization attributes.
    (can_inline_edge_p): Likewise.
    (sanitize_attrs_match_for_inline_p): New function.

gcc/testsuite/
    * gcc.dg/asan/nosanitize-and-inline.c: : New test.

Added:
    trunk/gcc/testsuite/gcc.dg/asan/nosanitize-and-inline.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/cif-code.def
    trunk/gcc/ipa-inline.c
    trunk/gcc/testsuite/ChangeLog

[Bug sanitizer/59600] no_sanitize_address mishandled when function is inlined

[y.gribov at samsung dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59600

--- Comment #10 from Yury Gribov <y.gribov at samsung dot com> ---
Paul, does this work for you?

[Bug sanitizer/59600] no_sanitize_address mishandled when function is inlined

[eggert at gnu dot org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59600

--- Comment #11 from eggert at gnu dot org ---
It's not easy for me to test the fix, sorry, I don't normally build GCC.  If it
works on the test case my guess is that it'll work for Emacs, though.