Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normative: IsDetachedBuffer should be checked before accessing [[ArrayLength]] #2207

Merged

Conversation

rkirsling
Copy link
Member

It's probably wrong to call this editorial, but it's a fix for an oversight in #2164.

Integer-indexed [[DefineOwnProperty]] is the only place where IsValidIntegerIndex is checked without being immediately preceded by an IsDetachedBuffer check, and I believe this to be a mistake.

The issue is that IsValidIntegerIndex checks [[ArrayLength]] of the typed array itself, which is unaffected by calling DetachArrayBuffer. This seems very silly and may warrant a more significant refactor, but the immediate fix is to avoid calling IsValidIntegerIndex on a typed array with a detached buffer.

Copy link
Contributor

@bakkot bakkot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Every other call to IsValidIntegerIndex is preceded by this call, so it makes sense to do it here as well. That said, it would perhaps make sense to move the IsDetachedBuffer check into IsValidIntegerIndex, so as to avoid this problem.

@bakkot bakkot added the editor call to be discussed in the next editor call label Oct 17, 2020
@rkirsling rkirsling changed the title Editorial: IsDetachedBuffer should precede IsValidIntegerIndex Editorial: IsDetachedBuffer should be checked before accessing [[ArrayLength]] Oct 17, 2020
@rkirsling rkirsling changed the title Editorial: IsDetachedBuffer should be checked before accessing [[ArrayLength]] IsDetachedBuffer should be checked before accessing [[ArrayLength]] Oct 17, 2020
@rkirsling
Copy link
Member Author

Added a commit to address #2164 (comment) and temporarily removed Editorial: from the title (feel free to re-add if appropriate).

@rkirsling rkirsling force-pushed the isdetachedbuffer-before-isvalidintegerindex branch from 6d1e652 to f744988 Compare October 17, 2020 23:56
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, these both look right to me.

@ljharb ljharb self-assigned this Oct 20, 2020
@ljharb ljharb added editorial change spec bug normative change Affects behavior required to correctly evaluate some ECMAScript source text and removed editor call to be discussed in the next editor call editorial change labels Oct 28, 2020
@bakkot
Copy link
Contributor

bakkot commented Oct 28, 2020

Per editor call: let's call this normative, but we'll land it without consensus on the belief that it was intended to be a part of #2164. @rkirsling we also think it makes sense to move move the IsDetachedBuffer calls which precede each call to IsValidIntegerIndex into IsValidIntegerIndex itself; do you want to do this or shall I take care of it?

@rkirsling
Copy link
Member Author

Done. Thanks for your patience. 🙇

@rkirsling rkirsling changed the title IsDetachedBuffer should be checked before accessing [[ArrayLength]] Normative: IsDetachedBuffer should be checked before accessing [[ArrayLength]] Nov 3, 2020
@ljharb ljharb added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label Dec 2, 2020
@ljharb ljharb force-pushed the isdetachedbuffer-before-isvalidintegerindex branch from 864a25d to 7ea0ac9 Compare December 3, 2020 07:01
@ljharb ljharb merged commit 7ea0ac9 into tc39:master Dec 4, 2020
@rkirsling rkirsling deleted the isdetachedbuffer-before-isvalidintegerindex branch December 4, 2020 22:32
jugglinmike added a commit to bocoup/proposal-resizablearraybuffer that referenced this pull request Jun 12, 2021
A recent change to ECMA262 inserted an invocation of IsDetachedBuffer
into the modified algorithm. Update the proposal to accommodate that
change and more faithfully describe the change will which take place
when the proposal is merged.

[1] tc39/ecma262#2207
syg pushed a commit to tc39/proposal-resizablearraybuffer that referenced this pull request Jun 16, 2021
A recent change to ECMA262 inserted an invocation of IsDetachedBuffer
into the modified algorithm. Update the proposal to accommodate that
change and more faithfully describe the change will which take place
when the proposal is merged.

[1] tc39/ecma262#2207
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
normative change Affects behavior required to correctly evaluate some ECMAScript source text ready to merge Editors believe this PR needs no further reviews, and is ready to land. spec bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants