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

WIP: add safer unwrap #718

Closed
wants to merge 1 commit into from

Conversation

DaAitch
Copy link
Contributor

@DaAitch DaAitch commented May 5, 2020

Hello,

this PR prevents that ObjectWrap::Unwrap can be used with any type. It has to be a type having ObjectWrap<T> as sub class.

I spend some time finding a bug in my appliction, which did not segfault, because I did something like this test/objectwrap-saferunwrap.cc and it compiles and the reason I found out something is really going wrong is, that the stacktrace shows calling a method ->A() actually called ->B(). Then I realized, that it might be is a vtable pointer fixup problem and understand that of course it's possible, but not allowed to reinterpret cast void * to any type but the the original one.

So to prevent that s.o. tries to seductively unwrap an ObjectWrap<Car> or ObjectWrap<Truck> to any sub class like IVehicle (which obviously is not a ObjectWrap) it should not compile.

What do you think?

If this has a chance to land, we my need "should not compile" tests.

@mhdawson
Copy link
Member

mhdawson commented May 6, 2020

I like the concept, do you have ideas of how we'd add the "should not compile" tests?

@gabrielschulhof
Copy link
Contributor

We should call the function SafeUnwrap not SaferUnwrap and the C preprocessor variable should be NAPI_SAFE_UNWRAP. If present, it should do the safe napi_unwrap, otherwise it should do the regular napi_unwrap. The way it is now is kind of confusing to me.

@DaAitch is this related to #732 (multiple inheritance)?

@mhdawson maybe we should have a second target in the test/binding.gyp file which only gets run in the CI and is considered failed if it successfully compile. Might be kind of a crude check though, because it might be quite an effort to start analyzing the compiler's stderr output to see if it generated the right error message. Our many supported platforms each have a compiler that produces its own individual message, and the message may change with the version of the compiler, so if we analyzed compiler stderr output we might be signing up for quite a maintenance burden.

The PR needs a rebase.

@mhdawson
Copy link
Member

@gabrielschulhof having that second target which fails makes sense to me :)

@mhdawson
Copy link
Member

@DaAitch any chance you can rebase so we can move forward.

@DaAitch
Copy link
Contributor Author

DaAitch commented Nov 9, 2020

@mhdawson sorry for the delay, I didn't read notifications for a long time :(. If you still think this may help, please feel free to copy and adapt my code and close this PR. At present I think I'm too far away from this topic. Sorry for that

Base automatically changed from master to main January 26, 2021 22:43
@mhdawson
Copy link
Member

I think at this point based on the last comment we'll close, and somebody can reopen in the future if they'd like to get it landed.

@mhdawson mhdawson closed this Jun 14, 2021
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.

3 participants