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: Remove @@species from InitializeTypedArrayFromTypedArray #2677

Closed
syg opened this issue Mar 2, 2022 · 7 comments · Fixed by #2719
Closed

Normative: Remove @@species from InitializeTypedArrayFromTypedArray #2677

syg opened this issue Mar 2, 2022 · 7 comments · Fixed by #2719
Labels
has consensus This has committee consensus. has test262 tests

Comments

@syg
Copy link
Contributor

syg commented Mar 2, 2022

When creating a TypedArray (the target) from another TypedArray (the source) in InitializeTypedArrayFromTypedArray, target gets a newly allocated ArrayBuffer whose prototype is the source's buffer's species constructor's prototype, but that species constructor is never called. Also this only happens if the buffer is not a SharedArrayBuffer.

Pretty funny!

Anyway not sure what purpose it serves to only hook up the species prototype but never call the species constructor. AFAICT, in all other uses of SpeciesConstructor, we call Construct on it.

Alas, major engines agree on this comedy:

let construction_count = 0;

class GrossBuffer extends ArrayBuffer {
  constructor() {
    super(...arguments);
    if (construction_count++ == 1) throw "bad";
  }
  get isGross() { return true; };
  static get [Symbol.species]() { print("barf"); return GrossBuffer; }
}

let gross_buf = new GrossBuffer(1024);
let gross_ta = new Uint8Array(gross_buf);
let mystery_ta = new Int8Array(gross_ta);
print(mystery_ta.buffer.__proto__ === GrossBuffer.prototype);
// true
print(mystery_ta.buffer.isGross);
// true
#### jsc
barf
true
true

#### sm
barf
true
true

#### v8
barf
true
true

#### xs
barf
true
true

Still, I want to see we can remove this one weird use.

/cc @marjakh who initially discovered this.

@bakkot
Copy link
Contributor

bakkot commented Mar 2, 2022

A cursory look around finds that use of extends ArrayBuffer on GitHub is almost exclusively restricted to tests of browser engines and exploits, so I'd be surprised if this breaks anything. :shipit:

@ljharb
Copy link
Member

ljharb commented Mar 2, 2022

What about node’s Buffer, polyfilled to run in the browser?

@bakkot
Copy link
Contributor

bakkot commented Mar 2, 2022

What about it?

@bathos
Copy link
Contributor

bathos commented Mar 2, 2022

I’m very in favor of this change. It’s currently a huge ordeal to clone array buffer data in a manner that’s guaranteed not to invoke user code because of this behavior.

@ljharb
Copy link
Member

ljharb commented Mar 2, 2022

@bakkot does it extend ArrayBuffer? I’m on mobile rn so i can’t check; i forget if it’s that or one of the typed array types.

@bakkot
Copy link
Contributor

bakkot commented Mar 2, 2022

It extends UInt8Array; its underlying .buffer is just a plain ArrayBuffer.

@bakkot
Copy link
Contributor

bakkot commented Mar 2, 2022

also found a jit exploit :P

I mentioned exploits! There's actually a couple distinct ones (e.g.).

I feel like "this feature has been used more often for exploits than for its actual intended use" is a pretty good reason to remove a feature.

@ljharb ljharb added the has consensus This has committee consensus. label Mar 28, 2022
syg added a commit to syg/ecma262 that referenced this issue Mar 31, 2022
@ljharb ljharb closed this as completed in e7979fd Apr 13, 2022
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jul 21, 2022
…FromTypedArray. r=mgaudet

Implement the changes from <tc39/ecma262#2677> and
<tc39/ecma262#2719>.

The next patches will perform further clean-ups.

Differential Revision: https://phabricator.services.mozilla.com/D152262
jamienicol pushed a commit to jamienicol/gecko that referenced this issue Jul 27, 2022
…FromTypedArray. r=mgaudet

Implement the changes from <tc39/ecma262#2677> and
<tc39/ecma262#2719>.

The next patches will perform further clean-ups.

Differential Revision: https://phabricator.services.mozilla.com/D152262
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. has test262 tests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants