Closed
Bug 1409285
Opened 7 years ago
Closed 7 years ago
Avoid using memcpy on HeapSlot that is not trivially copyable
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: andi, Assigned: andi)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
Using memcpy in this context has undefined behavior since HeapSlot is not a a trivial-copyable object:
>> } else {
>> memcpy(&elements_[dstStart], src, count * sizeof(HeapSlot));
>> elementsRangeWriteBarrierPost(dstStart, count);
>> }
Assignee | ||
Comment 1•7 years ago
|
||
A note on this matter, this has been found trying to build fx with gcc8 and with Werror:
/usr/bin/g++-8 -std=gnu++11 -o RegExp.o -c -I/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/system_wrappers -include /root/firefox-gcc-last/config/gcc_hidden.h -DDEBUG=1 -DENABLE_BINARYDATA -DENABLE_SIMD -DENABLE_WASM_THREAD_OPS -DWASM_HUGE_MEMORY -DJS_CACHEIR_SPEW -DENABLE_SHARED_ARRAY_BUFFER -DEXPORT_JS_API -DJS_HAS_CTYPES '-DDLL_PREFIX="lib"' '-DDLL_SUFFIX=".so"' -DFFI_BUILDING -DMOZ_HAS_MOZGLUE -I/root/firefox-gcc-last/js/src -I/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/js/src -I/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/js/src/ctypes/libffi/include -I/root/firefox-gcc-last/js/src/ctypes/libffi/src/x86 -I/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include -I/root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/dist/include/nspr -fPIC -DMOZILLA_CLIENT -include /root/firefox-gcc-last/obj-x86_64-pc-linux-gnu/js/src/js-confdefs.h -MD -MP -MF .deps/RegExp.o.pp -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wno-invalid-offsetof -Wc++14-compat -Wc++1z-compat -Wduplicated-cond -Wimplicit-fallthrough -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-error=free-nonheap-object -Wformat -Wformat-overflow=2 -Wno-noexcept-type -fno-rtti -fno-exceptions -fno-math-errno -pthread -pipe -g -freorder-blocks -O3 -fno-omit-frame-pointer -Werror -Wno-shadow -Werror=format -fno-strict-aliasing /root/firefox-gcc-last/js/src/builtin/RegExp.cpp
In file included from /root/firefox-gcc-last/js/src/builtin/RegExp.cpp:24:0:
/root/firefox-gcc-last/js/src/vm/NativeObject-inl.h: In member function 'void js::NativeObject::copyDenseElements(uint32_t, const JS::Value*, uint32_t)':
/root/firefox-gcc-last/js/src/vm/NativeObject-inl.h:155:67: error: 'void* memcpy(void*, const void*, size_t)' writing to an object of non-trivially copyable type 'class js::HeapSlot'; use copy-assignment or copy-initialization instead [-Werror=class-memaccess]
memcpy(&elements_[dstStart], src, count * sizeof(HeapSlot));
^
In file included from /root/firefox-gcc-last/js/src/jsatom.h:15:0,
from /root/firefox-gcc-last/js/src/vm/Runtime.h:22,
from /root/firefox-gcc-last/js/src/jscntxt.h:21,
from /root/firefox-gcc-last/js/src/vm/RegExpObject.h:15,
from /root/firefox-gcc-last/js/src/builtin/RegExp.h:10,
from /root/firefox-gcc-last/js/src/builtin/RegExp.cpp:7:
/root/firefox-gcc-last/js/src/gc/Barrier.h:655:7: note: 'class js::HeapSlot' declared here
class HeapSlot : public WriteBarrieredBase<Value>
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8919200 [details]
Bug 1409285 - Avoid using memcpy on HeapSlot that is not trivially copyable.
https://reviewboard.mozilla.org/r/190114/#review195364
::: js/src/vm/NativeObject-inl.h:260
(Diff revision 1)
> HeapSlot* src = elements_ + srcStart + count - 1;
> for (uint32_t i = 0; i < count; i++, dst--, src--)
> dst->set(this, HeapSlot::Element, dst - elements_ + numShifted, *src);
> }
> } else {
> - memmove(elements_ + dstStart, elements_ + srcStart, count * sizeof(HeapSlot));
> + memmove(reinterpret_cast<Value*>(elements_ + dstStart), elements_ + srcStart,
Should we do the same cast for the second argument?
::: js/src/vm/NativeObject-inl.h:276
(Diff revision 1)
> MOZ_ASSERT(dstStart + count <= getDenseCapacity());
> MOZ_ASSERT(srcStart + count <= getDenseCapacity());
> MOZ_ASSERT(!denseElementsAreCopyOnWrite());
> MOZ_ASSERT(!denseElementsAreFrozen());
>
> - memmove(elements_ + dstStart, elements_ + srcStart, count * sizeof(Value));
> + memmove(reinterpret_cast<Value*>(elements_ + dstStart), elements_ + srcStart,
Same here.
Attachment #8919200 -
Flags: review?(jdemooij) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9e0ca8aa027c
Avoid using memcpy on HeapSlot that is not trivially copyable. r=jandem
Comment 5•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•7 years ago
|
Blocks: class-memaccess
You need to log in
before you can comment on or make changes to this bug.
Description
•