emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.64k stars 3.29k forks source link

FS Nodes: Execute bit erroneously checked on read property #1893

Closed jvilk closed 10 years ago

jvilk commented 10 years ago

The problem is here: https://github.com/kripken/emscripten/blob/incoming/src/library_fs.js#L183

I have a node for a file with the octal mode 100644, which should be globally readable. However, the read mask includes the execute bit for some reason, causing the read property on the file's node to be false.

This property is checked in the SDL library before it dynamically loads a (non-preloaded) audio file: https://github.com/kripken/emscripten/blob/incoming/src/library_sdl.js#L1932

Is this a bug, or is there some reason why the Emscripten FS requires the execute bit before it can read a file (perhaps because the execute bit is required to list directory contents)?

inolen commented 10 years ago

It's part of the legacy FS code. That bit for whatever reason was set on any node created with the canRead flag, possibly due to reading directories as you mentioned.

The real solution would be to migrate whatever is depending on the old interfaces over to the new ones (https://github.com/kripken/emscripten/wiki/Filesystem-API).

Are these files being loaded through the built in preload?

jvilk commented 10 years ago

No. My project BrowserFS emulates the Node FS API in the browser, and I created an Emscripten FS object based off of NODEFS for it.

Are you saying that the read property itself is a legacy property, and that the solution is to refactor the code that uses it?

inolen commented 10 years ago

Yup.

try {
    bytes = FS.readFile(filename);
} catch (e) {
    Module.printErr('Couldn\'t find file for: ' + filename);
    return 0;
}

...

var blob = new Blob([bytes], {type: rwops.mimetype});

looks somewhat correct from a quick look.

jvilk commented 10 years ago

OK. Thanks for the pointers. I'll implement, test, and submit a PR.

inolen commented 10 years ago

I believe this is fully resolved with the merged pr.

Thanks for taking the time on this.