Hang [@ js_MinimizeDependentStrings] with gczeal, e4x comment

VERIFIED FIXED in mozilla1.9

Status

()

Core
JavaScript Engine
P2
critical
VERIFIED FIXED
11 years ago
3 years ago

People

(Reporter: Jesse Ruderman, Assigned: mrbkap)

Tracking

(Blocks: 1 bug, {hang, testcase})

Trunk
mozilla1.9
x86
Mac OS X
hang, testcase
Points:
---
Bug Flags:
wanted-next +
blocking1.9 -
in-testsuite ?
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
This hangs the JS shell:

gczeal(2);
void <b><x/><!----></b>;

Comment 1

11 years ago
We get 100 frames deep and then hang in the loop on jsstr.c:91

Looking at this now, unless anyone else sees it instantly.
Assignee: general → crowder
(Reporter)

Updated

11 years ago
Severity: normal → critical
(Reporter)

Comment 2

11 years ago
Still happens on trunk.

Brian, did you figure this out?  I'd like to at least know whether it's a GC hazard / potential memory safety bug.
(Reporter)

Updated

11 years ago
Flags: blocking1.9?

Comment 3

11 years ago
No, I didn't make much progress on it and don't have much time to spend on it at the moment.  Someone else should own.
Assignee: crowder → general

Updated

11 years ago
Flags: blocking1.9? → blocking1.9-

Comment 4

11 years ago
I will have a look.
Assignee: general → igor

Comment 5

11 years ago
I can not reproduce the bug on Linux with the current tip. Neither pasting the command directly into the shell or via executing through a file causes any hangs.
(Assignee)

Comment 6

11 years ago
This is sort of a GC hazard. In FoldXMLConstants we pass an unrooted |left| to js_ConcatStrings. |left| in this case is a flat string, so we set |ldep = left| and realloc its buffer. Then, we call js_NewString to create a new string to return. Because of |gczeal(2)|, we GC the string pointed to by |left| and |ldep|. Now, js_NewGCThing runs and returns the first thing on its freelist, which happens to be the string formerly known as |left| and still known as |ldep|. Here's the problem:

(gdb) p str
$53 = (JSString *) 0x41c598
(gdb) p ldep
$54 = (JSString *) 0x41c598

The |JSPREFIX_INIT(ldep, str, ln)| on line 180 in jsstr.c is now setting both |str| and |ldep| to look like dependent strings.
(Assignee)

Comment 7

11 years ago
Created attachment 298132 [details] [diff] [review]
Possible fix

This prevents |ldep| from being collected so that |str| cannot alias it.
Assignee: igor → mrbkap
Status: NEW → ASSIGNED
Attachment #298132 - Flags: review?(igor)

Comment 8

11 years ago
(In reply to comment #7)
> Created an attachment (id=298132) [details]
> Possible fix
> 
> This prevents |ldep| from being collected so that |str| cannot alias it.

I do not think this is a good fix. There is no GC hazard with js_ConcatString unless one supplies an unrooted left or right arguments. Which points to FoldXMLConstants, that concatenates strings without rooting them (which I should have seen long time ago).

That is, FoldXMLConstants does not root accum assuming that weak rooting works. But it does not as TOK_XMLCOMMENT or TOK_XMLCDATA will displace accum from the newborn root and than later call js_ConcatStrings(cx, accuum, str) with the unrooted accum.
Right, js_ConcatStrings should be fast. Its callers are obligated to root inputs.

/be

Updated

10 years ago
Attachment #298132 - Flags: review?(igor)
Blake's in town, this may get fixed sooner.

/be
Flags: wanted-next+
(Assignee)

Comment 11

10 years ago
I don't know where to target this to... Guessing.
Priority: -- → P2
Target Milestone: --- → mozilla1.9beta4
(Assignee)

Comment 12

10 years ago
Created attachment 306789 [details] [diff] [review]
Better fix
Attachment #298132 - Attachment is obsolete: true
Attachment #306789 - Flags: review?(igor)

Updated

10 years ago
Attachment #306789 - Flags: review?(igor) → review+
(Assignee)

Comment 13

10 years ago
Comment on attachment 306789 [details] [diff] [review]
Better fix

This patch fixes a GC hazard.
Attachment #306789 - Flags: approval1.9b4?
Comment on attachment 306789 [details] [diff] [review]
Better fix

We'll take this after beta4.
Attachment #306789 - Flags: approval1.9b4?
Attachment #306789 - Flags: approval1.9b4-
Attachment #306789 - Flags: approval1.9?
(Assignee)

Updated

10 years ago
Target Milestone: mozilla1.9beta4 → mozilla1.9
Comment on attachment 306789 [details] [diff] [review]
Better fix

a1.9=beltzner
Attachment #306789 - Flags: approval1.9? → approval1.9+
(Assignee)

Comment 16

10 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 17

10 years ago
Created attachment 312527 [details]
e4x/extensions/regress-393874.js

Updated

10 years ago
Flags: in-testsuite+
Flags: in-litmus-

Comment 18

10 years ago
v 1.9.0
Status: RESOLVED → VERIFIED

Comment 19

8 years ago
when this bug is opened, the test should be checked in.
Flags: in-testsuite+ → in-testsuite?
Group: core-security
You need to log in before you can comment on or make changes to this bug.