Closed Bug 615970 Opened 9 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
firefox5 - unaffected
firefox6 - unaffected
firefox7 - unaffected
firefox8 - unaffected
firefox9 - unaffected
firefox10 - unaffected
blocking2.0 --- -
status2.0 --- unaffected
blocking1.9.2 --- .20+
status1.9.2 --- .20-fixed
blocking1.9.1 --- -
status1.9.1 --- wontfix

People

(Reporter: igor, Assigned: igor)

Details

(Whiteboard: [sg:critical (on 1.9.x)] safe on trunk [qa-examined-192][qa-needs-STR])

Attachments

(1 file)

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.
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.
(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.
Igor, Luke, what's the sg rating for this?
Igor identified a potential source of GC hazards on branches, but trunk is safe.
Summary: Investigate if str->chars() shoul always clear EXTENSIBLE → Investigate if str->chars() should always clear EXTENSIBLE
Whiteboard: [sg:critical (on 1.9.x)] safe on trunk
blocking2.0: --- → -
Assignee: general → igor
blocking1.9.1: --- → needed
blocking1.9.2: --- → needed
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...
blocking1.9.1: needed → -
blocking1.9.2: needed → ?
I will try to come up with a small patch for branches.
blocking1.9.2: ? → .18+
(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
blocking1.9.2: .18+ → .19+
Igor, what's the eta for a safe branch patch here?
(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.
Reminder: code-freeze for 1.9.2.20 is about a week away (Aug 1).
Attached patch fix for 192Splinter Review
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 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.
Attachment #548950 - Flags: review?(luke)
Attachment #548950 - Flags: review?(luke) → review+
Attachment #548950 - Flags: approval1.9.2.20?
Comment on attachment 548950 [details] [diff] [review]
fix for 192

Approved for 1.9.2.20. Please land today.
Attachment #548950 - Flags: approval1.9.2.20? → approval1.9.2.20+
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/4ab39a74f474
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Do we have any STR to display this issue pre-fix? There doesn't seem to be much to QA here.
Whiteboard: [sg:critical (on 1.9.x)] safe on trunk → [sg:critical (on 1.9.x)] safe on trunk [qa-examined-192][qa-needs-STR]
(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.
Group: core-security
You need to log in before you can comment on or make changes to this bug.