Bug 89869 - -fsanitize=undefined miscompilation
Summary: -fsanitize=undefined miscompilation
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: sanitizer (show other bugs)
Version: 8.3.0
: P1 normal
Target Milestone: 7.5
Assignee: Jakub Jelinek
URL:
Keywords: wrong-code
Depends on:
Blocks:
 
Reported: 2019-03-28 13:37 UTC by Jörg Richter
Modified: 2019-08-30 13:34 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work: 8.3.1, 9.0
Known to fail: 7.4.0, 8.3.0
Last reconfirmed: 2019-03-29 00:00:00


Attachments
gcc9-pr89869.patch (1.19 KB, patch)
2019-03-29 10:35 UTC, Jakub Jelinek
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jörg Richter 2019-03-28 13:37:22 UTC
cat > t.cc <<EOF
struct Object
{
    Object* first_ = 0;
    Object* last_ = 0;
    Object* next_ = 0;
    Object* prev_ = 0;

    virtual ~Object() {}
};

void unlinkChild( Object* parent, Object* child )
{
  ( child->prev_ ? child->prev_->next_ : parent->first_ ) = child->next_;
  ( child->next_ ? child->next_->prev_ : parent->last_ ) = child->prev_;
}

int main( int argc, char** argv)
{
  Object a;
  Object b;
  unlinkChild( &a, &b );
  return 0;
}
EOF

g++ -o t t.cc -Wmaybe-uninitialized -fsanitize=undefined
t


Gives this:
t.cc: In function 'void unlinkChild(Object*, Object*)':
t.cc:13:68: warning: 'child.1' may be used uninitialized in this function [-Wmaybe-uninitialized]
   ( child->prev_ ? child->prev_->next_ : parent->first_ ) = child->next_;
                                                             ~~~~~~~^~~~~
t.cc:14:67: warning: 'child.5' may be used uninitialized in this function [-Wmaybe-uninitialized]
   ( child->next_ ? child->next_->prev_ : parent->last_ ) = child->prev_;
                                                            ~~~~~~~^~~~~
t.cc:13:68: runtime error: member access within address 0x000000400710 which does not point to an object of type 'Object'
0x000000400710: note: object has invalid vptr
 a0 ff ff ff  31 ed 49 89 d1 5e 48 89  e2 48 83 e4 f0 50 54 49  c7 c0 70 15 40 00 48 c7  c1 80 15 40
              ^~~~~~~~~~~~~~~~~~~~~~~
              invalid vptr
t.cc:14:67: runtime error: member access within address 0x7fff38bf5fa0 which does not point to an object of type 'Object'
0x7fff38bf5fa0: note: object has invalid vptr
 00 00 00 00  01 00 00 00 00 00 00 00  89 63 bf 38 ff 7f 00 00  00 00 00 00 00 00 00 00  8b 63 bf 38
              ^~~~~~~~~~~~~~~~~~~~~~~
              invalid vptr

There is no compiler warning or runtime error without -fsanitize=undefined.
This was reduced from a much larger testcase where a pointer value was set to zero by the -fsanitize=undefined code leading to wrong results.
Comment 1 Martin Liška 2019-03-29 08:18:51 UTC
Confirmed, lemme take a look.
Comment 2 Martin Liška 2019-03-29 09:23:17 UTC
Slightly reduced test-case:

$ cat pr89869.cc
struct Object
{
    Object* next = 0;
    virtual ~Object() {}
};

void unlinkChild(Object* child, Object *nul)
{
  ( child->next ? nul: child) = child->next;
}

int main( int argc, char** argv)
{
  Object a;
  unlinkChild(&a, 0);
  return 0;
}

UBSAN generates following original dump:

;; Function void unlinkChild(Object*, Object*) (null)

<<cleanup_point <<< Unknown tree: expr_stmt
  if ((.UBSAN_VPTR (SAVE_EXPR <child>, (long unsigned int) SAVE_EXPR <child>->_vptr.Object, 11320505648503524435, &_ZTI6Object, 3B);, SAVE_EXPR <child>;)->next != 0B)
    {
      (void) (nul = (.UBSAN_VPTR (SAVE_EXPR <child>, (long unsigned int) SAVE_EXPR <child>->_vptr.Object, 11320505648503524435, &_ZTI6Object, 3B);, SAVE_EXPR <child>;)->next);
    }
  else
    {
      (void) (child = (.UBSAN_VPTR (SAVE_EXPR <child>, (long unsigned int) SAVE_EXPR <child>->_vptr.Object, 11320505648503524435, &_ZTI6Object, 3B);, SAVE_EXPR <child>;)->next);
    } >>>>>;

which looks fine to me. However gimplification generates:

