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 { ^~~~~~~ ```
Created attachment 384132 [details] Patch
Uploaded patch to the wrong bug. :( Apologies.
Created attachment 384148 [details] Patch
Hopefully it is now the right patch. :-)
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);`.
(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.
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?
(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.
Created attachment 384163 [details] Patch
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.
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?
(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.
Created attachment 384596 [details] Patch
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?
(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.
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.
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.
Created attachment 385003 [details] Patch
Created attachment 385004 [details] Patch
(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.
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?
Created attachment 385142 [details] Patch
(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.
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?
(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/
ping for r+/cq+ if there are no further issues?
ping?
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?
(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.
Created attachment 392401 [details] Patch
Comment on attachment 392401 [details] Patch We prefer C++ style casts. reinterpret_cast<char*>(dst)
r=me too, and agree with Alex. Let's use C++ style cast.
Created attachment 392541 [details] Patch
(In reply to Yusuke Suzuki from comment #32) > r=me too, and agree with Alex. Let's use C++ style cast. Thanks. Sure
Comment on attachment 392541 [details] Patch r=me
Comment on attachment 392541 [details] Patch Clearing flags on attachment: 392541 Committed r257908: <https://trac.webkit.org/changeset/257908>
All reviewed patches have been landed. Closing bug.
<rdar://problem/60075573>