| Message ID | 20170323203705.GX11094@tucnak |
|---|---|
| State | New |
| Headers | show |
On Thu, Mar 23, 2017 at 11:37 PM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > Since late C++ folding has been committed, we don't sanitize some reference > bindings to NULL. Earlier we had always NOP_EXPR to REFERENCE_TYPE say from > INTEGER_CST or whatever else, but cp_fold can now turn that right into > INTEGER_CST with REFERENCE_TYPE. The following patch sanitizes even those. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2017-03-23 Jakub Jelinek <jakub@redhat.com> > > PR c++/79572 > * c-ubsan.h (ubsan_maybe_instrument_reference): Change argument to > tree *. > * c-ubsan.c (ubsan_maybe_instrument_reference): Likewise. Handle > not just NOP_EXPR to REFERENCE_TYPE, but also INTEGER_CST with > REFERENCE_TYPE. > > * cp-gimplify.c (cp_genericize_r): Sanitize INTEGER_CSTs with > REFERENCE_TYPE. Adjust ubsan_maybe_instrument_reference caller > for NOP_EXPR to REFERENCE_TYPE. > > * g++.dg/ubsan/null-8.C: New test. > ... > --- gcc/testsuite/g++.dg/ubsan/null-8.C.jj 2017-03-23 09:42:31.664696676 +0100 > +++ gcc/testsuite/g++.dg/ubsan/null-8.C 2017-03-23 09:43:31.501908802 +0100 > @@ -0,0 +1,19 @@ > +// PR c++/79572 > +// { dg-do run } > +// { dg-options "-fsanitize=null -std=c++14" } > +// { dg-output "reference binding to null pointer of type 'const int'" } > + > +void > +foo (const int &iref) > +{ > + if (&iref) > + __builtin_printf ("iref %d\n", iref); > + else > + __builtin_printf ("iref is NULL\n"); Hi Jakub, Using __builtin_printf causes this test to fail sporadically when cross-testing. Stdout and stderr output can get mixed in cross-testing, so dejagnu might see == g++.dg/ubsan/null-8.C:18:7: runtime error: reference binding to null pointer of type iref is NULL 'const int' == instead of == g++.dg/ubsan/null-8.C:18:7: runtime error: reference binding to null pointer of type 'const int' iref is NULL == Is it essential for this testcase to use __builtin_printf or simple "fprintf (stderr, ...)" would do just fine? > +} > + > +int > +main () > +{ > + foo (*((int*) __null)); > +} Regards,
On Fri, Nov 24, 2017 at 05:16:27PM +0300, Maxim Kuvyrkov wrote: > Using __builtin_printf causes this test to fail sporadically when > cross-testing. Stdout and stderr output can get mixed in > cross-testing, so dejagnu might see > == > g++.dg/ubsan/null-8.C:18:7: runtime error: reference binding to null > pointer of type iref is NULL > 'const int' > == > instead of > == > g++.dg/ubsan/null-8.C:18:7: runtime error: reference binding to null > pointer of type 'const int' > iref is NULL > == > > Is it essential for this testcase to use __builtin_printf or simple > "fprintf (stderr, ...)" would do just fine? That would mean bringing in stdio.h, which is very much undesirable. If you want, just revert the patch, verify the testcase FAILs, and then tweak it to say: __attribute__((noinline, noclone)) void bar (const *x, int y) { asm volatile ("" : : "g" (x), "g" (y) : "memory"); } and change __builtin_printf ("iref %d\n", iref); to bar ("iref %d\n", iref); and __builtin_printf ("iref is NULL\n"); to bar ("iref is NULL\n", 0); If the test still FAILs and is fixed after you reapply the patch, the change is preapproved. Jakub
On Fri, Nov 24, 2017 at 03:26:11PM +0100, Jakub Jelinek wrote: > On Fri, Nov 24, 2017 at 05:16:27PM +0300, Maxim Kuvyrkov wrote: > > Using __builtin_printf causes this test to fail sporadically when > > cross-testing. Stdout and stderr output can get mixed in > > cross-testing, so dejagnu might see > > == > > g++.dg/ubsan/null-8.C:18:7: runtime error: reference binding to null > > pointer of type iref is NULL > > 'const int' > > == > > instead of > > == > > g++.dg/ubsan/null-8.C:18:7: runtime error: reference binding to null > > pointer of type 'const int' > > iref is NULL > > == > > > > Is it essential for this testcase to use __builtin_printf or simple > > "fprintf (stderr, ...)" would do just fine? > > That would mean bringing in stdio.h, which is very much undesirable. > > If you want, just revert the patch, verify the testcase FAILs, > and then tweak it to say: > __attribute__((noinline, noclone)) > void > bar (const *x, int y) > { > asm volatile ("" : : "g" (x), "g" (y) : "memory"); > } > > and change __builtin_printf ("iref %d\n", iref); > to bar ("iref %d\n", iref); > and __builtin_printf ("iref is NULL\n"); > to bar ("iref is NULL\n", 0); > If the test still FAILs and is fixed after you reapply the patch, > the change is preapproved. Verified myself: ./cc1plus.246620 -O0 -quiet -fsanitize=null -std=c++14 null-8.C; g++ -fsanitize=undefined -o null-8{,.s}; ./null-8 ./cc1plus.246621 -O0 -quiet -fsanitize=null -std=c++14 null-8.C; g++ -fsanitize=undefined -o null-8{,.s}; ./null-8 null-8.C:25:7: runtime error: reference binding to null pointer of type 'const int' ./cc1plus.246620 -O2 -quiet -fsanitize=null -std=c++14 null-8.C; g++ -fsanitize=undefined -o null-8{,.s}; ./null-8 ./cc1plus.246621 -O2 -quiet -fsanitize=null -std=c++14 null-8.C; g++ -fsanitize=undefined -o null-8{,.s}; ./null-8 null-8.C:25:7: runtime error: reference binding to null pointer of type 'const int' Committed to trunk: 2017-11-27 Jakub Jelinek <jakub@redhat.com> * g++.dg/ubsan/null-8.C (bar): New function. (foo): Use bar instead of __builtin_printf. --- gcc/testsuite/g++.dg/ubsan/null-8.C.jj 2017-03-31 20:38:44.000000000 +0200 +++ gcc/testsuite/g++.dg/ubsan/null-8.C 2017-11-27 11:27:17.311529667 +0100 @@ -3,13 +3,20 @@ // { dg-options "-fsanitize=null -std=c++14" } // { dg-output "reference binding to null pointer of type 'const int'" } +__attribute__((noinline, noclone)) +void +bar (int x) +{ + asm volatile ("" : : "r" (x) : "memory"); +} + void foo (const int &iref) { if (&iref) - __builtin_printf ("iref %d\n", iref); + bar (iref); else - __builtin_printf ("iref is NULL\n"); + bar (1); } int Jakub
--- gcc/c-family/c-ubsan.h.jj 2017-01-01 12:45:46.000000000 +0100 +++ gcc/c-family/c-ubsan.h 2017-03-23 09:13:16.287888726 +0100 @@ -28,7 +28,7 @@ extern tree ubsan_instrument_return (loc extern tree ubsan_instrument_bounds (location_t, tree, tree *, bool); extern bool ubsan_array_ref_instrumented_p (const_tree); extern void ubsan_maybe_instrument_array_ref (tree *, bool); -extern void ubsan_maybe_instrument_reference (tree); +extern void ubsan_maybe_instrument_reference (tree *); extern void ubsan_maybe_instrument_member_call (tree, bool); /* Declare this here as well as in ubsan.h. */ --- gcc/c-family/c-ubsan.c.jj 2017-01-01 12:45:46.000000000 +0100 +++ gcc/c-family/c-ubsan.c 2017-03-23 09:18:51.775486699 +0100 @@ -458,17 +458,26 @@ ubsan_maybe_instrument_reference_or_call return fold_build2 (COMPOUND_EXPR, TREE_TYPE (op), call, op); } -/* Instrument a NOP_EXPR to REFERENCE_TYPE if needed. */ +/* Instrument a NOP_EXPR to REFERENCE_TYPE or INTEGER_CST with REFERENCE_TYPE + type if needed. */ void -ubsan_maybe_instrument_reference (tree stmt) +ubsan_maybe_instrument_reference (tree *stmt_p) { - tree op = TREE_OPERAND (stmt, 0); + tree stmt = *stmt_p; + tree op = stmt; + if (TREE_CODE (stmt) == NOP_EXPR) + op = TREE_OPERAND (stmt, 0); op = ubsan_maybe_instrument_reference_or_call (EXPR_LOCATION (stmt), op, TREE_TYPE (stmt), UBSAN_REF_BINDING); if (op) - TREE_OPERAND (stmt, 0) = op; + { + if (TREE_CODE (stmt) == NOP_EXPR) + TREE_OPERAND (stmt, 0) = op; + else + *stmt_p = op; + } } /* Instrument a CALL_EXPR to a method if needed. */ --- gcc/cp/cp-gimplify.c.jj 2017-03-03 13:23:58.000000000 +0100 +++ gcc/cp/cp-gimplify.c 2017-03-23 09:21:26.693460888 +0100 @@ -1130,6 +1130,19 @@ cp_genericize_r (tree *stmt_p, int *walk } } + if (TREE_CODE (stmt) == INTEGER_CST + && TREE_CODE (TREE_TYPE (stmt)) == REFERENCE_TYPE + && (flag_sanitize & (SANITIZE_NULL | SANITIZE_ALIGNMENT)) + && !wtd->no_sanitize_p) + { + ubsan_maybe_instrument_reference (stmt_p); + if (*stmt_p != stmt) + { + *walk_subtrees = 0; + return NULL_TREE; + } + } + /* Other than invisiref parms, don't walk the same tree twice. */ if (p_set->contains (stmt)) { @@ -1477,7 +1490,7 @@ cp_genericize_r (tree *stmt_p, int *walk if ((flag_sanitize & (SANITIZE_NULL | SANITIZE_ALIGNMENT)) && TREE_CODE (stmt) == NOP_EXPR && TREE_CODE (TREE_TYPE (stmt)) == REFERENCE_TYPE) - ubsan_maybe_instrument_reference (stmt); + ubsan_maybe_instrument_reference (stmt_p); else if (TREE_CODE (stmt) == CALL_EXPR) { tree fn = CALL_EXPR_FN (stmt); --- gcc/testsuite/g++.dg/ubsan/null-8.C.jj 2017-03-23 09:42:31.664696676 +0100 +++ gcc/testsuite/g++.dg/ubsan/null-8.C 2017-03-23 09:43:31.501908802 +0100 @@ -0,0 +1,19 @@ +// PR c++/79572 +// { dg-do run } +// { dg-options "-fsanitize=null -std=c++14" } +// { dg-output "reference binding to null pointer of type 'const int'" } + +void +foo (const int &iref) +{ + if (&iref) + __builtin_printf ("iref %d\n", iref); + else + __builtin_printf ("iref is NULL\n"); +} + +int +main () +{ + foo (*((int*) __null)); +}