Skip to content

Thread-safety for the type metadata cache #31663

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Thread-safety for the type metadata cache #31663

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions stdlib/public/runtime/Metadata.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,9 @@ extern "C" void _objc_setClassCopyFixupHandler(void (* _Nonnull newFixupHandler)
#define VM_TAG_FOR_SWIFT_METADATA (-1)
#endif

// Protects concurrent accesses to the metadata cache
static std::recursive_mutex metadata_mutex;

using namespace swift;
using namespace metadataimpl;

Expand Down Expand Up @@ -684,6 +687,7 @@ MetadataResponse
swift::swift_getGenericMetadata(MetadataRequest request,
const void * const *arguments,
const TypeContextDescriptor *description) {
std::lock_guard<std::recursive_mutex> guard(metadata_mutex);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We didn't just forget to add a lock here. If there's something wrong with our current implementation, we should figure it out, but grabbing a lock at the start of a bunch of runtime functions that are meant to work without locks is not acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rjmccall Hey, we know. We're just trying to get the problem on everybody's radar. How these functions are “meant to work” is not immediately obvious from the comments, and I guess some other people around here probably do understand that. If someone would like to explain that, we might be able to attempt an acceptable fix. We won't be insulted either if this PR is closed and replaced with one that is acceptable. 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, this is a pretty aggressive way of reporting a bug.

I opened #31768 for the associated-witness issue, which should fix Parker's bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, man, no offense intended. I thought such a bug in the runtime (which I presume to be a security issue) might be important enough that y'all might be interested in a quick fix. I don't know what you think is aggressive about it. Just trying to be helpful, here. Next time I'll stick to Jira.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No offense taken; I'm just trying to explain that this is not, in fact, a working quick fix.

Please CC me on any concurrency bugs you file; we definitely want to treat them as high-priority.

description = swift_auth_data_non_address(description, SpecialPointerAuthDiscriminators::TypeDescriptor);
auto &cache = getCache(*description);
assert(description->getFullGenericContextHeader().Base.NumKeyArguments ==
Expand Down Expand Up @@ -4789,6 +4793,7 @@ swift::swift_getAssociatedTypeWitness(MetadataRequest request,
const Metadata *conformingType,
const ProtocolRequirement *reqBase,
const ProtocolRequirement *assocType) {
std::lock_guard<std::recursive_mutex> guard(metadata_mutex);
assert(assocType->Flags.getKind() ==
ProtocolRequirementFlags::Kind::AssociatedTypeAccessFunction);

Expand Down Expand Up @@ -4916,6 +4921,7 @@ const WitnessTable *swift::swift_getAssociatedConformanceWitness(
const Metadata *assocType,
const ProtocolRequirement *reqBase,
const ProtocolRequirement *assocConformance) {
std::lock_guard<std::recursive_mutex> guard(metadata_mutex);
// We avoid using this function for initializing base protocol conformances
// so that we can have a better fast-path.
assert(assocConformance->Flags.getKind() ==
Expand Down