If you make the FixedArrays completely hidden from JS code, then they
should be
safe from tampering, and if the cache index is a compile time smi (e.g.,
constant defined in macros.py), then that parameter won't need checking
either.
I do believe that would remove any of the security concerns I have.
http://codereview.chromium.org/1563005/diff/12001/13008
File src/runtime.cc (right):
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10015
src/runtime.cc:10015: receiver,
That's ok for now. There doesn't seem to be any significant regressions.
Ofcourse, that may just be because there are no non-pathological uses of
String.search.
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10018
src/runtime.cc:10018: &pending_exception);
The reason I'm wary about this is that you are passing the address of a
handle location, but handles can be shared, so if the value is modified
by code (apart from GC, where it should be), it might affect other
references to the handle as well.
In this case, we know that the handle isn't shared (because we created
it from an Object* just above), and it isn't used after this call, so
even if the value is changed, it won't hurt anything.
So I guess the comment I want here is that the key handle isn't shared
and isn't used later.
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10063
src/runtime.cc:10063: }
Probably, it wasn't that clear :);.
I'm considering whether doing a single loop, from 0 to len, without a
special case for finger_index, would be so much simpler than the two
loops that it is worth the overhead of checking against finger_index
again (and not be equal).
However, I was missing that the current approach iterates in
Most-Recently-Used-order, which is ofcourse better, so ignore this
suggestion.
http://codereview.chromium.org/1563005/diff/12001/13008#newcode10071
src/runtime.cc:10071: int target_index = (finger_index > kEntriesIndex)
? finger_index - 2
I want LRU order too, but I don't think the current code does that for
the entries added before the cache fills up.
The current code fills up the array in increasing order, but evicts in
decreasing. I.e., if there are four positions, they are filled in the
order 0, 1, 2, 3, and then when it's full, the first to be
evicted/reused is 2, and then continuing with 1, 0, 3, 2.... (at which
point it is doing LRU order).
The LRU element at the time the cache fills up would be the one after
the finger (wrapping to 0).
I'm not sure what you mean by iterating backwards, but if you keep the
current filling strategy and evict the post-finger entry each time, you
will keep cycling in the same order for both partial and full caches.
http://codereview.chromium.org/1563005/diff/12001/13011
File test/mjsunit/fuzz-natives.js (right):
http://codereview.chromium.org/1563005/diff/12001/13011#newcode165
test/mjsunit/fuzz-natives.js:165: "_GetFromCache": true,
We do not assume that the builtins object is secure. A malicious user is
assumed to be able to get a reference to it, and thereby control any
builtin function or variable. In this case, it includes the variable
holding the cache. If that is overwritten by a carefully crafted object
(maybe a heap number, maybe a JSObject with well-designed internal
properties), your code can be made to read any address, and, on a cache
miss, write the address with the result of a user-defined function.
I.e., write any writable address.
I'm pretty sure that would be an "arbitrary code execution" waiting to
happen. :)
With your suggested change, I'm no longer worried about this.
http://codereview.chromium.org/1563005/show
--
v8-dev mailing list
[email protected]
http://groups.google.com/group/v8-dev
To unsubscribe, reply using "remove me" as the subject.