unlinkChild (struct Object * child, struct Object * nul)
{
  struct Object * child.0;
  struct Object * child.1;

  child.0 = child;
  _1 = child.0->_vptr.Object;
  _2 = (long unsigned int) _1;
  .UBSAN_VPTR (child.0, _2, 11320505648503524435, &_ZTI6Object, 3B);
  _3 = child.0->next;
  if (_3 != 0B) goto <D.2727>; else goto <D.2728>;
  <D.2727>:
  child.1 = child;
  _4 = child.1->_vptr.Object;
  _5 = (long unsigned int) _4;
  .UBSAN_VPTR (child.1, _5, 11320505648503524435, &_ZTI6Object, 3B);
  nul = child.1->next;
  goto <D.2730>;
  <D.2728>:
  _6 = child.1->_vptr.Object;
  _7 = (long unsigned int) _6;
  .UBSAN_VPTR (child.1, _7, 11320505648503524435, &_ZTI6Object, 3B);
  child = child.1->next;
  <D.2730>:
}

which is wrong because child.1 is used in <D.2728> uninitialized. Richi is it a gimplification bug?
Or is the generic wrongly generated?
Comment 3 Jakub Jelinek 2019-03-29 09:31:08 UTC
If there is a SAVE_EXPR used in both arms of the conditional and not used before that, then it is a bug in the generic code.
Comment 4 Jakub Jelinek 2019-03-29 10:19:39 UTC
The problem is that for the COND_EXPR as lvalue, cp_build_modify_expr does:
      /* Handle (a ? b : c) used as an "lvalue".  */
    case COND_EXPR:
      {
        /* Produce (a ? (b = rhs) : (c = rhs))
           except that the RHS goes through a save-expr
           so the code to compute it is only emitted once.  */

It uses stabilize_expr on the rhs, but this is before ubsan instrumentation, so there is nothing to stabilize.
And then we emit:
        cond = build_conditional_expr
          (input_location, TREE_OPERAND (lhs, 0),
           cp_build_modify_expr (loc, TREE_OPERAND (lhs, 1),
                                 modifycode, rhs, complain),
           cp_build_modify_expr (loc, TREE_OPERAND (lhs, 2),
                                 modifycode, rhs, complain),
           complain);
so rhs is a tree shared in two COND_EXPR branches.
And then we later instrument this and wrap in a SAVE_EXPR for VPTR instrumentation.
Comment 5 Jakub Jelinek 2019-03-29 10:35:40 UTC
Created attachment 46056 [details]
gcc9-pr89869.patch

Untested fix.
Comment 6 Jakub Jelinek 2019-03-29 20:10:51 UTC
Author: jakub
Date: Fri Mar 29 20:10:19 2019
New Revision: 270024

URL: https://gcc.gnu.org/viewcvs?rev=270024&root=gcc&view=rev
Log:
	PR sanitizer/89869
	* typeck.c: Include gimplify.h.
	(cp_build_modify_expr) <case COND_EXPR>: Unshare rhs before using it
	for second time.  Formatting fixes.

	* g++.dg/ubsan/vptr-14.C: New test.

Added:
    trunk/gcc/testsuite/g++.dg/ubsan/vptr-14.C
Modified:
    trunk/gcc/cp/ChangeLog
    trunk/gcc/cp/typeck.c
    trunk/gcc/testsuite/ChangeLog
Comment 7 Jakub Jelinek 2019-03-29 20:11:50 UTC
Fixed for 9.1+ so far.
Comment 8 Jakub Jelinek 2019-04-30 20:57:12 UTC
Author: jakub
Date: Tue Apr 30 20:56:40 2019
New Revision: 270744

URL: https://gcc.gnu.org/viewcvs?rev=270744&root=gcc&view=rev
Log:
	Backported from mainline
	2019-03-29  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/89869
	* typeck.c: Include gimplify.h.
	(cp_build_modify_expr) <case COND_EXPR>: Unshare rhs before using it
	for second time.  Formatting fixes.

	* g++.dg/ubsan/vptr-14.C: New test.

Added:
    branches/gcc-8-branch/gcc/testsuite/g++.dg/ubsan/vptr-14.C
Modified:
    branches/gcc-8-branch/gcc/cp/ChangeLog
    branches/gcc-8-branch/gcc/cp/typeck.c
    branches/gcc-8-branch/gcc/testsuite/ChangeLog
Comment 9 Jakub Jelinek 2019-05-01 07:16:05 UTC
Fixed for 8.4+ too.
Comment 10 Jakub Jelinek 2019-08-30 12:34:51 UTC
Author: jakub
Date: Fri Aug 30 12:34:19 2019
New Revision: 275145

URL: https://gcc.gnu.org/viewcvs?rev=275145&root=gcc&view=rev
Log:
	Backported from mainline
	2019-03-29  Jakub Jelinek  <jakub@redhat.com>

	PR sanitizer/89869
	* typeck.c: Include gimplify.h.
	(cp_build_modify_expr) <case COND_EXPR>: Unshare rhs before using it
	for second time.  Formatting fixes.

	* g++.dg/ubsan/vptr-14.C: New test.

Added:
    branches/gcc-7-branch/gcc/testsuite/g++.dg/ubsan/vptr-14.C
Modified:
    branches/gcc-7-branch/gcc/cp/ChangeLog
    branches/gcc-7-branch/gcc/cp/typeck.c
    branches/gcc-7-branch/gcc/testsuite/ChangeLog
Comment 11 Jakub Jelinek 2019-08-30 13:34:29 UTC
Fixed.