Skip to content

Cast IMPs to the correct signature before calling - #418

Open
HendrikHuebner wants to merge 1 commit into
gnustep:masterfrom
HendrikHuebner:wasm-imp-signatures
Open

Cast IMPs to the correct signature before calling#418
HendrikHuebner wants to merge 1 commit into
gnustep:masterfrom
HendrikHuebner:wasm-imp-signatures

Conversation

@HendrikHuebner

Copy link
Copy Markdown
Contributor

This PR ensures we do not call any raw IMP function pointers without casting them to the correct type first.

Calling a funtion pointer with a mismatching signature is technically UB I believe but tends to work just fine on many native platforms.
With WebAssembly, this is a problem however, because WASM indirect calls require an exact signature match. This can cause runtime failures.

Comment thread arc.mm Outdated
Comment thread arc.mm Outdated
Comment on lines +572 to +574
NewAutoreleasePool = reinterpret_cast<NewAutoreleasePoolIMP>(
class_getMethodImplementation(object_getClass(AutoreleasePool),
SELECTOR(new)));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we consolidate the casts with something like:

auto storeSelector = [](auto &target, IMP imp)
    {
         target = reinterpret_cast<std::remove_reference_t<decltype(target)>>(imp);
    };

Comment thread runtime.c
// `.cxx_destruct` method may deallocate the class.
Class currentClass = cls;
cls = cls->super_class;
if (currentClass->cxx_destruct)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We should probably change the type of these from IMP and do the cast when we set them.

@HendrikHuebner

Copy link
Copy Markdown
Contributor Author

updated

@davidchisnall

Copy link
Copy Markdown
Member

Please can you squash for merging?

@HendrikHuebner

Copy link
Copy Markdown
Contributor Author

Done. I don't think I have permissions to merge PRs myself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants