UBSAN error in GetBackingStore()

[jeroen]

The call to GetBackingStore() which was introduced in #117 seems to trigger an ubsan error.

Reproduce with Dockerfile like this:

FROM wch1/r-debug
RUN RDsan -e "install.packages('V8')"
RUN RDsan -e 'ctx=V8::v8(); ctx$get("new ArrayBuffer(8);")'

# real world example
# RUN RDsan -e "example(wasm, package='V8')"

[JanMarvin]

Maybe unrelated, but we can replace this

buffer->GetBackingStore()->Data()

with this

buffer->Data()

Though I haven't tested it with docker, just found it googling.

[jeroen]

Strangely this gives a compile error on libv8 9.1 and 9.4 (which is what CRAN uses)

bindings.cpp:212:34: error: invalid use of ‘v8::Data::Data’
  212 |     memcpy(data.begin(), buffer->Data(), data.size());

Which is weird because they were introduced at the same time: v8/v8@55c4882

[JanMarvin]

Okay, strange indeed. It works on my Arch with V8 10.x (whatever the latest version was), but I'm not sure why I didn't try it earlier. Maybe it wasn't working at the time. My last V8 9.4 build was mid August last year.

[jeroen]

Actually no, that API was only added recently in 10.5: v8/v8@00704f5

[JanMarvin]

There's a national holiday upcoming, I can try to craft a c++ only example to replicate the issue. If successful we could open an issue at the V8 bug tracker.

My assumption is still that it's a false positive and that it's caused by V8 not exporting std::shared_pointer in GetBackingStore() and ubsan getting confused by shared_pointer coming from the package and the shared library.

[JanMarvin]

I have been messing around with RDsan. In #153 I have replaced the following for v8 builds 10.5+ (and prepared a switch to v8::Globals<> from the lately deprecated v8::Persistent<>):

buffer->GetBackingStore()->Data()

with this

buffer->Data()

Using a local v8 static build 10.9.130 the issue is gone:

root@ae6223218f8b:/# RDsan

R Under development (unstable) (2022-10-29 r83201) -- "Unsuffered Consequences"
Copyright (C) 2022 The R Foundation for Statistical Computing
Platform: x86_64-pc-linux-gnu (64-bit)

R is free software and comes with ABSOLUTELY NO WARRANTY.
You are welcome to redistribute it under certain conditions.
Type 'license()' or 'licence()' for distribution details.

  Natural language support but running in an English locale

R is a collaborative project with many contributors.
Type 'contributors()' for more information and
'citation()' on how to cite R or R packages in publications.

Type 'demo()' for some demos, 'help()' for on-line help, or
'help.start()' for an HTML browser interface to help.
Type 'q()' to quit R.

> library(V8)
Using V8 engine 10.9.130
> ctx=V8::v8(); ctx$get("new ArrayBuffer(8);")
[1] 00 00 00 00 00 00 00 00
> 

I was unable to figure out a reason. With a modified v8 hello-world.cc I was unable to reproduce the ubsan warning. In addition I had a look with nm on the bindings.so file, in the hopes to verify my above statement. Though I couldn't see anything useful. I end my digging here.

[jeroen]

Thanks! CRAN seems to be testing with 9.4.146.24-node.20(Fedora-36) though. And the current LTS release of nodejs has 10.2.154.15-node.12 so even there we don't have the new API available. I think we need a workaround on those platforms too...

[JanMarvin]

Well, that should be solvable with skip_on_cran() :)

[jeroen]

That doesn't really solve the problem, because other CRAN packages that use V8 will also get these ubsan errors in their tests/examples.

[JanMarvin]

Yes, but to me that's still fighting windmills and I prefer to be Sancho in this tale. The initial fix was correct and once we use the new V8 API the issue is gone as well.

[jeroen]

Perhaps we can just use the old GetContents API on Fedora... When exactly did that get deprecated?

[JanMarvin]

v8/v8@578f6be if GitHub is correct in 9.1.117. I guess there is a window in which only the GetBackingStore logic works and CRAN using 9.4 is in this window.
You could update the static builds for Linux and enable them as default for CRAN testing? After all only the ubsan test throws a warning and ubsan runs on Linux?

[JanMarvin]

But the new logic was back ported by nodejs: nodejs/node#43921. Found this while searching for a node bug report regarding slow performance of GetBackingStore. At work right now and only windows around me, can test later today.

[jeroen]

Ah good find! So if I understand it correctly this API is available in NodeJS 18.8.0.

[jeroen]

And on NodeJS 16 (which is what CRAN/Fedora uses) they have re-added the old GetContents() API to make it compatible with v8-9.0: nodejs/node@a3db203.

So we can avoid the GetBackingStore() api on both Node-16 and Node-18. I just need to find the proper C macro to detect NodeJS.

[JanMarvin]

Maybe something along the lines of this #155 ?

[jeroen]

This is fixed, at least on the NodeJS LTS versions with dcee641

Thanks for your help @JanMarvin

[jeroen]

Fix is on CRAN now 🎉

[JanMarvin]

Congrats!