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)

defect
Not set
normal

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); >> }
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 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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: