This is an archive of the discontinued LLVM Phabricator instance.

[lldb/DataExtractor] Fix UB shift in GetMaxS64Bitfield
ClosedPublic

Authored by vsk on Feb 3 2020, 11:28 AM.

Details

Summary

DataExtractor::GetMaxS64Bitfield performs a shift with UB in order to
construct a bitmask when bitfield_bit_size is 64. The current
implementation actually does “work” in this case, because the assumption
that the shift result is 0 holds, and 0 minus 1 gives the all-ones value
(the correct mask). However, the more readable/maintainable approach
might be to use an off-the-shelf UB-free helper.

Fixes a UBSan issue:

"col" : 37,
"description" : "invalid-shift-exponent",
"filename" : "/Users/vsk/src/llvm-project-master/lldb/source/Utility/DataExtractor.cpp",
"instrumentation_class" : "UndefinedBehaviorSanitizer",
"line" : 615,
"memory_address" : 0,
"summary" : "Shift exponent 64 is too large for 64-bit type 'uint64_t' (aka 'unsigned long long')",

rdar://59117758

Diff Detail

Event Timeline

vsk created this revision.Feb 3 2020, 11:28 AM
shafik added a subscriber: shafik.Feb 3 2020, 11:57 AM

I wish I had caught that one when I did D70992, might be worth checking out more of the code for similar issues.

shafik added inline comments.Feb 3 2020, 12:00 PM
lldb/source/Utility/DataExtractor.cpp
610

We could do an early exit if bitfield_bit_size is zero.

davide added a subscriber: davide.Feb 3 2020, 12:06 PM

I wish I had caught that one when I did D70992, might be worth checking out more of the code for similar issues.

I guess this is why we have tooling =)

davide accepted this revision.Feb 3 2020, 12:06 PM

LGTM, thanks for taking care of this.

This revision is now accepted and ready to land.Feb 3 2020, 12:06 PM
vsk marked an inline comment as done.Feb 3 2020, 2:37 PM
vsk added inline comments.
lldb/source/Utility/DataExtractor.cpp
610

Yes, I think that'd simplify things. I'll do this in a follow up commit.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2020, 4:03 PM