search

microsoft / node-api-dotnet

Advanced interoperability between .NET and JavaScript in the same process.
MIT License
426 stars 49 forks source link

Remove undefined behavior from JSTypedArray.GetJSValueForMemory #295

Closed DaZombieKiller closed 1 month ago

DaZombieKiller commented 1 month ago

The GetJSValueForMemory method currently uses unsafe code to access the internals of the Memory<T> type, resulting in undefined behavior.

Contrary to the comments inside, there has always been a public API to access the MemoryManager<T> backing a Memory<T> instance: MemoryMarshal.TryGetMemoryManager<T, TManager>. This PR replaces the implementation with one using that API to remove the undefined behavior.

With this new implementation, it is no longer necessary to strip the high bit from the index (it is an implementation detail that the MemoryMarshal APIs do not expose). Technically it wasn't necessary to begin with as that bit is only set for Memory<T> instances backed by pinned arrays, which are already ruled out by checking for a MemoryManager instance.

DaZombieKiller commented 1 month ago

@microsoft-github-policy-service agree

jasongin commented 1 month ago

Thanks for the PR! This is a great clean-up, as the previous code there (that I wrote) was certainly dubious. I guess I just did not discover the TryGetMemoryManager() API.