615970 - Investigate if str->chars() should always clear EXTENSIBLE

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

Currently JSString::chars() and similar methods that leaks the char array pointer do not clear the EXTENSIBLE flag assuming that the caller will do it on its own if the char array pointer must stay the same.

But that may lead to a hazard if the caller forgets to clear a flag when a script can be executed before the char array is accessed again. For example, consider the following fragment from array_toString_sub, http://hg.mozilla.org/tracemonkey/file/e5a107d91377/js/src/jsarray.cpp#l1224 :

sepstr->getCharsAndLength(sep, seplen);
...
js_GetLengthProperty(cx, obj, &length)
...
cb.append(sep, seplen)

If a getter executed by js_GetLengthProperty can turn sepstr into a dependent string, then we would get a hazard as chars will point to freed memory which code then proceed to copy into a script visible memory. Note I have not verified that we have a real hazard here, but just want to give an example of code where we can have a problem.

To stop such hazards from realizing I suggest to always clear EXTENSIBLE in JSString::chars() and similar methods. If this turned out to harm some benchmarks, we can consider to add something like JSString::getVolatileChars and use it at places that affect benchmarks.

Comment 1

[Luke Wagner [:luke]]

That's not what EXTENSIBLE means; I described this in the comment at the top of JSString::flatten in attachment 494278 [details] [diff] [review].  EXTENSIBLE just means that the empty space of the buffer can be filled during concatentation.  The buffer is not realloc'd or free'd.  Thus, extensibility is only a problem if code depends on the null character to determine string length (since the null character is overwritten when the string is extended).  That's why the only place we clear extensibility is JS_GetStringCharsZ and not, e.g., JS_GetStringCharsAndLength.

Comment 2

[Igor Bukanov]

(In reply to comment #1)
> That's not what EXTENSIBLE means; I described this in the comment at the top of
> JSString::flatten in attachment 494278 [details] [diff] [review].

I have missed that on trunk js_ConcatStrings never changes the char array pointer. That means that on trunk we do not have hazards. But on branches where the array can be mutated we do get one. So this is branch-only.

Comment 3

[Johnny Stenback (:jst)]

Igor, Luke, what's the sg rating for this?

Comment 4

[Luke Wagner [:luke]]

Igor identified a potential source of GC hazards on branches, but trunk is safe.

Comment 5

[Johnny Stenback (:jst)]

A small regression on 1.9 branches is not a problem here, can we get a patch that fixes this potential security issue? Smaller and safer is better, even if it costs a bit in performance...

Comment 6

[Johnny Stenback (:jst)]

Igor?

Comment 7

[Igor Bukanov]

I will try to come up with a small patch for branches.

Comment 8

[Daniel Veditz [:dveditz]]

(In reply to comment #7)
> I will try to come up with a small patch for branches.

ping? code freeze for 1.9.2.18 is June 6

Comment 9

[Johnny Stenback (:jst)]

Igor, what's the eta for a safe branch patch here?

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> Igor, what's the eta for a safe branch patch here?

Sorry, this drifted out of focus. I am on vacation until 2011-07-19 and will write a status for this bug shortly after that.

Comment 11

[Daniel Veditz [:dveditz]]

Reminder: code-freeze for 1.9.2.20 is about a week away (Aug 1).

Comment 12

[Igor Bukanov]

Attachment: fix for 192

The patch here rearanges the code so the chars pointers is only accessed when no JS code can be executed in the default configuration of the browser.

If some extensions shares objects between threads, then the problem is that any allocation of GC things can potentially trigger a last-ditch GC. That in turn suspends the current request and may trigger object sharing code on another thread. But that calls js_MakeStringImmutable on any string stored in the object mutating the char pointer stored in the string. To fix against that requires rather significant extension of the patch with non-trivial changes.

As extensions with above capabilities are rare and are not supported at all starting from FF 4.0, my suggestion is won't fix for that part.

For now I will monitor the usage of JSString::chars and similar calls one more time and then ask to review the patch.

Comment 13

[Igor Bukanov]

Comment on attachment 548950 [details] [diff] [review]
fix for 192

The patch fixes on the 192 branch the places that I found where JSString::chars() result were used after a code that can execute JS via getters/setters/custom-toString. The remedy is to move JSString::chars() after pitentially-JS-executing calls or call it several times.

Comment 14

[LegNeato]

Comment on attachment 548950 [details] [diff] [review]
fix for 192

Approved for 1.9.2.20. Please land today.

Comment 15

[Igor Bukanov]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4ab39a74f474

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Do we have any STR to display this issue pre-fix? There doesn't seem to be much to QA here.

Comment 17

[Igor Bukanov]

(In reply to Al Billings [:abillings] from comment #16)
> Do we have any STR to display this issue pre-fix?

No - the issue has come up from the code analysis. But it should be possible to come up with regression test case after some work.