Skip to content
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.

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

Fix global ::delete on virtual object #229

Merged
merged 1 commit into from
May 2, 2024
Merged

Fix global ::delete on virtual object #229

merged 1 commit into from
May 2, 2024

Conversation

DutChen18
Copy link
Member

Using ::delete would fail because ItaniumCXXABI::emitVirtualObjectDelete used the wrong offset (-2) into the vtable to find the complete-object pointer.

Example (triggers address sanitizer):

struct Base {
        virtual ~Base() {}
};

struct Derived : Base {};

int main() {
        Derived* derived = ::new Derived();
        ::delete derived;
}

@DutChen18 DutChen18 requested a review from yuri91 May 1, 2024 14:19
@yuri91
Copy link
Member

yuri91 commented May 1, 2024

I was incorrect in my explanation on discord: for dynamic_cast<void*> we generate a "virtualcast" for both asmjs and genericjs. It's just different the way we compute the offset (for genericjs it's always 0, for asmjs it is stored at index 0 in the vtable and needs to be loaded).

So here you are computing the offset correctly but then you are going to the regular logic for the pointer adjustment, while you still need the virtualcast. (it's possible that for asmjs/wasm it ends up giving you the exact same address, but for consistency let's use virtualcast for both cases)

Using `::delete` would fail because
`ItaniumCXXABI::emitVirtualObjectDelete` used the wrong offset (-2) into
the vtable to find the complete-object pointer.

Example (triggers address sanitizer):

```
struct Base {
        virtual ~Base() {}
};

struct Derived : Base {};

int main() {
        Derived* derived = ::new Derived();
        ::delete derived;
}
```
@DutChen18 DutChen18 force-pushed the fix-global-delete branch from 5b48af3 to 0bf4281 Compare May 1, 2024 15:09
@DutChen18
Copy link
Member Author

How about this then?

Copy link
Member

@yuri91 yuri91 left a comment

Choose a reason for hiding this comment

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

Looks good to me. I will wait the CI before merging.

@yuri91 yuri91 merged commit 0f0e782 into master May 2, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants