Bug 204411 – [JSCOnly] 32-bits warning on memset of JSValue

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Caio Lima]

This is current warning:

```
[42/54] Building CXX object Source/JavaScriptCore/CMakeFiles/JavaScriptCore.dir/__/__/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f2e18ffc-17.cpp.o
In file included from ../../Source/JavaScriptCore/heap/Heap.h:31,
                 from ../../Source/JavaScriptCore/heap/DeferGC.h:29,
                 from ../../Source/JavaScriptCore/runtime/ConcurrentJSLock.h:28,
                 from ../../Source/JavaScriptCore/runtime/VM.h:34,
                 from ../../Source/JavaScriptCore/interpreter/CallFrame.h:30,
                 from ../../Source/JavaScriptCore/runtime/ArgList.h:24,
                 from ../../Source/JavaScriptCore/runtime/JSArray.h:23,
                 from ../../Source/JavaScriptCore/bytecode/ArrayAllocationProfile.h:29,
                 from ../../Source/JavaScriptCore/runtime/JSGlobalObject.h:24,
                 from ../../Source/JavaScriptCore/runtime/JSFixedArray.h:28,
                 from ../../Source/JavaScriptCore/runtime/JSFixedArray.cpp:27,
                 from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f2e18ffc-17.cpp:1:
../../Source/JavaScriptCore/heap/GCMemoryOperations.h: In instantiation of ‘void JSC::gcSafeZeroMemory(T*, size_t) [with T = JSC::JSValue; size_t = unsigned int]’:
../../Source/JavaScriptCore/runtime/RegExpMatchesArray.h:97:123:   required from here
../../Source/JavaScriptCore/heap/GCMemoryOperations.h:307:11: warning: ‘void* memset(void*, int, size_t)’ clearing an object of non-trivial type ‘class JSC::JSValue’; use assignment or value-initialization instead [-Wclass-memaccess]
     memset(dst, 0, bytes);
     ~~~~~~^~~~~~~~~~~~~~~
In file included from ../../Source/JavaScriptCore/bytecode/SpeculatedType.h:32,
                 from ../../Source/JavaScriptCore/runtime/IndexingType.h:28,
                 from ../../Source/JavaScriptCore/bytecode/ArrayAllocationProfile.h:28,
                 from ../../Source/JavaScriptCore/runtime/JSGlobalObject.h:24,
                 from ../../Source/JavaScriptCore/runtime/JSFixedArray.h:28,
                 from ../../Source/JavaScriptCore/runtime/JSFixedArray.cpp:27,
                 from DerivedSources/JavaScriptCore/unified-sources/UnifiedSource-f2e18ffc-17.cpp:1:
../../Source/JavaScriptCore/runtime/JSCJSValue.h:136:7: note: ‘class JSC::JSValue’ declared here
 class JSValue {
       ^~~~~~~

```

[Paulo Matos]

Created attachment 384132 [details]
Patch

[Paulo Matos]

Uploaded patch to the wrong bug. :( Apologies.

[Paulo Matos]

Created attachment 384148 [details]
Patch

[Paulo Matos]

Hopefully it is now the right patch. :-)

[Caio Lima]

Comment on attachment 384148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384148&action=review

> Source/JavaScriptCore/heap/GCMemoryOperations.h:309
> +        bitwise_cast<volatile uint32_t*>(dst)[i] = 0;

What about casting `dst` to `void*`? The we would have  `memset(bitwise_cast<void*>(dst), 0, bytes);`.

[Paulo Matos]

