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

Synthesized reflective proxies have wrong This type for hijacked classes #4982

Closed
sjrd opened this issue May 11, 2024 · 0 comments
Closed

Synthesized reflective proxies have wrong This type for hijacked classes #4982

sjrd opened this issue May 11, 2024 · 0 comments
Assignees
Labels
bug Confirmed bug. Needs to be fixed. internal Not visible to users of Scala.js (only by devs in this repo)
Milestone

Comments

@sjrd
Copy link
Member

sjrd commented May 11, 2024

When the base linker's MethodSynthesizer generates reflective proxies in hijacked classes, it creates This nodes with type ClassType(HijackedClass). This is not valid in the IR; they should have the corresponding primitive type. For IR coming from ClassDefs, this is checked by the ClassDefChecker. Synthesized methods, however, are not checked by the ClassDefChecker and therefore bypass the problem.

I was not able to produce a user-visible bug because of this. This seems to be because

  • The only hijacked class for which the tpe of This node matters is Character (in terms of how the Emitter works)
  • Character has a direct implementation of every method that is not declared on Any

Therefore, we never synthesize a reflective proxy in Character, and we never run into the issue in practice.

However, we discovered this issue while working on the WebAssembly backend, for which all hijacked classes need to have This types with the correct type in order to produce well-typed Wasm code. See the workaround here:
https://github.com/tanishiking/scala-wasm/blob/c9b8270e3da1a8f5335b18fc9bc79969e7541ec5/wasm/src/main/scala/org/scalajs/linker/backend/wasmemitter/WasmExpressionBuilder.scala#L1684-L1692

The same problem likely exists in default bridges in theory. In practice, though, none of the hijacked classes inherit from any default method that they don't override. So it never even leads to any invalid default bridge being generated, let alone one that we can exploit to produce a bug.

@sjrd sjrd added bug Confirmed bug. Needs to be fixed. internal Not visible to users of Scala.js (only by devs in this repo) labels May 11, 2024
@sjrd sjrd added this to the v1.16.1 milestone May 11, 2024
@sjrd sjrd self-assigned this May 11, 2024
sjrd added a commit to sjrd/scala-js that referenced this issue May 11, 2024
sjrd added a commit to sjrd/scala-js that referenced this issue May 11, 2024
sjrd added a commit to sjrd/scala-js that referenced this issue May 15, 2024
@gzm0 gzm0 closed this as completed in 69baff1 May 16, 2024
gzm0 added a commit that referenced this issue May 16, 2024
…-type

Fix #4982: Synthesize This nodes with primitive types in hijacked classes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug. Needs to be fixed. internal Not visible to users of Scala.js (only by devs in this repo)
Projects
None yet
Development

No branches or pull requests

1 participant