Closed Bug 615970 Opened 9 years ago Closed 8 years ago
Investigate if str->chars() should always clear EXTENSIBLE
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
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...
I will try to come up with a small patch for branches.
(In reply to comment #7) > I will try to come up with a small patch for branches. ping? code freeze for 18.104.22.168 is June 6
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 22.214.171.124 is about a week away (Aug 1).
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)
Comment on attachment 548950 [details] [diff] [review] fix for 192 Approved for 126.96.36.199. Please land today.
Attachment #548950 - Flags: approval188.8.131.52? → approval184.108.40.206+
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.
You need to log in before you can comment on or make changes to this bug.