Page MenuHomePhabricator

Remove undefined behavior around the use of StateType
ClosedPublic

Authored by shafik on Aug 29 2018, 11:00 AM.

Details

Summary

lldb::StateType is an enum without an explicit underlying type.

According to [dcl.enum]p8 http://eel.is/c++draft/dcl.enum#8 and [expr.static.cast]p10 http://eel.is/c++draft/expr.static.cast#10 we are limited to setting StateType to values bound with a value range based on the values of the enumerators. Currently we have a test and SWIG interface that either explicitly set StateType to a value outside of the allowed range or in the case of SWIG does not prevent it.

In both cases using UBSan generates a runtime error when we load a value outside of the range allowed by the standard.

This change refactors StateType to keep track of the largest value and range checking to the SWIG interface and removes the invalid test case.

Diff Detail

Repository
rL LLVM

Event Timeline

shafik created this revision.Aug 29 2018, 11:00 AM

I wonder if it is worth asserting if a process ever returns a state of kNumStateType? I don't think it is necessary in things like StateIsStoppedState, somebody might have found their way here from the SB API's and we want to be forgiving there. But wherever we've done process->GetState(), that should never come back with kNumStateTypes. We already have eStateInvalid to mean "The debugger is confused about the state of the process". So there's never a reason why a process should report its state as kNumStateTypes.

Other than that, this looks fine to me.

aprantl added inline comments.Aug 29 2018, 1:00 PM
include/lldb/lldb-enumerations.h
60 ↗(On Diff #163140)

I think we usually do something like eLastsState = eStateSuspended. This avoid having to add the case to all the switches.

scripts/Python/python-typemaps.swig
89 ↗(On Diff #163140)

nice!
There's an extra space before the ;

source/Plugins/DynamicLoader/MacOSX-DYLD/DynamicLoaderDarwin.cpp
779 ↗(On Diff #163140)

Shouldn't this value trigger an error of sorts in all of these switch statements?

shafik marked 3 inline comments as done.Aug 30 2018, 5:01 PM

@jingham I am switching to the @aprantl suggestions which feels cleaner and removes this issue.

include/lldb/lldb-enumerations.h
60 ↗(On Diff #163140)

This approach seems like a much better idiom, I will switch to that.

shafik updated this revision to Diff 163450.Aug 30 2018, 5:02 PM
shafik marked an inline comment as done.

Switching enum guard to kLastStateType which references the last valid enum which lead to cleaner code instead of inventing a new value which does not have a good meaning in many cases.

aprantl accepted this revision.Aug 31 2018, 8:20 AM

Thanks! I assume there is a whole bunch of other enums for which we should be doing the same thing now?
Also there might be some 1-based ones for which we also need to check the lower bound.

This revision is now accepted and ready to land.Aug 31 2018, 8:20 AM

Thanks! I assume there is a whole bunch of other enums for which we should be doing the same thing now?
Also there might be some 1-based ones for which we also need to check the lower bound.

Be careful about enums in lldb-enumerations.h since they do make it into the SB API's. I don't think the terminating element matters because Python doesn't actually know these are enums, it just thinks they are global variables, but you'd have to make sure you don't mess up other C++ clients.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.