This is an archive of the discontinued LLVM Phabricator instance.

Avoid calling std::memcmp with nullptr
ClosedPublic

Authored by vitalybuka on Nov 14 2016, 3:51 PM.

Details

Summary

UBSAN complains that this is undefined behavior.

We can assume that empty substring (N==1) always satisfy conditions. So
std::memcmp will be called only only for N > 1 and Str.size() > 0.

Event Timeline

vitalybuka updated this revision to Diff 77903.Nov 14 2016, 3:51 PM
vitalybuka retitled this revision from to Avoid calling std::memcmp with nullptr.
vitalybuka updated this object.
vitalybuka added reviewers: ruiu, zturner.
vitalybuka added a subscriber: llvm-commits.
zturner added inline comments.Nov 14 2016, 3:54 PM
include/llvm/ADT/StringSwitch.h
76

I don't think this second assert is valid. Consider this code:

char dir[MAX_PATH];
sprintf(dir, "%s/%s", parent, child);
auto X = StringSwitch<int>(MyPath).Case(dir, 1);

There is no guarantee what is the value of S[N-1]

ruiu edited edge metadata.Nov 14 2016, 3:57 PM

Do you mean that if it is called with "", such as StartsWith("", foo), S becomes a nullptr?

zturner edited edge metadata.Nov 14 2016, 4:01 PM
In D26646#595108, @ruiu wrote:

Do you mean that if it is called with "", such as StartsWith("", foo), S becomes a nullptr?

Were you asking me? My comment is just saying that string literal is not the only way to invoke this overload. You can invoke it with a char array too, and strlen(char_array) is not guaranteed to be the same as sizeof(char_array) - 1.

ruiu added a comment.Nov 14 2016, 4:03 PM

No, I was asking Vitaly. The commit message says that "Avoid calling std::memcmp with nullptr", but I don't understand how can a nullptr be passed to memcmp.

If Str is empty then Str.data() will be nullptr

vitalybuka updated this revision to Diff 77909.Nov 14 2016, 4:05 PM
vitalybuka edited edge metadata.

Removed assert(S[N-1] == '\0')

vitalybuka marked an inline comment as done.Nov 14 2016, 4:05 PM
zturner accepted this revision.Nov 14 2016, 4:09 PM
zturner edited edge metadata.
This revision is now accepted and ready to land.Nov 14 2016, 4:09 PM
This revision was automatically updated to reflect the committed changes.