| Auto-Submit | +1 |
| Commit-Queue | +1 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
first round of comments; I didn't yet read all of this
len(), TypedArrayAccessMode::kWrite);I find it a bit odd that we're passing an accessmode to TypedArraySpeciesCreateByLength. Why... what? We're just creating a TypedArray so we can never fail to write into it? Also what it would mean to do TypedArraySpeciesCreateByLength(..., TypedArrayAccessMode::kRead) vs TypedArraySpeciesCreateByLength(..., TypedArrayAccessMode::kWrite)? (Is there a comment about this somewhere?)
// The only way how this can bailout is because of a detached or out of bounds
// buffer.This is out of date now?
size_t current_len = array_buffer->GetByteLength();Are you reloading the length on purpose? (If yes, please add a comment.) We already have it as `len` above. If you reload, we need to think about what happens if the sandbox attacker changes it inbetween...
TypedArrayAccessMode access_mode);this could also have kWrite as a default param like the code before, wdyt? (I don't have a super strong opinion here)
TypedArrayAccessMode access_mode = TypedArrayAccessMode::kRead);ditto
TypedArrayAccessMode access_mode = TypedArrayAccessMode::kRead);Having kRead as a default access mode sounds a bit scary! Then we might just forget to update a call site which actually does writing.. Is this important to have this way?
BUILTIN(TypedArrayPrototypeIncludes) {Related to the comment about testing (see below), these don't seem to be tested in the test you're adding. Does test262 have tests which ensure that these work for immutable ABs? (Maybe relevant to add them here anyway...)
transitioning macro TypedArraySpeciesCreateMaybeOOB(Can you add a comment explaining why we can have OOB here? I'm confused!
const ab = new ArrayBuffer(10);Resizable arraybuffers can also be made immutable, right? Can they be resized after? Could you make all the tests below also test resizable immutable arraybuffers too?
const immutable = ab.transferToImmutable();could you add a test that a SAB cannot be transferToImmutable()d?
const slicedImmutable = immutable.sliceToImmutable(0, 5);What about calling sliceToImmutable on a non-immutable buffer?
ta.set([1]);Could also use
```assertThrows(() => ta.set([1]), TypeError);```
})();Can you also add tests for all the code paths you're modifying, like, the Atomics stuff doesn't seem to be covered here. (Looks like you can still use atomics with a non-shared ArrayBuffer, so immutability is relevant there.)
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Auto-Submit | +1 |
len(), TypedArrayAccessMode::kWrite);I find it a bit odd that we're passing an accessmode to TypedArraySpeciesCreateByLength. Why... what? We're just creating a TypedArray so we can never fail to write into it? Also what it would mean to do TypedArraySpeciesCreateByLength(..., TypedArrayAccessMode::kRead) vs TypedArraySpeciesCreateByLength(..., TypedArrayAccessMode::kWrite)? (Is there a comment about this somewhere?)
This implementation is following the spec text.
In particular subarray passes kRead.
TypedArrayAccessMode access_mode);this could also have kWrite as a default param like the code before, wdyt? (I don't have a super strong opinion here)
Since this is a function from the spec and the spec has kRead as default I would find that confusing. Thus I opted not having a default.
TypedArrayAccessMode access_mode = TypedArrayAccessMode::kRead);Having kRead as a default access mode sounds a bit scary! Then we might just forget to update a call site which actually does writing.. Is this important to have this way?
It's the default from the spec. But I agree that no default is better.
BUILTIN(TypedArrayPrototypeIncludes) {Related to the comment about testing (see below), these don't seem to be tested in the test you're adding. Does test262 have tests which ensure that these work for immutable ABs? (Maybe relevant to add them here anyway...)
I mean I am not really changing anything here, except passing kRead, which ignores immutability.
Most of this should be covered in test262. But not all the tests are merged upstream.
transitioning macro TypedArraySpeciesCreateMaybeOOB(Can you add a comment explaining why we can have OOB here? I'm confused!
This was always the case. I am just changing the name to reflect what it is actually doing. We are calling `ValidateTypedArray` which had a "TODO verify not OOB" in it. I checked all the callsites and they deal with OOB, but still I decided to give it an appropriate name.
Will add a comment.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
len(), TypedArrayAccessMode::kWrite);Olivier FlückigerI find it a bit odd that we're passing an accessmode to TypedArraySpeciesCreateByLength. Why... what? We're just creating a TypedArray so we can never fail to write into it? Also what it would mean to do TypedArraySpeciesCreateByLength(..., TypedArrayAccessMode::kRead) vs TypedArraySpeciesCreateByLength(..., TypedArrayAccessMode::kWrite)? (Is there a comment about this somewhere?)
This implementation is following the spec text.
In particular subarray passes kRead.
Ah and to your actual question this guards against the constructor being overwritten and returning an immutable array.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
len(), TypedArrayAccessMode::kWrite);Olivier FlückigerI find it a bit odd that we're passing an accessmode to TypedArraySpeciesCreateByLength. Why... what? We're just creating a TypedArray so we can never fail to write into it? Also what it would mean to do TypedArraySpeciesCreateByLength(..., TypedArrayAccessMode::kRead) vs TypedArraySpeciesCreateByLength(..., TypedArrayAccessMode::kWrite)? (Is there a comment about this somewhere?)
Olivier FlückigerThis implementation is following the spec text.
In particular subarray passes kRead.
Ah and to your actual question this guards against the constructor being overwritten and returning an immutable array.
Ah, could you add a test for that? And a comment
Marja HölttäCan you also add tests for all the code paths you're modifying, like, the Atomics stuff doesn't seem to be covered here. (Looks like you can still use atomics with a non-shared ArrayBuffer, so immutability is relevant there.)
Ah, I just now noticed there are two test files added, I guess many of the things are in the other file... pls ignore the comments if so :)
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
// The only way how this can bailout is because of a detached or out of bounds
// buffer.This is out of date now?
Done
size_t current_len = array_buffer->GetByteLength();Are you reloading the length on purpose? (If yes, please add a comment.) We already have it as `len` above. If you reload, we need to think about what happens if the sandbox attacker changes it inbetween...
It can change as a side-effect of getting IntegerValue of start and end. I actually also forgot to re-check if we are detached. Added a scope so we don't accidentally re-use the old len.
TypedArrayAccessMode access_mode) {Olivier Flückigerditto
Done
Marja HölttäCan you also add tests for all the code paths you're modifying, like, the Atomics stuff doesn't seem to be covered here. (Looks like you can still use atomics with a non-shared ArrayBuffer, so immutability is relevant there.)
Ah, I just now noticed there are two test files added, I guess many of the things are in the other file... pls ignore the comments if so :)
yeah, let me merge them and make sure your comments are addresses.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Auto-Submit | +1 |
| Commit-Queue | +1 |
addressed first round of comments. ptal again
len(), TypedArrayAccessMode::kWrite);Olivier FlückigerI find it a bit odd that we're passing an accessmode to TypedArraySpeciesCreateByLength. Why... what? We're just creating a TypedArray so we can never fail to write into it? Also what it would mean to do TypedArraySpeciesCreateByLength(..., TypedArrayAccessMode::kRead) vs TypedArraySpeciesCreateByLength(..., TypedArrayAccessMode::kWrite)? (Is there a comment about this somewhere?)
Olivier FlückigerThis implementation is following the spec text.
In particular subarray passes kRead.
Marja HölttäAh and to your actual question this guards against the constructor being overwritten and returning an immutable array.
Ah, could you add a test for that? And a comment
Done
size_t current_len = array_buffer->GetByteLength();Olivier FlückigerAre you reloading the length on purpose? (If yes, please add a comment.) We already have it as `len` above. If you reload, we need to think about what happens if the sandbox attacker changes it inbetween...
It can change as a side-effect of getting IntegerValue of start and end. I actually also forgot to re-check if we are detached. Added a scope so we don't accidentally re-use the old len.
And I also had to move around some more code since I realized that the spec mandates a different order for the exceptions.
For mutable arrays we (1) throw if the result buffer is smaller than requested, (2) throw if the source got detached, (3) then copy up to end or the current source size.
For immutable ones we (1) know the result buffer is big enough, (2) throw if the source got detached, (3) throw if the source shrunk below end.
size_t current_len = array_buffer->GetByteLength();Olivier FlückigerAre you reloading the length on purpose? (If yes, please add a comment.) We already have it as `len` above. If you reload, we need to think about what happens if the sandbox attacker changes it inbetween...
It can change as a side-effect of getting IntegerValue of start and end. I actually also forgot to re-check if we are detached. Added a scope so we don't accidentally re-use the old len.
Done
TypedArrayAccessMode access_mode = TypedArrayAccessMode::kRead);Olivier Flückigerditto
Done
TypedArrayAccessMode access_mode = TypedArrayAccessMode::kRead);Olivier FlückigerHaving kRead as a default access mode sounds a bit scary! Then we might just forget to update a call site which actually does writing.. Is this important to have this way?
It's the default from the spec. But I agree that no default is better.
Done
const ab = new ArrayBuffer(10);Resizable arraybuffers can also be made immutable, right? Can they be resized after? Could you make all the tests below also test resizable immutable arraybuffers too?
Done
const immutable = ab.transferToImmutable();could you add a test that a SAB cannot be transferToImmutable()d?
Done
const slicedImmutable = immutable.sliceToImmutable(0, 5);What about calling sliceToImmutable on a non-immutable buffer?
Done
ta.set([1]);Could also use
```assertThrows(() => ta.set([1]), TypeError);```
Done
Marja HölttäCan you also add tests for all the code paths you're modifying, like, the Atomics stuff doesn't seem to be covered here. (Looks like you can still use atomics with a non-shared ArrayBuffer, so immutability is relevant there.)
Olivier FlückigerAh, I just now noticed there are two test files added, I guess many of the things are in the other file... pls ignore the comments if so :)
yeah, let me merge them and make sure your comments are addresses.
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
size_t current_len = array_buffer->GetByteLength();Olivier FlückigerAre you reloading the length on purpose? (If yes, please add a comment.) We already have it as `len` above. If you reload, we need to think about what happens if the sandbox attacker changes it inbetween...
Olivier FlückigerIt can change as a side-effect of getting IntegerValue of start and end. I actually also forgot to re-check if we are detached. Added a scope so we don't accidentally re-use the old len.
And I also had to move around some more code since I realized that the spec mandates a different order for the exceptions.
For mutable arrays we (1) throw if the result buffer is smaller than requested, (2) throw if the source got detached, (3) then copy up to end or the current source size.
For immutable ones we (1) know the result buffer is big enough, (2) throw if the source got detached, (3) throw if the source shrunk below end.
SGTM; can you add a test that would fail if you didn't have the correct order of exceptions?
DCHECK_GE(new_array_buffer->GetByteLength(), new_len_size);Is this legit in the sandbox sense? GetByteLength will read data from the sandbox so it can be whatever
DCHECK_IMPLIES(to_immutable, new_len_size <= from_byte_length - first_size);ditto
TypedArrayAccessMode access_mode);this could also have kWrite as a default param like the code before, wdyt? (I don't have a super strong opinion here)
Since this is a function from the spec and the spec has kRead as default I would find that confusing. Thus I opted not having a default.
Acknowledged
It's actually funny how for the spec the "safe" option is the other one :) We don't want to have a spec bug where default is kWrite but some trivial place forgets to pass kRead, but here we don't want to have bugs the other way around.
BUILTIN(TypedArrayPrototypeIncludes) {Olivier FlückigerRelated to the comment about testing (see below), these don't seem to be tested in the test you're adding. Does test262 have tests which ensure that these work for immutable ABs? (Maybe relevant to add them here anyway...)
I mean I am not really changing anything here, except passing kRead, which ignores immutability.
Most of this should be covered in test262. But not all the tests are merged upstream.
It'd be quite easy to have a bug where you're using a function which has a kWrite default, and then forget to pass kRead. We'd only notice that if we test all the 30 or so TypedArray funcs for immutable buffers :/
transitioning macro TypedArraySpeciesCreateMaybeOOB(Can you add a comment explaining why we can have OOB here? I'm confused!
This was always the case. I am just changing the name to reflect what it is actually doing. We are calling `ValidateTypedArray` which had a "TODO verify not OOB" in it. I checked all the callsites and they deal with OOB, but still I decided to give it an appropriate name.
Will add a comment.
Acknowledged
TestImmutableBuffer(() => new ArrayBuffer(10), "Fixed-length ArrayBuffer");
TestImmutableBuffer(() => new ArrayBuffer(10, {maxByteLength: 20}), "Resizable ArrayBuffer");The same tests?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Auto-Submit | +1 |
size_t current_len = array_buffer->GetByteLength();Olivier FlückigerAre you reloading the length on purpose? (If yes, please add a comment.) We already have it as `len` above. If you reload, we need to think about what happens if the sandbox attacker changes it inbetween...
Olivier FlückigerIt can change as a side-effect of getting IntegerValue of start and end. I actually also forgot to re-check if we are detached. Added a scope so we don't accidentally re-use the old len.
Marja HölttäAnd I also had to move around some more code since I realized that the spec mandates a different order for the exceptions.
For mutable arrays we (1) throw if the result buffer is smaller than requested, (2) throw if the source got detached, (3) then copy up to end or the current source size.
For immutable ones we (1) know the result buffer is big enough, (2) throw if the source got detached, (3) throw if the source shrunk below end.
SGTM; can you add a test that would fail if you didn't have the correct order of exceptions?
Done
DCHECK_GE(new_array_buffer->GetByteLength(), new_len_size);Is this legit in the sandbox sense? GetByteLength will read data from the sandbox so it can be whatever
The authorative thing is new_len_size, which at most is as big as a ReadBoundedSizeField. So we won't write more than that into the target buffer.
The dcheck is just here to ensure we cover all the cases above to catch cases where the buffer would legitimately too small (i.e., without sandbox corruption).
DCHECK_IMPLIES(to_immutable, new_len_size <= from_byte_length - first_size);Olivier Flückigerditto
same
BUILTIN(TypedArrayPrototypeIncludes) {Olivier FlückigerRelated to the comment about testing (see below), these don't seem to be tested in the test you're adding. Does test262 have tests which ensure that these work for immutable ABs? (Maybe relevant to add them here anyway...)
Marja HölttäI mean I am not really changing anything here, except passing kRead, which ignores immutability.
Most of this should be covered in test262. But not all the tests are merged upstream.
It'd be quite easy to have a bug where you're using a function which has a kWrite default, and then forget to pass kRead. We'd only notice that if we test all the 30 or so TypedArray funcs for immutable buffers :/
Done
TestImmutableBuffer(() => new ArrayBuffer(10), "Fixed-length ArrayBuffer");
TestImmutableBuffer(() => new ArrayBuffer(10, {maxByteLength: 20}), "Resizable ArrayBuffer");| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
THROW_NEW_ERROR_RETURN_FAILURE(Do you have a test which goes here?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
if (!keyed_mode.IsStore() || !v8_flags.js_immutable_arraybuffer ||
dependencies()->DependOnArrayBufferDetachingProtector()) {When this test doesn't path, we'll just go into the regular non-ArrayBuffer path further down. Are you sure that this is correct for ArrayBuffers?
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Auto-Submit | +1 |
| Commit-Queue | +1 |
THROW_NEW_ERROR_RETURN_FAILURE(Do you have a test which goes here?
yes:
```
99: // Test TypedArray.prototype.map throws TypeError if species creates immutable buffer
...
```
if (!keyed_mode.IsStore() || !v8_flags.js_immutable_arraybuffer ||
dependencies()->DependOnArrayBufferDetachingProtector()) {When this test doesn't path, we'll just go into the regular non-ArrayBuffer path further down. Are you sure that this is correct for ArrayBuffers?
aha, good catch. indeed the code below would be wrong for TA.
turns out the easiest fix is to just implement the check in this case.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
| Commit-Queue | +2 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Commit-Queue | +0 |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
(re-added my LGTM since it got lost)
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
[immutable-arraybuffer] Implement immutable arraybuffer
Implements the TC39 immutable array buffer proposal
https://github.com/tc39/proposal-immutable-arraybuffer.
This CL mostly disables typed array support in the optimizing
compilers. This part still needs to be done.
Drive-By:
* Cleanup error messages for detached vs. out-of-bounds buffers
* Cleanup naming and implementation around ValidateTypedArray.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
📍 Job mac-m1_mini_2020-perf/speedometer-main.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/15fab000b10000
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
📍 Job mac-m1_mini_2020-perf/jetstream-main.crossbench complete.
See results at: https://pinpoint-dot-chromeperf.appspot.com/job/133a8347310000
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |