Repository navigation
Fix EACCES-on-Windows sharing-violation check comparing args tuple to an int - #612
Open
ApparentlyTejas wants to merge 3 commits into
Open
ApparentlyTejas wants to merge 3 commits into
ApparentlyTejas wants to merge 3 commits into
Conversation
… an int convert_os_errors special-cases EACCES on Windows to report ResourceLocked when the OS error is really ERROR_SHARING_VIOLATION (winerror 32), not a plain permission problem. But it checked getattr(exc_value, "args", None) == 32 -- args is always a tuple (e.g. (13, 'Permission denied')), so this comparison can never be true and the sharing-violation branch is dead code. The actual Windows-specific attribute carrying this code is winerror, not args.
The 2-arg OSError(errno, strerror) form triggers a CPython/Windows legacy path that reinterprets the first int as a winerror and recomputes .errno from it, which was corrupting these tests' intended errno on real Windows CI (caught by AppVeyor). The 3-arg form (errno, strerror, filename) keeps .errno as given.
It asserted PermissionDenied, but FILE_ERRORS[13] is already remapped to FileExpected on real Windows builds (_WINDOWS_PLATFORM is computed once at import time via platform.system(), so mocking sys.platform in the test doesn't affect it) -- a pre-existing, unrelated quirk in this file. That assumption isn't something this PR's fix needs; the sharing-violation test alone already covers the args-vs-winerror bug.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
In
fs/error_tools.py,_ConvertOSErrorsspecial-casesEACCESon Windows to raiseResourceLockedinstead ofPermissionDeniedwhen the underlying error is actuallyERROR_SHARING_VIOLATION(winerror 32), i.e. the file is locked by another process rather than genuinely permission-denied.The check is:
OSError.argsis always a tuple (e.g.(13, 'Permission denied')), so this comparison against the int32can never be true. The sharing-violation branch is unreachable dead code, and everyEACCESon Windows falls through toPermissionDenied, even when it's really a sharing violation.The attribute that actually carries the Windows error code is
winerror, notargs. This PR fixes the check to usegetattr(exc_value, "winerror", None) == 32.Test plan
test_convert_eacces_sharing_violation_on_windows: constructs anOSError(errno.EACCES, ...)withwinerror = 32set, patchessys.platformto"win32", and assertsconvert_os_errorsraisesResourceLocked.test_convert_eacces_plain_permission_denied_on_windows: same setup withoutwinerror, asserts it still raisesPermissionDenied(guards against overcorrecting).