Skip to content

Conversation

kripken
Copy link
Member

@kripken kripken commented Feb 15, 2022

This way we just need to define one syscall instead of two. (Musl will
call the proper syscall based on what it sees is defined.)

@kripken kripken requested a review from sbc100 February 15, 2022 19:20
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Nice!

@kripken kripken merged commit af4a408 into main Feb 15, 2022
@kripken kripken deleted the noaccess branch February 15, 2022 20:54
kripken added a commit that referenced this pull request Feb 15, 2022
After #16296 it is simple to get that test passing:

* Add FS.chmod
* Fix FS.mkdir which did not receive or sent the mode param to wasm
* Test tweaks
@kleisauke
Copy link
Collaborator

I think this PR broke access(2) with absolute Windows paths. See for example this test case on Windows:

#include <unistd.h>
#include <stdio.h>

int main() {
  printf("%d\n", access("C:/existingfile", F_OK));
  
  return 0;
}
# Make a new file named existingfile in C:/
$ type nul > C:/existingfile

# Ensure C:/existingfile does really exist
$ IF EXIST "C:/existingfile" (echo 1) ELSE (echo 0)
1

# faccessat(2) should ignore dirfd when pathname is absolute
# (i.e. it should check the presence of C:/existingfile and not ./C:/existingfile)
$ emcc test.c -o test.js -sNODERAWFS -sEXIT_RUNTIME

# This should print 0, indicating that C:/existingfile does exists
$ node test.js
-1

$ rem C:/existingfile

# This should always print -1, since we deleted C:/existingfile above
$ node test.js
-1

Absolute POSIX paths are already handled here:

if (path[0] === '/') {
return path;
}

So, it needs something similar to Node's Windows variant of path.isAbsolute:
https://nodejs.org/api/path.html#pathisabsolutepath

Perhaps we can just call that utility instead when ENVIRONMENT_IS_NODE? Since this is only a issue with -sNODERAWFS (AFAIK).

@sbc100
Copy link
Collaborator

sbc100 commented Mar 27, 2022

Interesting.. I think maybe we need to create a PATH.isAbs and then have NODERAWFS override various part of the PATH object.

sbc100 added a commit that referenced this pull request Mar 27, 2022
Also, use arrow functions in `library_path.js` to save a little
on code size.

As a followup we can have NODERAWFS replace the isAbs function
(most likely calling out to node's `path.isAbsolute`).

See #16296
@kripken
Copy link
Member Author

kripken commented Mar 28, 2022

PATH.isAbs() sgtm, however I'm not sure why using faccessat() caused this to start failing on windows, so I'm missing the source of the problem here, and am not sure why PATH.isAbs() would fix it...

@sbc100
Copy link
Collaborator

sbc100 commented Mar 28, 2022

PATH.isAbs() sgtm, however I'm not sure why using faccessat() caused this to start failing on windows, so I'm missing the source of the problem here, and am not sure why PATH.isAbs() would fix it...

Because then NODERAWRS under windows could override this method such that c:/foo is also reported as absolute.. not just /foo.

sbc100 added a commit that referenced this pull request Mar 28, 2022
Also, use arrow functions in `library_path.js` to save a little
on code size.

As a followup we can have NODERAWFS replace the isAbs function
(most likely calling out to node's `path.isAbsolute`).

See #16296
@kripken
Copy link
Member Author

kripken commented Mar 28, 2022

But why was such an overriding necessary after this PR (which does not modify JS code in a significant way AFAICT)?

@sbc100
Copy link
Collaborator

sbc100 commented Mar 28, 2022

This change forces more code to through fassessat as opposed to the old faccess.. these means more code hits calculateAt than before which mean that a broken isAboslute check the top of this function effects more code.

@kripken
Copy link
Member Author

kripken commented Mar 28, 2022

Got it, thanks @sbc100 !

kripken pushed a commit that referenced this pull request Nov 7, 2022
Ensures that absolute paths are handled correctly on Windows as well. Using these helpers
should be safe, since binaries linked with -sNODERAWFS can only be used on Node.js, see:

emscripten/src/library_noderawfs.js

Line 28 in e98554f
       throw new Error("NODERAWFS is currently only supported on Node.js environment.") 

Context: #16296
Resolves: #17360.
Resolves: #17819.
Supersedes: #17821.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants