This is an archive of the discontinued LLVM Phabricator instance.

Differential D40743

Make rehash(0) work with ubsan's unsigned-integer-overflow.
ClosedPublic

Authored by danalbert on Dec 1 2017, 12:29 PM.

Details

Reviewers
mclow.lists
EricWF
Commits
rG0802327a3931: Make rehash(0) work with ubsan's unsigned-integer-overflow.
rL322031: Make rehash(0) work with ubsan's unsigned-integer-overflow.
rCXX322031: Make rehash(0) work with ubsan's unsigned-integer-overflow.

Diff Detail

Event Timeline

danalbert created this revision.Dec 1 2017, 12:29 PM
mclow.lists edited edge metadata.Dec 11 2017, 10:26 AM

Dan - I think I need a bit more context here.
How does UBSan get triggered?

danalbert added inline comments.Dec 11 2017, 8:59 PM
include/__hash_table
2141

With rehash(0) this is 0 & (0 - 1), which triggers unsigned-integer-overflow.

mclow.lists accepted this revision.Dec 14 2017, 7:29 AM
mclow.lists added inline comments.
include/__hash_table
2141

Grumble, grumble. That's not UB, that's just UBSan whining.
On the other hand, this doesn't appear to change any behavior, and shuts UBSan up.

This revision is now accepted and ready to land.Dec 14 2017, 7:29 AM
Closed by commit rCXX322031: Make rehash(0) work with ubsan's unsigned-integer-overflow. (authored by danalbert). · Explain WhyJan 8 2018, 1:50 PM
This revision was automatically updated to reflect the committed changes.
danalbert added inline comments.Jan 8 2018, 1:53 PM
include/__hash_table
2141

FWIW, this check isn't enabled in ubsan by default. Android uses it for testing and mititgation in a handful of projects because it does catch errors (stagefright was an unsigned overflow issue). It's good to keep the libc++ headers clean of these issues so we're not dictating the compile flags of libc++ users.

Revision Contents

PathSize
include/
2 lines

Diff 125193

include/__hash_table

Show First 20 Lines • Show All 2,130 Lines • ▼ Show 20 Lines
} }
#endif // _LIBCPP_CXX03_LANG #endif // _LIBCPP_CXX03_LANG
template <class _Tp, class _Hash, class _Equal, class _Alloc> template <class _Tp, class _Hash, class _Equal, class _Alloc>
void void
__hash_table<_Tp, _Hash, _Equal, _Alloc>::rehash(size_type __n) __hash_table<_Tp, _Hash, _Equal, _Alloc>::rehash(size_type __n)
{ {
if (__n == 1) if (__n < 2)
__n = 2; __n = 2;
else if (__n & (__n - 1)) else if (__n & (__n - 1))
danalbertAuthorUnsubmitted
Not Done
ReplyInline Actions

With rehash(0) this is 0 & (0 - 1), which triggers unsigned-integer-overflow.

danalbert: With `rehash(0)` this is `0 & (0 - 1)`, which triggers unsigned-integer-overflow.
mclow.listsUnsubmitted
Not Done
ReplyInline Actions

Grumble, grumble. That's not UB, that's just UBSan whining.
On the other hand, this doesn't appear to change any behavior, and shuts UBSan up.

mclow.lists: Grumble, grumble. That's not UB, that's just UBSan whining. On the other hand, this doesn't…
danalbertAuthorUnsubmitted
Not Done
ReplyInline Actions

FWIW, this check isn't enabled in ubsan by default. Android uses it for testing and mititgation in a handful of projects because it does catch errors (stagefright was an unsigned overflow issue). It's good to keep the libc++ headers clean of these issues so we're not dictating the compile flags of libc++ users.

danalbert: FWIW, this check isn't enabled in ubsan by default. Android uses it for testing and mititgation…
__n = __next_prime(__n); __n = __next_prime(__n);
size_type __bc = bucket_count(); size_type __bc = bucket_count();
if (__n > __bc) if (__n > __bc)
__rehash(__n); __rehash(__n);
else if (__n < __bc) else if (__n < __bc)
{ {
__n = _VSTD::max<size_type> __n = _VSTD::max<size_type>
( (
▲ Show 20 Lines • Show All 527 Lines • Show Last 20 Lines