Fix undefined behavior #210
Conversation
It's undefined behavior to dereference a NULL pointer, even if the value pointed to doesn't get used.
|
This problem was detected running with clang with -fsanitize=undefined. |
| limit = &old_rep->elements[current_size_]; | ||
| for (; e < limit; e++) { | ||
| e->Element::~Element(); | ||
| if (current_size_ > 0) { |
There was a problem hiding this comment.
Maybe simpler just to write:
for (size_t i = 0; i < current_size_; i++) {
Element* e = old_rep->elements[i];
e->Element::~Element();
}
Also doesn't some of the code above have the same problem if new_size == 0 (that doesn't make a lot of sense to do, but it shouldn't trigger undefined behavior).
There was a problem hiding this comment.
I think you're right: will update.
There was a problem hiding this comment.
Careful! If Element::~Element() is not visible to the compiler, it implies a memory barrier, so the old_rep->elements fetch is not loop-invariant, so it can't be hoisted out of the loop. That's the reason for the e and limit pointers pre-computed above the loop. This code is very sensitive for both code size and performance, so please verify equivalent machine code for types with nontrivial destructors.
There was a problem hiding this comment.
(And to be clear, when I say "memory barrier" I mean "barrier across which loads/stores can't be moved", not a true hardware barrier.)
|
Unfortunately, for the reasons Chris mentioned the more complicated code is probably required for efficiency/code-size reasons. :( |
|
Revisiting with new comment -- I think the |
|
Hi @ctiller, I'm cleaning out old PRs. It looks like the second part of your PR has been fixed in the meantime -- the second loop is now surrounded with Have you seen any more of these sanitize errors since filing this report? Do you have an easy way of re-running this test? |
It's undefined behavior to dereference a NULL pointer, even if the value pointed to doesn't get used.