Page MenuHomePhabricator

[lldb-mi] Check raw pointers before passing them to std::string ctor/assignment
ClosedPublic

Authored by tatyana-krasnukha on Dec 13 2018, 6:11 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

Fixed misspelling

shafik added a subscriber: shafik.Feb 14 2019, 12:05 PM

Looks like a great fix!

MIDataTypes.h
67 ↗(On Diff #186892)

Can we use llvm::Optional instead? Then we can use getValueOr().

71 ↗(On Diff #186892)

If we can not use llvm::Optional then why not just take T *?

Thanks for the review!

MIDataTypes.h
67 ↗(On Diff #186892)

Unfortunately no, with Optional<T*> getValueOr doesn't have the constraint that alternative value is not nullptr.
In case of Optional<T> getValueOr returns T by value which is not what we need.

71 ↗(On Diff #186892)

It is easier to read:

const char *pFileName =
    GetDereferenceable(fileSpec.GetFilename()).Or(*pUnknown);

vs

const char *pFileName = fileSpec.GetFilename();
pFileName = (pFileName != nullptr) ? pFileName : pUnknown;

isn't it?

In the future please include context with the diff (-U9999 should do the trick), it makes reviewing a lot easier.

Have you considered wrapping raw strings in llvm's StringRefs? It looks like lldb-mi is already using some ADT classes from llvm.
Unfortunately we cannot use those in the SB interface, but they're a lot safer. They have a .str() which does the right thing and returns an empty string when the raw string is a nullptr. Personally I would very much prefer that over the This.or(That) pattern, but if you really wanted that you could have an Optional<StringRef> and do getValueOr() as Shafik suggested.

tatyana-krasnukha retitled this revision from Check pointer results on nullptr before using them to [lldb-mi] Check raw pointers before passing them to std::string ctor/assignment.

Thank you for mentioning StringRef, you gave me the idea to keep pointers check inside the CMIUtilString to obviate undefined behavior. This is the best place to do it, however, a caller still should examine pointers he passes to CMIUtilString::Format as the ellipsis parameter.

Good initiative to fix potential nullptr assignments to std::string.

a caller still should examine pointers he passes to CMIUtilString::Format as the ellipsis parameter.

So Format() and Printf() don't handle nullptr arguments already?

Have you considered wrapping raw strings in llvm's StringRefs?

+1 in general (but it may be a bigger effort)

unittests/tools/lldb-mi/CMakeLists.txt
1 ↗(On Diff #187153)

LLDB_PROJECT_ROOT is rarely used, maybe prefer LLDB_SOURCE_DIR?

unittests/tools/lldb-mi/utils/CMakeLists.txt
3 ↗(On Diff #187153)

Nice workaround for not having a library target at hand for linking!

unittests/tools/lldb-mi/utils/StringTest.cpp
1 ↗(On Diff #187153)

update name

Thanks for the review!

So Format() and Printf() don't handle nullptr arguments already?

They just forward arguments to vsprintf/vsnprintf. Otherwise, the would have to do twice the work parsing format string.

+1 in general (but it may be a bigger effort)

Since lldb-mi has its own wrapper for strings, we would have a conversion const char* -> StringRef -> CMIUtilString everywhere. This looks redundant to me and it is easy to forget converting a result of SB class's function to the StringRef.

LLDB_PROJECT_ROOT -> LLDB_SOURCE_DIR

abidh accepted this revision.Feb 25 2019, 3:57 AM

The lldb-mi bits look ok to me.

tools/lldb-mi/MIUtilString.h
36 ↗(On Diff #187613)

May be spurious new line

This revision is now accepted and ready to land.Feb 25 2019, 3:57 AM

Removed new lines from MIUtilString.h

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2019, 8:42 AM
hintonda added inline comments.
lldb/trunk/unittests/tools/lldb-mi/utils/CMakeLists.txt
12

Just wanted to let you know that using $<TARGET_OBJECTS:...> in anything other than add_library and add_executable wasn't added until cmake 3.9, so I'm unable to configure lldb using the officially supported cmake 3.4.3 version.

Btw, I use cmake 3.4.3 locally so I don't unintentionally use new unsupported features to the clang/llvm cmake files.

lldb/trunk/unittests/tools/lldb-mi/utils/CMakeLists.txt
12

Thank you for pointing to this issue. I had cmake 3.6.2 version installed and, oddly enough, it worked well.
I'd appreciate your review of the patch that fixes the issue.

hintonda added inline comments.Jun 19 2019, 10:51 AM
lldb/trunk/unittests/tools/lldb-mi/utils/CMakeLists.txt
12

I don't have all the various version installed, so I was trying to figure out which version by looking at the online docs. If 3.6.2 works, then it changed somewhere between 3.4.3 and 3.6.2, so I guess you can just use >= 3.6.2 for your test.

Thanks again for looking into this.