1211432 - Expand the safeWhenRacy operation set to avoid void* footguns

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Lars T Hansen [:lth]]

Follow-up work to bug 1084248.

Currently the safeWhenRacy operation set in jit/AtomicOperations.h has only load, store, memcpy, and memmove.  In a few places in the code, what used to be PodCopy is mapped onto the memcpy operation.  In a few other places, plain memcpy was previously used but on a byte count that was the result of a multiplication.

It might be desirable to add a PodCopy-like operation to avoid the multiplication (it's safer, for one thing), and to convert some untyped uses of the memcpy operation to this typed operation.

Comment 1

[Lars T Hansen [:lth]]

The one place I know that had memcpy before and now have the memcpy operation is in AsmJSSignalHandlers.

Comment 2

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=016fe741297e

Comment 3

[Lars T Hansen [:lth]]

The "p" failure in that run is bug 1230162, technically a problem with running tests in parallel but fundamentally we should also clean up SpiderMonkey.

Comment 4

[Lars T Hansen [:lth]]

Attachment: provide PodCopy and PodMove safe-when-racy operations

I think this is probably good enough because there is no guarantee against tearing - expressed or implied - and what this does is merely allow for existing PodCopy/PodMove use cases to be made safe-for-races without introducing multiplication footguns.  Please opine on the appropriateness of the release-mode assertion.

Comment 5

[Jeff Walden [:Waldo]]

Comment on attachment 8695792 [details] [diff] [review]
provide PodCopy and PodMove safe-when-racy operations

Review of attachment 8695792 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/AtomicOperations.h
@@ +248,5 @@
> +
> +    template<typename T>
> +    static void PodCopySafeWhenRacy(SharedMem<T*> dest, SharedMem<T*> src, size_t nelem) {
> +        MOZ_RELEASE_ASSERT(nelem <= SIZE_MAX / sizeof(T),
> +                           "trying to copy an impossible number of elements");

I don't think there's any need to assert this, here or below, let alone release-assert it.  Even if we release-asserted for things like this (note that mfbt/PodOperations.h doesn't), scribbling over all of memory a *very* improbable failure mode, IMO.

@@ +249,5 @@
> +    template<typename T>
> +    static void PodCopySafeWhenRacy(SharedMem<T*> dest, SharedMem<T*> src, size_t nelem) {
> +        MOZ_RELEASE_ASSERT(nelem <= SIZE_MAX / sizeof(T),
> +                           "trying to copy an impossible number of elements");
> +        memcpySafeWhenRacy(dest, src, nelem*sizeof(T));

Spaces around binary operators like *, here and below (those seem to be the only instances this patch perturbs without happening to remove).

::: js/src/vm/TypedArrayCommon.h
@@ +184,5 @@
>          ::memmove(dest.unwrapUnshared(), src.unwrapUnshared(), size);
>      }
>  
> +    template<typename T>
> +    static void PodCopy(SharedMem<T*> dest, SharedMem<T*> src, size_t nelem) {

camelCaps for the static method names.

Comment 6

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3832a93e1d70552830979376c2e31dea2ddc8c34
Bug 1211432 - provide PodCopy and PodMove safe-when-racy operations.  r=waldo

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3832a93e1d70