Closed Bug 994133 Opened 11 years ago Closed 11 years ago

JSValue overloading and crunching into 64bit strikes back - this time in the real world

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Version:
28 Branch
Platform:
Sun
NetBSD
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla38

People

(Reporter: martin, Assigned: martin)

References

Details

Attachments

(1 file, 1 obsolete file)

Make sure allocated memory has the upper 17 bits clear on NetBSD/sparc64 (just like ia64)
2.19 KB, patch
luke
: review+
Details | Diff | Splinter Review
As disscussed #618485, using 64bit JSValue is problematic. When discussing this previously, we overlooked a variant that actually happens (and now causes deterministic crashes at startup on NetBSD): some JSObjects are global variables in shared libraries. An example can be found in js/src/jsobj.cpp: const Class JSObject::class_ = { js_Object_str, JSCLASS_HAS_CACHED_PROTO(JSProto_Object), JS_PropertyStub, /* addProperty */ JS_DeletePropertyStub, /* delProperty */ JS_PropertyStub, /* getProperty */ JS_StrictPropertyStub, /* setProperty */ JS_EnumerateStub, JS_ResolveStub, JS_ConvertStub }; const Class* const js::ObjectClassPtr = &JSObject::class_; Now, with so called "topdown vm" enabled, NetBSD will load the shared libraries at adresses starting with 0xffffff (in sparc64 terms: above the VA hole). The crash then looks like this (with some printf anotations to get around optimized-out variables): Rooted<TaggedProto> proto(this, proto_) (this = 0xfffffffff2738150, proto_ = 0x 7ffff1406040) = 0x7ffff1406040, isObject=1, toObject() = 0x7ffff1406040 Program received signal SIGSEGV, Segmentation fault. [Switching to LWP 1] js::ObjectImpl::setFlag (this=0x7ffff1406040, cx=cx@entry=0xfffffffff2738150, flag_=flag_@entry=8, generateShape=generateShape@entry=js::ObjectImpl::GENERATE_SHAPE) at /usr/pkgobj/www/firefox/work/mozilla-release/js/src/vm/Shape.cpp:1372 1372 if (lastProperty()->getObjectFlags() & flag) (gdb) x/x 0x7ffff1406040 0x7ffff1406040: Cannot access memory at address 0x7ffff1406040 (gdb) p *((JSObject*)0xfffffffff1406040) $1 = {<js::ObjectImpl> = {<js::gc::BarrieredCell<js::ObjectImpl>> = {<js::gc::Ce ll> = {<No data fields>}, <No data fields>}, shape_ = {<js::BarrieredPtr<js::Shape, unsigned long>> = {{ value = 0xfffffffff14021f0, other = 18446744073462096368}}, <No data fields>}, type_ = {<js::BarrieredPtr<js::types::TypeObject, unsigned long>> = {{ value = 0xfffffffff14000e8, other = 18446744073462087912}}, <No data fields>}, slots = 0x0, elements = 0xfffffffffe6aab88, static SLOT_CAPACITY_MIN = 8}, static class_ = {name = 0xfffffffffe69c138 <js_Object_str> "Object", flags = 67108864, addProperty = 0xfffffffffe4e03c0 <JS_PropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, JS::MutableHandle<JS::Value>)>, delProperty = 0xfffffffffe4e03d0 <JS_DeletePropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, bool*)>, getProperty = 0xfffffffffe4e03c0 <JS_PropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, JS::MutableHandle<JS::Value>)>, setProperty = 0xfffffffffe4e03c8 <JS_StrictPropertyStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>, bool, JS::MutableHandle<JS::Value>)>, enumerate = 0xfffffffffe4e03e0 <JS_EnumerateStub(JSContext*, JS::Handle<JSObject*>)>, resolve = 0xfffffffffe4e03e8 <JS_ResolveStub(JSContext*, JS::Handle<JSObject*>, JS::Handle<long>)>, convert = 0xfffffffffe4e04a8 <JS_ConvertStub(JSContext*, JS::Handle<JSObject*>, JSType, JS::MutableHandle<JS::Value>)>, finalize = 0x0, checkAccess = 0x0, call = 0x0, hasInstance = 0x0, construct = 0x0, trace = 0x0, ext = { outerObject = 0x0, innerObject = 0x0, iteratorObject = 0x0, isWrappedNative = false, weakmapKeyDelegateOp = 0x0}, ops = { lookupGeneric = 0x0, lookupProperty = 0x0, lookupElement = 0x0, lookupSpecial = 0x0, defineGeneric = 0x0, defineProperty = 0x0, defineElement = 0x0, defineSpecial = 0x0, getGeneric = 0x0, getProperty = 0x0, getElement = 0x0, getElementIfPresent = 0x0, getSpecial = 0x0, setGeneric = 0x0, setProperty = 0x0, setElement = 0x0, setSpecial = 0x0, getGenericAttributes = 0x0, setGenericAttributes = 0x0, deleteProperty = 0x0, deleteElement = 0x0, deleteSpecial = 0x0, watch = 0x0, unwatch = 0x0, enumerate = 0x0, thisObject = 0x0}, pad = '\000' <repeats 87 times>, static NON_NATIVE = 262144}, static NELEMENTS_LIMIT = 268435456, static MAX_FIXED_SLOTS = 16, static MIN_SPARSE_INDEX = 1000, static SPARSE_DENSITY_RATIO = 8, static ITER_CLASS_NFIXED_SLOTS = 1} As you can see "proto" is a truncated pointer 0x7ffff1406040 which should have been 0xfffffffff1406040. So, what would it take to make JSValue 128 bit on some platforms? This seems to be the only sane solution. There are two workarounds for now: (1) add "sign extension" to the JSValue accessors that convert to a pointer or (2) on NetBSD/sparc64, for now disable topdown VM for firefox (by compiling with -mcmodel=medlow)
I'm a bit confused: the global variable you reference is an object of Class, not JSObject, which should not be pointed to by a JS::Value. In general, I'm not aware of any JSObjects or JSStrings that are stored in static data (some "static strings" used to be statically allocated, but billm fixed that a year or two ago), although perhaps one has snuck in?
Yeah, the example quoted is not the actual value (which I haven't identified yet). If you say there should be none, that is good news - I'll hunt further to see where this one comes from.
Indeed there could be a straggler that crept in, but I'm wondering if gc::MapAlignedPages is just returning chunks above 0x00007fffffffffff. You might want to run a quick test to see if this is happening (js/src/gc/Memory.cpp). It looks like __ia64__ already ran into a problem and has a hack in MapMemory.
Spot on - of course! Basically all 64bit archs != x86 could use this code path (just to be save), unless the OS supports some special mmap() flag to force "low" mapping (or always guarantees this). Testing this now...
Indeed, this fixes the problem for me. Memory layout now looks as expected: 0000000000100000 96K read/exec /usr/pkgobj/www/firefox/work/build/dist/bin/firefox 0000000000216000 8K read/write/exec /usr/pkgobj/www/firefox/work/build/dist/bin/firefox 0000000000218000 8K read/write/exec [ anon ] 000006FFFC400000 1024K read/write [ anon ] 000006FFFC600000 1024K read/write [ anon ] 000006FFFC800000 1024K read/write [ anon ] 000006FFFCA00000 1024K read/write [ anon ] 000006FFFCC00000 1024K read/write [ anon ] 000006FFFCE00000 1024K read/write [ anon ] 000006FFFD000000 1024K read/write [ anon ] 000006FFFD200000 1024K read/write [ anon ] 000006FFFD400000 1024K read/write [ anon ] 000006FFFD600000 1024K read/write [ anon ] 000006FFFD800000 1024K read/write [ anon ] 000006FFFDA00000 1024K read/write [ anon ] [..] 000006FFFFE00000 1024K read/write [ anon ] 0000070000000000 1024K read/write [ anon ] FFFFFFFFE2C00000 1024K read/write [ anon ] FFFFFFFFE3100000 1024K read/write [ anon ] FFFFFFFFE3228000 1888K read /home/martin/.cache/mozilla/firefox/5 pqrkhen.default/startupCache/startupCache.8.big FFFFFFFFE3400000 1024K read/write [ anon ] FFFFFFFFE3500000 1024K read/write [ anon ] FFFFFFFFE3600000 1024K read/write [ anon ] FFFFFFFFE3700000 1024K read/write [ anon ] FFFFFFFFE3800000 1024K read/write [ anon ] FFFFFFFFE3900000 1024K read/write [ anon ] [..] FFFFFFFFEBCE0000 128K read/write [ anon ] FFFFFFFFEBD00000 24K read/exec /usr/pkg/lib/gdk-pixbuf-2.0/2.10.0/lo aders/libpixbufloader-png.so FFFFFFFFEBD06000 1016K /usr/pkg/lib/gdk-pixbuf-2.0/2.10.0/lo aders/libpixbufloader-png.so FFFFFFFFEBE04000 8K read/write/exec /usr/pkg/lib/gdk-pixbuf-2.0/2.10.0/lo aders/libpixbufloader-png.so FFFFFFFFEBF00000 1024K read/write [ anon ] FFFFFFFFEC058000 63128K read /usr/pkg/share/icons/gnome/icon-theme .cache [..] FFFFFFFFF1FE0000 128K read/write [ anon ] FFFFFFFFF2000000 64K read/exec /usr/pkgobj/www/firefox/work/build/browser/components/build/libbrowsercomps.so FFFFFFFFF2010000 1024K /usr/pkgobj/www/firefox/work/build/browser/components/build/libbrowsercomps.so FFFFFFFFF2110000 16K read/write/exec /usr/pkgobj/www/firefox/work/build/browser/components/build/libbrowsercomps.so FFFFFFFFF2200000 40K read/exec /usr/pkgobj/www/firefox/work/build/toolkit/system/gnome/libmozgnome.so FFFFFFFFF220A000 1016K /usr/pkgobj/www/firefox/work/build/toibxfce.so [..] FFFFFFFFFF534000 112K read /usr/pkg/share/mime/mime.cache FFFFFFFFFF552000 8K [ anon ] FFFFFFFFFF554000 128K read/write [ anon ] FFFFFFFFFF574000 96K read / -?- FFFFFFFFFF58C000 464K read / -?- FFFFFFFFFF600000 80K read/exec /libexec/ld.elf_so FFFFFFFFFF614000 1016K [ anon ] FFFFFFFFFF712000 8K read/write [ anon ] FFFFFFFFFF714000 48K read/write [ anon ] FFFFFFFFFF720000 32K read / -?- FFFFFFFFFF728000 32K read / -?- FFFFFFFFFF730000 464K read / -?- FFFFFFFFFF7A4000 208K read/write [ anon ] FFFFFFFFFF7D8000 64K read/write [ uvm_aobj ] FFFFFFFFFF7E8000 80K read/write [ anon ] FFFFFFFFFF7FC000 8K read/exec [ uvm_aobj ] FFFFFFFFFF7FE000 6144K [ stack ] FFFFFFFFFFDFE000 1928K read/write [ stack ] FFFFFFFFFFFE0000 120K read/write [ stack ] total 340664K I can only test this for NetBSD and don't know the VA-layout details of Solaris, FreeBSD and OpenBSD, so the ifdef is pretty explicit (and others could be added later). Should we have an assert in the direct callers to verify the "17 msbits clear" condition at least in debug versions?
Yes. Can you update the patch to include assertions? Looks like just two call sites.
I'm being told that this could work the same on OpenBSD/sparc64, but using 0x0000030000000000 because (quoting) 'sparc64 has a hole in its VA space, and our pmap implementation makes the hole a bit bigger to avoid another page table level - the 0x0000070000000000 might just work though'.
(In reply to Landry Breuil (:gaston) from comment #7) Note that this is only needed if your address space layout uses addresses beyound the VA hole - and if you haven't run into it, you probably should leave the choise to the kernel (i.e. not enable the #ifdef code).
Disregard what i said, i'm being told we shouldnt use that codepath :)
Attachment #8404472 - Flags: review?(luke)
Comment on attachment 8404472 [details] [diff] [review] Use 17-msb-clear hack for NetBSD/sparc64 as well sorry, missed the assertion comment - will update patch
Attachment #8404472 - Flags: review?(luke)
Attachment #8556422 - Flags: review?(luke) → review+
Attachment #8556422 - Flags: checkin?
can we get a try run here, thanks!
Flags: needinfo?(martin)
Keywords: checkin-needed
Comment on attachment 8556422 [details] [diff] [review] Make sure allocated memory has the upper 17 bits clear on NetBSD/sparc64 (just like ia64) This only touches ifdefs that don't affect our builds. We can land it as-is.
Flags: needinfo?(martin)
Attachment #8556422 - Flags: checkin?
https://hg.mozilla.org/mozilla-central/rev/9ea1e520692e
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Hi! We're currently looking at Firefox on Linux/sparc64 and discovered that the problem exists here, too. Unfortunately, the suggested solution does not work on Linux since the hint does not guarantee that consecutive calls to mmap will provide memory in the region above 0x0000070000000000UL: root@deb4g:~# cat vmtest.c #include <stdio.h> #include <sys/mman.h> int main(void) { void *ptr1, *ptr2, *ptr3; ptr1 = mmap(NULL, 1024*1024, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0); ptr2 = mmap((void*)0x0000070000000000UL, 1024*1024, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0); ptr3 = mmap((void*)0x0000070000000000UL, 1024*1024, PROT_READ, MAP_PRIVATE | MAP_ANONYMOUS, 0, 0); printf("%p %p %p\n", ptr1, ptr2, ptr3); } root@deb4g:~# ./vmtest 0xfff800010038e000 0x70000000000 0xfff800010048e000 root@deb4g:~# uname -a Linux deb4g 4.5.0-2-sparc64-smp #1 SMP Debian 4.5.3-1 (2016-05-07) sparc64 GNU/Linux root@deb4g:~# Shall I file a new bug report for this? Thanks, Adrian
Yes please. Cc: me.
(In reply to Jason Orendorff [:jorendorff] from comment #17) > Yes please. Cc: me. I have posted a number of patches now which fix the issue for me, see: > https://bugzilla.mozilla.org/show_bug.cgi?id=1275204 Do you want me to add you as a reviewer?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: