Normative: throw whenever creating TA from OOB TA

[jugglinmike]

No description provided.

[syg]

It is an invariant, unfortunately unstated, that lengths on ABs should only be read after a detached check, so it's not right to read the length via IsIntegerIndexedObjectOutOfBounds before doing a detached check.

The right fix here is to also modify the CloneArrayBuffer AO to call IsIntegerIndexedObjectOutOfBounds after it checks IsDetachedBuffer.

[jugglinmike]

It is an invariant, unfortunately unstated, that lengths on ABs should only be read after a detached check, so it's not right to read the length via IsIntegerIndexedObjectOutOfBounds before doing a detached check.

Ah, I didn't know about that. Step 4 of InitializeTypedArrayFromTypedArray reads:

4. If IsDetachedBuffer(srcData) is true, throw a TypeError exception.

Does that not satisfy the invariant? Should we remove the corresponding "out of bounds" check from SetTypedArrayFromTypedArray?

The right fix here is to also modify the CloneArrayBuffer AO to call IsIntegerIndexedObjectOutOfBounds after it checks IsDetachedBuffer.

That crossed my mind, but CloneArrayBuffer is currently agnostic of any particular view of the buffer. I think fully embracing this change would involve rewriting the signature to accept just the TypedArray instance and the "byteLength getter" closure since they make the existing parameters redundant. That's a little more drastic than simply adding a couple parameters, but then again, there are only two call sites in all of ECMA262. Does it sound correct to you?

[jugglinmike]

Oh, looks like CloneArrayBuffer will need to keep the cloneConstructor parameter

[syg]

Ah, I didn't know about that. Step 4 of InitializeTypedArrayFromTypedArray reads:

4. If IsDetachedBuffer(srcData) is true, throw a TypeError exception.

Does that not satisfy the invariant? Should we remove the corresponding "out of bounds" check from SetTypedArrayFromTypedArray?

Unfortunately no, because step 15.a, the species construction, can call arbitrary user code which could detach or resize the source buffer, thus the need for a second IsDetached check (and second, missed OOB check).

That crossed my mind, but CloneArrayBuffer is currently agnostic of any particular view of the buffer. I think fully embracing this change would involve rewriting the signature to accept just the TypedArray instance and the "byteLength getter" closure since they make the existing parameters redundant. That's a little more drastic than simply adding a couple parameters, but then again, there are only two call sites in all of ECMA262. Does it sound correct to you?

Yes, I think you are correct. The idea is to put IsDetached and OOB checks immediately before the buffer needs to be accessed, after all other calls that may end up executing arbitrary user code have already been made.

Let me know if you're up for doing this PR, much appreciated if so.

[syg]

I wonder if we should just combine the detachedness and OOB checks into a single AO so we don't miss anything. Need to think through if that has adverse implications.

[jugglinmike]

Let me know if you're up for doing this PR, much appreciated if so.

I'm up for it! I'll push modifications to this pull request unless you'd prefer a new one.

Unfortunately no, because step 15.a, the species construction, can call arbitrary user code which could detach or resize the source buffer, thus the need for a second IsDetached check (and second, missed OOB check).

Oh, good point. We should definitely have tests in Test262 which detach the buffer in the species constructor. I almost missed that!

It is an invariant, unfortunately unstated, that lengths on ABs should only be read after a detached check, so it's not right to read the length via IsIntegerIndexedObjectOutOfBounds before doing a detached check.

Why is that an unstated invariant rather than a formally-enforced requirement via a normative step in ArrayBufferByteLength or MakeIdempotentArrayBufferByteLengthGetter?

[jugglinmike]

Unfortunately no, because step 15.a, the species construction, can call arbitrary user code which could detach or resize the source buffer, thus the need for a second IsDetached check (and second, missed OOB check).

Oh, good point. We should definitely have tests in Test262 which detach the buffer in the species constructor. I almost missed that!

I spoke too soon--we already have tests for that:

• test262/test/built-ins/TypedArrayConstructors/ctors/typedarray-arg/detached-when-species-retrieved-different-type.js
• test262/test/built-ins/TypedArrayConstructors/ctors/typedarray-arg/detached-when-species-retrieved-same-type.js

Nice work as usual, @anba :)

[jugglinmike]

@syg Pinging in case you missed my previous comment. The answer to that question may help me make the change you have in mind.

[syg]

Thanks for the ping.

Why is that an unstated invariant rather than a formally-enforced requirement via a normative step in ArrayBufferByteLength or MakeIdempotentArrayBufferByteLengthGetter?

You mean why doesn't ArrayBufferByteLength do its own detach checks? Because we also want to minimize the number of detach checks. You only want to do a single detach check after each "might have called arbitrary user code" point, including the beginning.

You can argue that it's fine to just do checks excessively in the spec, since it can be coalesced by the implementation if it's not observable. That's true, but I prefer the editorial direction of minimizing the number of detach checks and asserting when we expect a non-detached AB. Mainly, I don't want steps that can throw to be obviously omittable. Even if ArrayBufferByteLength did its own detach check, I'd want judicious uses of ! and ? to make it clear what points can actually throw due to detach. Also, a weaker argument for the current editorial style is that implementations tend to not play around with trying to optimize away detach checks, at least outside of the optimizing compiler, because getting that wrong would be a bad security bug. I'd rather not make the spec unnecessarily slow and invite implementers from thinking about optimizing those away even in the baseline implementation.

Does this give you enough info to make progress?

[jugglinmike]

Yes! I'm relieved to read your rationale because although I'm not an implementer, I appreciate the policy. And speaking as a test maintainer specifically, limiting uneccessary branches makes reasoning about coverage far easier.

[jugglinmike]

Relocating buffer attachedness checks inside the out-of-bounds operations is a little more nuanced than I expected, so I opened a separate pull request to demonstrate that: #66

[jugglinmike]

Superseded by gh-72.