Skip to content

Commit

Permalink
Merging r276003:
Browse files Browse the repository at this point in the history
------------------------------------------------------------------------
r276003 | ericwf | 2016-07-19 11:56:20 -0600 (Tue, 19 Jul 2016) | 35 lines

Fix undefined behavior in __tree

Summary:
This patch attempts to fix the undefined behavior in __tree by changing the node pointer types used throughout. The pointer types are changed for raw pointers in the current ABI and for fancy pointers in ABI V2 (since the fancy pointer types may not be ABI compatible).

The UB in `__tree` arises because tree downcasts the embedded end node and then deferences that pointer. Currently there are 3 node types in __tree.

* `__tree_end_node` which contains the `__left_` pointer. This node is embedded within the container.
* `__tree_node_base` which contains `__right_`, `__parent_` and `__is_black`. This node is used throughout the tree rebalancing algorithms.
* `__tree_node` which contains `__value_`.

Currently `__tree` stores the start of the tree, `__begin_node_`, as a pointer to a `__tree_node`. Additionally the iterators store their position as a pointer to a `__tree_node`. In both of these cases the pointee can be the end node. This is fixed by changing them to store `__tree_end_node` pointers instead.

To make this change I introduced an `__iter_pointer` typedef which is defined to be a pointer to either `__tree_end_node` in the new ABI or `__tree_node` in the current one.
Both `__tree::__begin_node_` and iterator pointers are now stored as `__iter_pointers`.

The other situation where `__tree_end_node` is stored as the wrong type is in `__tree_node_base::__parent_`.  Currently `__left_`, `__right_`, and `__parent_` are all `__tree_node_base` pointers. Since the end node will only be stored in `__parent_` the fix is to change `__parent_` to be a pointer to `__tree_end_node`.

To make this change I introduced a `__parent_pointer` typedef which is defined to be a pointer to either `__tree_end_node` in the new ABI or `__tree_node_base` in the current one.

Note that in the new ABI `__iter_pointer` and `__parent_pointer` are the same type (but not in the old one). The confusion between these two types is unfortunate but it was the best solution I could come up with that maintains the ABI.

The typedef changes force a ton of explicit type casts to correct pointer types and to make current code compatible with both the old and new pointer typedefs. This is the bulk of the change and it's really messy. Unfortunately I don't know how to avoid it.

Please let me know what you think.





Reviewers: howard.hinnant, mclow.lists

Subscribers: howard.hinnant, bbannier, cfe-commits

Differential Revision: https://reviews.llvm.org/D20786
------------------------------------------------------------------------

llvm-svn: 276212
  • Loading branch information
EricWF committed Jul 20, 2016
1 parent 64903ae commit 669800f
Show file tree
Hide file tree
Showing 8 changed files with 302 additions and 188 deletions.
2 changes: 2 additions & 0 deletions libcxx/include/__config
Expand Up @@ -41,6 +41,8 @@
#define _LIBCPP_ABI_INCOMPLETE_TYPES_IN_DEQUE
// Fix undefined behavior in how std::list stores it's linked nodes.
#define _LIBCPP_ABI_LIST_REMOVE_NODE_POINTER_UB
// Fix undefined behavior in how __tree stores its end and parent nodes.
#define _LIBCPP_ABI_TREE_REMOVE_NODE_POINTER_UB
#define _LIBCPP_ABI_FORWARD_LIST_REMOVE_NODE_POINTER_UB
#define _LIBCPP_ABI_FIX_UNORDERED_CONTAINER_SIZE_TYPE
#define _LIBCPP_ABI_VARIADIC_LOCK_GUARD
Expand Down
444 changes: 257 additions & 187 deletions libcxx/include/__tree

Large diffs are not rendered by default.

Expand Up @@ -24,6 +24,9 @@ struct Node
Node* __parent_;
bool __is_black_;

Node* __parent_unsafe() const { return __parent_; }
void __set_parent(Node* x) { __parent_ = x;}

Node() : __left_(), __right_(), __parent_(), __is_black_() {}
};

Expand Down
Expand Up @@ -23,6 +23,9 @@ struct Node
Node* __right_;
Node* __parent_;

Node* __parent_unsafe() const { return __parent_; }
void __set_parent(Node* x) { __parent_ = x;}

Node() : __left_(), __right_(), __parent_() {}
};

Expand Down
Expand Up @@ -24,6 +24,9 @@ struct Node
Node* __parent_;
bool __is_black_;

Node* __parent_unsafe() const { return __parent_; }
void __set_parent(Node* x) { __parent_ = x;}

Node() : __left_(), __right_(), __parent_(), __is_black_() {}
};

Expand Down
Expand Up @@ -23,6 +23,9 @@ struct Node
Node* __right_;
Node* __parent_;

Node* __parent_unsafe() const { return __parent_; }
void __set_parent(Node* x) { __parent_ = x;}

Node() : __left_(), __right_(), __parent_() {}
};

Expand Down
@@ -0,0 +1,31 @@
//===----------------------------------------------------------------------===//
//
// The LLVM Compiler Infrastructure
//
// This file is dual licensed under the MIT and the University of Illinois Open
// Source Licenses. See LICENSE.TXT for details.
//
//===----------------------------------------------------------------------===//

// RUN: %build -O2
// RUN: %run

// <map>

// Previously this code caused a segfault when compiled at -O2 due to undefined
// behavior in __tree. See https://llvm.org/bugs/show_bug.cgi?id=28469

#include <functional>
#include <map>

void dummy() {}

struct F {
std::map<int, std::function<void()> > m;
F() { m[42] = &dummy; }
};

int main() {
F f;
f = F();
}
1 change: 0 additions & 1 deletion libcxx/test/ubsan_blacklist.txt
@@ -1,2 +1 @@
fun:*__tree*
fun:*__hash_table*

0 comments on commit 669800f

Please sign in to comment.