This is an archive of the discontinued LLVM Phabricator instance.

[LLDB][NFC] Fix valobj_sp null pointer checks in lldb/source/Plugins/Language/
AbandonedPublic

Authored by xgupta on Jan 23 2023, 2:54 AM.

Details

Summary

The issue is that the SyntheticChildrenFrontEnd constructor is being
called with the dereferenced valobj_sp pointer, before the valobj_sp
pointer is verified to be non-null inside the body of the constructor function.

To fix the issue, we have moved the call to the SyntheticChildrenFrontEnd
constructor to after the null check for valobj_sp in the constructor
function body, so that it will only be called if valobj_sp is non-null.

Found by PVS-Studio - https://pvs-studio.com/en/blog/posts/cpp/1003/, N19-N36.

Diff Detail

Event Timeline

xgupta created this revision.Jan 23 2023, 2:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 2:54 AM
xgupta requested review of this revision.Jan 23 2023, 2:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 23 2023, 2:54 AM
xgupta edited the summary of this revision. (Show Details)Jan 23 2023, 2:55 AM
Michael137 added a subscriber: Michael137.EditedJan 23 2023, 3:16 AM

Looks correct to me, given we seem to allow valobj_sp to be null. Though we seem to be doing it in a lot of other places too around this file. So could fix those all at once?

Looks correct to me, given we seem to allow valobj_sp to be null. Though we seem to be doing it in a lot of other places too around this file. So could fix those all at once?

Yeah I am doing that, will update in 30 minutes or so.

xgupta updated this revision to Diff 491288.Jan 23 2023, 3:44 AM

address comments

xgupta retitled this revision from [LLDB][NFC] Fix a null pointer check in LibCxx.cpp to [LLDB][NFC] Fix valobj_sp null pointer checks in lldb/source/Plugins/Language/.Jan 23 2023, 3:48 AM
xgupta edited the summary of this revision. (Show Details)
xgupta added a reviewer: Michael137.
Michael137 added inline comments.Jan 23 2023, 5:53 AM
lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
225

this won't initialise the parent constructor though will it? Just creates a temporary and immediately destructs it? You might need to put it back into the initialiser list and use a ternary

Michael137 added inline comments.Jan 23 2023, 5:59 AM
lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
225

Oh but SyntheticChildrenFrontEnd seems to always take a reference. The interfaces seem to be a little mismatched

Michael137 added inline comments.
lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
225

Perhaps the null-check in the constructor is just redundant and should actually be an assert.

@jingham might know more about the assumptions here

xgupta added a comment.Feb 3 2023, 2:45 AM

gentle ping for review.

gentle ping for review.

kastiglione added inline comments.Feb 17 2023, 7:19 AM
lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
225

@xgupta reiterating Michael's point, I think this change results in mis-construction. Have you run the test suite, I would expect some failures with this change.

I agree that this should be an assert. The other option would be to change the constructor's signature, using a lldb::ValueObject & instead of lldb::ValueObjectSP, which puts the onus on callers to handle any null-ness. Are we aware of any callers passing null?

kastiglione added inline comments.Feb 17 2023, 7:21 AM
lldb/source/Plugins/Language/ObjC/NSSet.cpp
673–675

by the way, these changes are missing {} around the if body.

Michael137 added inline comments.Feb 17 2023, 10:11 AM
lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp
225

FYI, from offline conversation with Jim:

This is one of those things where you really should have a general guard against trying to create a FrontEnd with a null ValueObjectSP, since this isn't going to do any good.  But then you worry "what if one leaks through by accident" so you put some fallback code when that happens.  We were really insistent that "if we make a programming error in viewing a variable we'd really like to limit the damage and not crash..." So we didn't go straight to asserts in all these places the way clang, for instance, does.  Taking down a whole debug session because one variable was bizarre (maybe the DWARF was bad in a way that confused us...) is not acceptable, there's too much state in the investigations the debugger supports so we were often over cautious about these sort of checks.

The better solution would be to go to all the places that produce synthetic children and make sure that they error out early when told to format empty SP's.

Not sure how doable that is.  For most cases, the Synthetic Child Providers come from type matches, and an empty ValueObjectSP doesn't have a type, so it would never make a match.  But we do have a few "try it against anything" formatters now, things like the C++ std::function matchers, and the python functional matchers, so there are places where we might end up screwing this up.
xgupta abandoned this revision.Jul 16 2023, 9:29 AM

I am sorry for taking time, but I could not understand the right issue. Hence I am abandoning it.