Skip to content

Field access doesn't check for null #507

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

Closed
vgrichina opened this issue Feb 22, 2019 · 9 comments
Closed

Field access doesn't check for null #507

vgrichina opened this issue Feb 22, 2019 · 9 comments

Comments

@vgrichina
Copy link

export class Chunk {
  data: Data[];

  constructor() {
    this.data = new Array<Data>(CHUNK_SIZE);
    for (let i = 0; i < CHUNK_SIZE; i++) {
      // this.data[i] = new Data() 
      this.data[i].rgb = START_COLOR; // this is expected to abort
    }
  }
}

I assume it's probably because null is 0 in AssemblyScript and this.data[i].rgb points to some perfectly valid address even when this.data[i] is null.

@vgrichina
Copy link
Author

This can result in somewhat hard to check errors. I understand this can be performance trade-off, but at least should fail when optimizations are disabled.

@dcodeIO
Copy link
Member

dcodeIO commented Feb 22, 2019

What will happen here is that if data[i] is null and let's say the .rgb property is at offset 8 of a Chunk, that the compiler emits a i32.store(null, START_COLOR, 8) effectively storing a value at linear memory offset 8 and corrupting whatever is stored there. The performance trade-off of a runtime check would be one if (<lhs> == null) branch per property access, which would be quite significant.

Of course, by typing it Data[] instead of (Data | null)[], this already implies an assumption that is hard to evaluate statically. I also believe TypeScript can't catch this as well, but I agree that we should investigate whether it can be done, somehow.

Any ideas or experience how other languages do this?

@vgrichina
Copy link
Author

Any ideas or experience how other languages do this?

  1. Native targets usually just have low addresses page fault AFAIK.
  2. I think NULL isn't 0 when you compile C/C++ to WebAssembly. It's very high address instead, so also results in memory violation on access.

I see two options viable for AssemblyScript:

  1. Add explicit null checks which are removed when compiled in optimized mode.
  2. Do thing similar to C/C++, but this has a trade-off that e.g. checking for truthiness of value need to change to treat special NULL case + initial field/var values will have to be NULL.

@MaxGraey
Copy link
Member

MaxGraey commented Feb 22, 2019

Hmm, I tried replicate this with C++/Clang and see how this handle here but Clang generated 0.5 Mb binary size with -Oz and max level LTO O_o:
https://webassembly.studio/?f=km149lduwb7

@MaxGraey
Copy link
Member

MaxGraey commented Feb 22, 2019

@dcodeIO what about define null as -1 instead 0? It should produce "out of bound" exception:
https://webassembly.studio/?f=65o9r2zmfqh

@dcodeIO
Copy link
Member

dcodeIO commented Feb 23, 2019

Oh, yeah, a null = -1 / 0xffffffff seems like a viable solution. Technically also works with offset=X immediates because these don't wrap. Other pointer arithmetic must be avoided ofc, like that would be problematic with memcpy and friends.

@MaxGraey
Copy link
Member

It emit runtime error for now:
https://webassembly.studio/?f=w26x4373978

@vgrichina could you check plz maybe we could close this?

@MaxGraey
Copy link
Member

@dcodeIO could we close this, wdyt?

@dcodeIO
Copy link
Member

dcodeIO commented May 27, 2020

Closing this issue as part of 2020 vacuum because it seems to have been solved meanwhile. In particular, a runtime check is now being performed if the array is potentially "holey", and a convention is established that any such array must be filled before use to avoid the error.

@dcodeIO dcodeIO closed this as completed May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants