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

win,errors: remap ERROR_ACCESS_DENIED to UV_EACCES #3193

Conversation

RaisinTen
Copy link
Contributor

If we try to use uv_fs_rmdir on a read-only directory on Windows, it
internally calls _wrmdir, which sets _doserrno to ERROR_ACCESS_DENIED
and errno to EACCES. However, ERROR_ACCESS_DENIED is mapped to UV_EPERM,
so I believe it should be remapped to UV_EACCES.

Signed-off-by: Darshan Sen raisinten@gmail.com

@RaisinTen
Copy link
Contributor Author

Also, question: Why are we using _doserrno error values instead of using errno values directly if we are mapping them to errno error values prefixed with a UV_?

@stale
Copy link

stale bot commented Jun 22, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jun 22, 2021
@RaisinTen RaisinTen force-pushed the win,errors/remap-ERROR_ACCESS_DENIED-to-UV_EACCES branch from a73f273 to c857a5f Compare October 22, 2021 14:49
@stale stale bot removed the stale label Oct 22, 2021
If we try to use uv_fs_rmdir on a read-only directory on Windows, it
internally calls _wrmdir, which sets _doserrno to ERROR_ACCESS_DENIED
and errno to EACCES. However, ERROR_ACCESS_DENIED is mapped to UV_EPERM,
so I believe it should be remapped to UV_EACCES.

Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen force-pushed the win,errors/remap-ERROR_ACCESS_DENIED-to-UV_EACCES branch from c857a5f to 35f72bc Compare October 22, 2021 14:57
@RaisinTen
Copy link
Contributor Author

cc @vtjnash can this get a review plz? CI is green

@vtjnash
Copy link
Member

vtjnash commented Oct 26, 2021

Would this end up changing anything else, such as uv_chmod (which calls NtSetInformationFile)?

@RaisinTen
Copy link
Contributor Author

Would this end up changing anything else, such as uv_chmod (which calls NtSetInformationFile)?

@vtjnash I don't know of a way to cause an EPERM or an EACCES using chmod on Windows but yea, this would probably fix few other error codes as well.

@vtjnash
Copy link
Member

vtjnash commented Oct 30, 2021

For example, what result do you get if you try to chmod C:\\Windows?

@RaisinTen
Copy link
Contributor Author

For example, what result do you get if you try to chmod C:\\Windows?

@vtjnash I tried running https://github.com/RaisinTen/tests/blob/3abee9e7a2344e450eaa1715dd89718a31abde66/test.js and it seems to work with no errors: https://github.com/RaisinTen/tests/runs/4060722205?check_suite_focus=true

@stale
Copy link

stale bot commented Jan 9, 2022

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.

@stale stale bot added the stale label Jan 9, 2022
@vtjnash vtjnash merged commit 04a35ef into libuv:v1.x Feb 13, 2022
@RaisinTen RaisinTen deleted the win,errors/remap-ERROR_ACCESS_DENIED-to-UV_EACCES branch February 13, 2022 05:34
RaisinTen added a commit to RaisinTen/libuv that referenced this pull request Mar 17, 2022
Although the change remapped the error code to the correct one, a lot of
code already depends on the incorrect one, so it's not worth the
breakage.

This reverts commit 04a35ef.

Refs: nodejs/node#42340
Signed-off-by: Darshan Sen <raisinten@gmail.com>
bnoordhuis pushed a commit that referenced this pull request Mar 20, 2022
…3565)

Although the change remapped the error code to the correct one, a lot of
code already depends on the incorrect one, so it's not worth the
breakage.

This reverts commit 04a35ef.

Refs: nodejs/node#42340
Signed-off-by: Darshan Sen <raisinten@gmail.com>
JeffroMF pushed a commit to JeffroMF/libuv that referenced this pull request May 16, 2022
If we try to use uv_fs_rmdir on a read-only directory on Windows, it
internally calls _wrmdir, which sets _doserrno to ERROR_ACCESS_DENIED
and errno to EACCES. However, ERROR_ACCESS_DENIED is mapped to
UV_EPERM, so I believe it should be remapped to UV_EACCES.
JeffroMF pushed a commit to JeffroMF/libuv that referenced this pull request May 16, 2022
…" (libuv#3565)

Although the change remapped the error code to the correct one, a lot of
code already depends on the incorrect one, so it's not worth the
breakage.

This reverts commit 04a35ef.

Refs: nodejs/node#42340
Signed-off-by: Darshan Sen <raisinten@gmail.com>
vtjnash added a commit to JuliaLang/libuv that referenced this pull request Aug 3, 2022
…ibuv#3193)" (libuv#3565)"

This reverts commit a6ba1d7.

This revert was supposed to happen only on libuv v1.x, not libuv master.
vtjnash added a commit to vtjnash/libuv that referenced this pull request May 23, 2023
…ibuv#3193)" (libuv#3565)"

This reverts commit a6ba1d7.

This revert was supposed to happen only on libuv v1.x, not libuv master.
vtjnash added a commit to vtjnash/libuv that referenced this pull request May 23, 2023
This reverts commit a6ba1d7 (libuv#3565)
This revert was supposed to happen only on libuv v1.x, not libuv master.
vtjnash added a commit to vtjnash/libuv that referenced this pull request May 23, 2023
…ibuv#3193)" (libuv#3565)"

This reverts commit a6ba1d7.

This revert was supposed to happen only on libuv v1.x, not libuv master.
vtjnash added a commit that referenced this pull request Jun 20, 2023
…4018)

This reverts commit a6ba1d7 (#3565)
This revert was supposed to happen only on libuv v1.x, not libuv master.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants