Opened 4 years ago
Last modified 4 years ago
#5636 new defect
sprintf.js SpiderMonkey Object.keys crash
| Reported by: | elexis | Owned by: | |
|---|---|---|---|
| Priority: | Should Have | Milestone: | Backlog |
| Component: | Build & Packages | Keywords: | |
| Cc: | Patch: | Phab:D2421 |
Description
While writing a lobby patch and applying the following optimization, I observed consistently reproducible segfaults with a SpiderMonkey stacktrace:
diff --git a/binaries/data/mods/public/gui/lobby/lobby.js b/binaries/data/mods/public/gui/lobby/lobby.js
index 66a3de4d14..aaa4df4dbe 100644
--- a/binaries/data/mods/public/gui/lobby/lobby.js
+++ b/binaries/data/mods/public/gui/lobby/lobby.js
@@ -196,9 +196,7 @@ var g_NetMessageTypes = {
},
"leave": msg => {
addChatMessage({
- "text": "/special " + sprintf(translate("%(nick)s has left."), {
- "nick": msg.nick
- }),
+ "text": "/special " + sprintf(translate("%(nick)s has left."), msg),
"time": msg.time,
"isSpecial": true
});
The stacktrace is:
Thread 1 "pyrogenesis" received signal SIGSEGV, Segmentation fault.
js::HeapTypeSetKey::instantiate (this=0x555556c420d0, cx=0x555558d00610) at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-45.0.2/js/src/vm/TypeInference.cpp:1301
1301 if (object()->isSingleton() && !object()->singleton()->getGroup(cx)) {
(gdb) info stack
#0 js::HeapTypeSetKey::instantiate (this=0x555556c420d0, cx=0x555558d00610) at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-45.0.2/js/src/vm/TypeInference.cpp:1301
#1 0x00007ffff7aa4cf4 in (anonymous namespace)::CompilerConstraintInstance<(anonymous namespace)::ConstraintDataFreezeObjectFlags>::generateTypeConstraint (this=0x555556c420c8, cx=0x555558d00610, recompileInfo=...)
at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-45.0.2/js/src/vm/TypeInference.h:1123
#2 0x00007ffff7aa7584 in js::FinishCompilation (cx=cx@entry=0x555558d00610, script=..., script@entry=..., constraints=constraints@entry=0x555558e2ce48, precompileInfo=precompileInfo@entry=0x7fffffffd054,
isValidOut=isValidOut@entry=0x7fffffffd052) at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-45.0.2/js/src/vm/TypeInference.cpp:1404
#3 0x00007ffff766f509 in js::jit::CodeGenerator::link (this=this@entry=0x7fffd4021b50, cx=cx@entry=0x555558d00610, constraints=0x555558e2ce48) at ../../dist/include/js/RootingAPI.h:926
#4 0x00007ffff76cae65 in LinkCodeGen (cx=cx@entry=0x555558d00610, builder=builder@entry=0x555558e2cec0, codegen=codegen@entry=0x7fffd4021b50, scripts=..., scripts@entry=..., info=info@entry=0x7fffffffd2f0)
at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-45.0.2/js/src/jit/IonBuilder.h:1055
#5 0x00007ffff76cb353 in LinkBackgroundCodeGen (info=0x7fffffffd2f0, scripts=..., builder=0x555558e2cec0, cx=0x555558d00610) at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-45.0.2/js/src/jit/Ion.cpp:588
#6 js::jit::LazyLink (cx=0x555558d00610, calleeScript=...) at /home/elexis/code/0ad-svn/trunk/libraries/source/spidermonkey/mozjs-45.0.2/js/src/jit/Ion.cpp:614
#7 0x00007ffff76cb59a in js::jit::LazyLinkTopActivation (cx=0x555558d00610) at ../../dist/include/js/RootingAPI.h:926
Bisecting has shown that the segfault could be triggered this way since r22856 but not before.
After that I tried changing JS::Heap to JS::PersistentRootedValue and used a JS::MutableHandleValue function argument instead of JS::Value as a return value, the crash still reproduced however.
After that I tried reproducing the segfault without sprintf, but I failed to (I suspected a rooting / garbage collection error in that commit).
After that I updated the sprintf library to the most recent file https://github.com/alexei/sprintf.js/blob/master/src/sprintf.js and the crash was gone!
Then I bisected the sprintf versions to find the commit that removed the SpiderMonkey segfault.
It was this:
https://github.com/alexei/sprintf.js/commit/61c795624204883948c0e19f8af208f5359e6fdb#diff-13ed28d46a5f76f4d44561850bda81bb
> Allow access to prototype properties
diff --git a/src/sprintf.js b/src/sprintf.js
index ccb78d8..1ade05d 100644
--- a/src/sprintf.js
+++ b/src/sprintf.js
@@ -41,7 +41,7 @@
if (ph.keys) { // keyword argument
arg = argv[cursor]
for (k = 0; k < ph.keys.length; k++) {
- if (!arg.hasOwnProperty(ph.keys[k])) {
+ if (arg[ph.keys[k]] === undefined) {
throw new Error(sprintf('[sprintf] property "%s" does not exist', ph.keys[k]))
}
arg = arg[ph.keys[k]]
One can apply only this line to the current (as of 23154) svn revision of sprintf.js and the error will become irreproducible too.
With a websearch I found two spidermonkey bugreports about crashes that have the same stacktrace as this one:
https://bugzilla.mozilla.org/show_bug.cgi?id=1234425 Duplicate: https://bugzilla.mozilla.org/show_bug.cgi?id=1238475
The duplicate has a piece of code that allows reproducing this crash on current svn without sprintf:
Object.defineProperty(this, "f", {
get: function() {
[].keys
return (function() {
f();
});
}
});
f();
However the commit that marked these tickets as fixed is applied to this SpiderMonkey version 45.0.2 already, see mozjs-45.0.2.tar.bz2 and https://hg.mozilla.org/mozilla-central/rev/f0f950d9cca5 https://hg.mozilla.org/integration/mozilla-inbound/rev/f0f950d9cca5 https://hg.mozilla.org/releases/mozilla-aurora/rev/dc043de54b77
--- a/js/src/jit/JitFrameIterator.h
+++ b/js/src/jit/JitFrameIterator.h
@@ -169,16 +169,19 @@ class JitFrameIterator
return type_ == JitFrame_IonJS;
}
bool isIonStub() const {
return type_ == JitFrame_IonStub;
}
bool isIonStubMaybeUnwound() const {
return type_ == JitFrame_IonStub || type_ == JitFrame_Unwound_IonStub;
}
+ bool isIonAccessorICMaybeUnwound() const {
+ return type_ == JitFrame_IonAccessorIC || type_ == JitFrame_Unwound_IonAccessorIC;
+ }
bool isBailoutJS() const {
return type_ == JitFrame_Bailout;
}
bool isBaselineStub() const {
return type_ == JitFrame_BaselineStub;
}
bool isBaselineStubMaybeUnwound() const {
return type_ == JitFrame_BaselineStub || type_ == JitFrame_Unwound_BaselineStub;
--- a/js/src/jit/JitFrames.cpp
+++ b/js/src/jit/JitFrames.cpp
@@ -1625,21 +1625,21 @@ GetPcScript(JSContext* cx, JSScript** sc
++it;
// Skip rectifier frames.
if (it.isRectifierMaybeUnwound()) {
++it;
MOZ_ASSERT(it.isBaselineStub() || it.isBaselineJS() || it.isIonJS());
}
- // Skip Baseline or Ion stub frames.
+ // Skip Baseline/Ion stub and accessor IC frames.
if (it.isBaselineStubMaybeUnwound()) {
++it;
MOZ_ASSERT(it.isBaselineJS());
- } else if (it.isIonStubMaybeUnwound()) {
+ } else if (it.isIonStubMaybeUnwound() || it.isIonAccessorICMaybeUnwound()) {
++it;
MOZ_ASSERT(it.isIonJS());
}
MOZ_ASSERT(it.isBaselineJS() || it.isIonJS());
// Don't use the return address if the BaselineFrame has an override pc.
// The override pc is cheap to get, so we won't benefit from the cache,
Notice that this is also marked as a security issue.
If we have the fix, why can we still reproduce the crash with the given crash recipe in the duplicate upstream link with the same stacktrace as claimed fixed?
Also I could reproduce the crash with the upstream recipe in alpha23b with mozjs 38.
Perhaps this is just fixed by updating SpiderMonkey.
Attachments (1)
Change History (4)
by , 4 years ago
| Attachment: | TryPersistentRooted.patch added |
|---|
comment:1 by , 4 years ago
| Patch: | → Phab:D2421 |
|---|
comment:3 by , 4 years ago
With the new sprintf revision, the crash with sprintf can be reproduced with this diff (and and lobby chat sprintf using XmppClient messages, see diff above or Phab:D2412)
Index: binaries/data/mods/mod/globalscripts/sprintf.js
===================================================================
--- binaries/data/mods/mod/globalscripts/sprintf.js (revision 23157)
+++ binaries/data/mods/mod/globalscripts/sprintf.js (working copy)
@@ -72,7 +72,7 @@
if (ph.keys) { // keyword argument
arg = argv[cursor]
for (k = 0; k < ph.keys.length; k++) {
- if (arg == undefined) {
+ if (!arg.hasOwnProperty(ph.keys[k])) {
throw new Error(sprintf('[sprintf] Cannot access property "%s" of undefined value "%s"', ph.keys[k], ph.keys[k-1]))
}
arg = arg[ph.keys[k]]
(As posted above, the crash can be triggered without sprintf too.)
This patch demonstrates that the same bug is also triggered when using PersistentRootedValue instead of JS::HeapValue