(In reply to Caio Lima from comment #5)
> Comment on attachment 384148 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384148&action=review
> 
> > Source/JavaScriptCore/heap/GCMemoryOperations.h:309
> > +        bitwise_cast<volatile uint32_t*>(dst)[i] = 0;
> 
> What about casting `dst` to `void*`? The we would have 
> `memset(bitwise_cast<void*>(dst), 0, bytes);`.

Actually using memset here is always bad because GCC can potentially optimize it away. Instead, we could potentially use std::fill but I thought it would fit better with the code to do it as x86_64 does it.

At the same time, I am still looking at this and wondering if this is correct, because although I haven't seen test failures yet, I think the JSVALUE in 32bits are two 64bits, in which case, we might need to double the count.

[Caio Lima]

Comment on attachment 384148 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384148&action=review

>>> Source/JavaScriptCore/heap/GCMemoryOperations.h:309
>>> +        bitwise_cast<volatile uint32_t*>(dst)[i] = 0;
>> 
>> What about casting `dst` to `void*`? The we would have  `memset(bitwise_cast<void*>(dst), 0, bytes);`.
> 
> Actually using memset here is always bad because GCC can potentially optimize it away. Instead, we could potentially use std::fill but I thought it would fit better with the code to do it as x86_64 does it.
> 
> At the same time, I am still looking at this and wondering if this is correct, because although I haven't seen test failures yet, I think the JSVALUE in 32bits are two 64bits, in which case, we might need to double the count.

sizeof(JSValue) on 32-bits is also 8 bytes (https://github.com/caiolima/webkit/blob/master/Source/JavaScriptCore/runtime/JSCJSValue.h#L344). The difference from JSValue64 is the enconding of values. On 32-bits we use high 32-bits as the tag and the other 32-bits as the payload. One thing that I'm missing here is what's bad about GCC optimizing `memset`? Is it possible to remove this code with DCE or something like that?

[Paulo Matos]

(In reply to Caio Lima from comment #7)
> Comment on attachment 384148 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384148&action=review
> 
> >>> Source/JavaScriptCore/heap/GCMemoryOperations.h:309
> >>> +        bitwise_cast<volatile uint32_t*>(dst)[i] = 0;
> >> 
> >> What about casting `dst` to `void*`? The we would have  `memset(bitwise_cast<void*>(dst), 0, bytes);`.
> > 
> > Actually using memset here is always bad because GCC can potentially optimize it away. Instead, we could potentially use std::fill but I thought it would fit better with the code to do it as x86_64 does it.
> > 
> > At the same time, I am still looking at this and wondering if this is correct, because although I haven't seen test failures yet, I think the JSVALUE in 32bits are two 64bits, in which case, we might need to double the count.
> 
> sizeof(JSValue) on 32-bits is also 8 bytes
> (https://github.com/caiolima/webkit/blob/master/Source/JavaScriptCore/
> runtime/JSCJSValue.h#L344). The difference from JSValue64 is the enconding
> of values. On 32-bits we use high 32-bits as the tag and the other 32-bits
> as the payload. 

I thought that could be the case, which is why I mentioned my patch is probably broken even though no tests were seen to fail.

> One thing that I'm missing here is what's bad about GCC
> optimizing `memset`? Is it possible to remove this code with DCE or
> something like that?

If gcc optimizes memset away, then it won't zero the memory. And this is a security issue. Yes, DCE can in GCC eliminate the memset.

[Paulo Matos]

Created attachment 384163 [details]
Patch

[Mark Lam]

Comment on attachment 384163 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384163&action=review

r=me with fix.

> Source/JavaScriptCore/heap/GCMemoryOperations.h:256
> +    RELEASE_ASSERT(bytes % 8 == 0);

Please remove this RELEASE_ASSERT.  It's not necessarily free and it's redundant with the one at the top of gcSafeZeroMemory().  You can make it a debug ASSERT.

[Saam Barati]

Comment on attachment 384163 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384163&action=review

> Source/JavaScriptCore/heap/GCMemoryOperations.h:-307
> -    memset(dst, 0, bytes);

Why are we no longer calling memset? That’s faster than the loop. Maybe just silence the warning instead?

[Paulo Matos]

(In reply to Saam Barati from comment #11)
> Comment on attachment 384163 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384163&action=review
> 
> > Source/JavaScriptCore/heap/GCMemoryOperations.h:-307
> > -    memset(dst, 0, bytes);
> 
> Why are we no longer calling memset? That’s faster than the loop. Maybe just
> silence the warning instead?

There's a possibility that the compiler might remove the memset call and the zero'ing of memory won't happen.

[Paulo Matos]

Created attachment 384596 [details]
Patch

[Mark Lam]

Comment on attachment 384596 [details]
Patch

Wasn't this already landed previously?  Was it rolled out?  If so, why, and why is it ok to land again?

[Mark Lam]

(In reply to Mark Lam from comment #14)
> Comment on attachment 384596 [details]
> Patch
> 
> Wasn't this already landed previously?  Was it rolled out?  If so, why, and
> why is it ok to land again?

OK, looks like it was never landed.

[Mark Lam]

Comment on attachment 384596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384596&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        Implemented alternative to memset, and refactored solutions.
> +        Fixes warning on 32bit builds.

It's probably worth adding a comment here that a compiler may choose to optimize away the memset (given Saam's question), or elaborate on what the warning is if it is related.

[Mark Lam]

Comment on attachment 384596 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384596&action=review

> Source/JavaScriptCore/heap/GCMemoryOperations.h:311
>          bitwise_cast<volatile uint64_t*>(dst)[i] = 0;

Please also change this #else case to use gcSafeZeroMemoryHelper(dst, bytes) since it is identical code.

[Paulo Matos]

Created attachment 385003 [details]
Patch

[Paulo Matos]

Created attachment 385004 [details]
Patch

[Paulo Matos]

(In reply to Mark Lam from comment #17)
> Comment on attachment 384596 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384596&action=review
> 
> > Source/JavaScriptCore/heap/GCMemoryOperations.h:311
> >          bitwise_cast<volatile uint64_t*>(dst)[i] = 0;
> 
> Please also change this #else case to use gcSafeZeroMemoryHelper(dst, bytes)
> since it is identical code.

Mark, sorry for the delay on this. I hope all concerns have now been taken care of.

[Darin Adler]

Comment on attachment 385004 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=385004&action=review

> Source/JavaScriptCore/heap/GCMemoryOperations.h:277
> +    return;

Seems that using unconditional return like this could eventually lead to unreachable code warnings. Can we use #else instead of return?

[Paulo Matos]

Created attachment 385142 [details]
Patch

[Paulo Matos]

(In reply to Darin Adler from comment #21)
> Comment on attachment 385004 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385004&action=review
> 
> > Source/JavaScriptCore/heap/GCMemoryOperations.h:277
> > +    return;
> 
> Seems that using unconditional return like this could eventually lead to
> unreachable code warnings. Can we use #else instead of return?

Right - well thought out. I have updated the patch.

[Saam Barati]

Comment on attachment 384163 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384163&action=review

>>> Source/JavaScriptCore/heap/GCMemoryOperations.h:-307
>>> -    memset(dst, 0, bytes);
>> 
>> Why are we no longer calling memset? That’s faster than the loop. Maybe just silence the warning instead?
> 
> There's a possibility that the compiler might remove the memset call and the zero'ing of memory won't happen.

How? Why could the compiler delete memset?

[Paulo Matos]

(In reply to Saam Barati from comment #24)
> Comment on attachment 384163 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=384163&action=review
> 
> >>> Source/JavaScriptCore/heap/GCMemoryOperations.h:-307
> >>> -    memset(dst, 0, bytes);
> >> 
> >> Why are we no longer calling memset? That’s faster than the loop. Maybe just silence the warning instead?
> > 
> > There's a possibility that the compiler might remove the memset call and the zero'ing of memory won't happen.
> 
> How? Why could the compiler delete memset?

Saam, sorry for the delay.
See Notes in https://en.cppreference.com/w/c/string/byte/memset:
Quote:
-
memset may be optimized away (under the as-if rules) if the object modified by this function is not accessed again for the rest of its lifetime (e.g. gcc bug 8537). For that reason, this function cannot be used to scrub memory (e.g. to fill an array that stored a password with zeroes). This optimization is prohibited for memset_s: it is guaranteed to perform the memory write. Third-party solutions for that include FreeBSD explicit_bzero or Microsoft SecureZeroMemory.
-

memset should not be used for safe zero'ing, which is why memset_s exists.
I have found this explanation as well: 
https://www.cryptologie.net/article/419/zeroing-memory-compiler-optimizations-and-memset_s/

[Paulo Matos]

ping for r+/cq+ if there are no further issues?

[Paulo Matos]

ping?

[Yusuke Suzuki]

Comment on attachment 384163 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384163&action=review

>>>>> Source/JavaScriptCore/heap/GCMemoryOperations.h:-307
>>>>> -    memset(dst, 0, bytes);
>>>> 
>>>> Why are we no longer calling memset? That’s faster than the loop. Maybe just silence the warning instead?
>>> 
>>> There's a possibility that the compiler might remove the memset call and the zero'ing of memory won't happen.
>> 
>> How? Why could the compiler delete memset?
> 
> Saam, sorry for the delay.
> See Notes in https://en.cppreference.com/w/c/string/byte/memset:
> Quote:
> -
> memset may be optimized away (under the as-if rules) if the object modified by this function is not accessed again for the rest of its lifetime (e.g. gcc bug 8537). For that reason, this function cannot be used to scrub memory (e.g. to fill an array that stored a password with zeroes). This optimization is prohibited for memset_s: it is guaranteed to perform the memory write. Third-party solutions for that include FreeBSD explicit_bzero or Microsoft SecureZeroMemory.
> -
> 
> memset should not be used for safe zero'ing, which is why memset_s exists.
> I have found this explanation as well: 
> https://www.cryptologie.net/article/419/zeroing-memory-compiler-optimizations-and-memset_s/

I think this "memset" issue is typically tight to the security issues. And in this case, this is unrelated.
The issue happens when zeroing the value which can contain security-token etc. But it is not scratched (https://www.usenix.org/system/files/conference/usenixsecurity17/sec17-yang.pdf).
In this case, I think it does not matter. We are not using memset to the security related thing, it is used for GC memory's clearing. And if it is read by GC, which means it is escaped from the allocation, so GCC never removes this memset.
Is my understanding correct?

[Paulo Matos]

(In reply to Yusuke Suzuki from comment #28)
> 
> I think this "memset" issue is typically tight to the security issues. And
> in this case, this is unrelated.
> The issue happens when zeroing the value which can contain security-token
> etc. But it is not scratched
> (https://www.usenix.org/system/files/conference/usenixsecurity17/sec17-yang.
> pdf).
> In this case, I think it does not matter. We are not using memset to the
> security related thing, it is used for GC memory's clearing. And if it is
> read by GC, which means it is escaped from the allocation, so GCC never
> removes this memset.
> Is my understanding correct?


Hi Yusuke,

Thanks for the comments on this. I think you are correct. I was looking at the code and at how DSE deals with this in GCC8 and it looks like it won't be removed. Unsure exactly why but my guess it that GCC might not be able to prove that the address is not read or because it's inside a template. Usually GCC can be quite aggressive in removing stores like this in C code. For example:
https://godbolt.org/z/TYQeqx

But it is certainly true that this is not a security concern for us and as far as I understand this code (although first time looking at GC), it wouldn't be a problem if indeed GCC removes the store.

I will propose a simpler patch where I cast the dst pointer to a (char *) to avoid the error of memsetting a non-POD type.

[Paulo Matos]

Created attachment 392401 [details]
Patch

[Alex Christensen]

Comment on attachment 392401 [details]
Patch

We prefer C++ style casts.
reinterpret_cast<char*>(dst)

[Yusuke Suzuki]

r=me too, and agree with Alex. Let's use C++ style cast.

[Paulo Matos]

Created attachment 392541 [details]
Patch

[Paulo Matos]

(In reply to Yusuke Suzuki from comment #32)
> r=me too, and agree with Alex. Let's use C++ style cast.

Thanks. Sure

[Mark Lam]

Comment on attachment 392541 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 392541 [details]
Patch

Clearing flags on attachment: 392541

Committed r257908: <https://trac.webkit.org/changeset/257908>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/60075573>