This is an archive of the discontinued LLVM Phabricator instance.

Return a shared_ptr from ScratchTypeSystemClang::GetForTarget()
ClosedPublic

Authored by aprantl on Jan 5 2023, 5:01 PM.

Details

Summary

The current interface theoretically could lead to a use-after-free when a client holds on to the returned pointer. Ficx this by returning a shared_ptr to the scratch typesystem

rdar://103619233

Diff Detail

Event Timeline

aprantl created this revision.Jan 5 2023, 5:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2023, 5:01 PM
aprantl requested review of this revision.Jan 5 2023, 5:01 PM
Michael137 accepted this revision.Jan 9 2023, 6:07 AM

LGTM

lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
9944

Redundant std::static_pointer_cast?

This revision is now accepted and ready to land.Jan 9 2023, 6:07 AM

I don't like how this patch (unintentionally) hides that the returned object is a shared pointer.

  • Part of the issue is the use of auto. I don't think the uses in this patch pass the mustard with regards to the coding standards.
  • The other part is the variable name: we use the _sp suffix for shared pointers. Changing the name of the variable might cause a bunch of churn so I understand why this isn't desirable.

Replacing the instances of auto with TypeSystemClangSP would address my concerns.

when a client holds on to the returned pointer

I'm not familiar with the APIs involved here, but when I read the sentence above, what comes to mind is that the contract of a raw pointer is that a client shouldn't hold it past the lifetime of the object that produced this raw pointer. In fact, all uses updated in this patch seem to respect that.

An inspection of the code here seems to indicate that the Target object is the owner of the TypeSystem object.

As I said though, I looked at this code for the first time just now, so feel free to ignore it if I'm missing something :)

I don't like how this patch (unintentionally) hides that the returned object is a shared pointer.

  • Part of the issue is the use of auto. I don't think the uses in this patch pass the mustard with regards to the coding standards.
  • The other part is the variable name: we use the _sp suffix for shared pointers. Changing the name of the variable might cause a bunch of churn so I understand why this isn't desirable.

Replacing the instances of auto with TypeSystemClangSP would address my concerns.

As much as I dislike the amount of work this is going to be, I actually agree.

when a client holds on to the returned pointer

I'm not familiar with the APIs involved here, but when I read the sentence above, what comes to mind is that the contract of a raw pointer is that a client shouldn't hold it past the lifetime of the object that produced this raw pointer. In fact, all uses updated in this patch seem to respect that.

An inspection of the code here seems to indicate that the Target object is the owner of the TypeSystem object.

As I said though, I looked at this code for the first time just now, so feel free to ignore it if I'm missing something :)

Scratch TypeSystems (particularly TypeSystemSwift) can live shorter than their owning Target. Fatal compiler errors can cause them to get torn down and replaced by a new instance.

aprantl updated this revision to Diff 487558.Jan 9 2023, 2:36 PM

Address @JDevlieghere 's comments.

JDevlieghere accepted this revision.Jan 9 2023, 2:36 PM

LGTM 🚢

aprantl added inline comments.Jan 9 2023, 2:38 PM
lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.cpp
9944

No, this one is to be necessary to cast from TypeSystem -> TypeSystremClang.

This revision was landed with ongoing or failed builds.Jan 9 2023, 3:05 PM
This revision was automatically updated to reflect the committed changes.