Skip to content

bpo-38030: Fix os.stat failures on block devices on Windows #15681

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Sep 4, 2019
Merged

bpo-38030: Fix os.stat failures on block devices on Windows #15681

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Lib/test/test_os.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -591,6 +591,14 @@ def test_access_denied(self):
result = os.stat(fname)
self.assertNotEqual(result.st_size, 0)

@unittest.skipUnless(sys.platform == "win32", "Win32 specific tests")
def test_stat_block_device(self):
# bpo-38030: os.stat fails for block devices
# Test a filename like "//./C:"
fname = "//./" + os.path.splitdrive(os.getcwd())[0]
result = os.stat(fname)
self.assertEqual(result.st_mode, stat.S_IFBLK)


class UtimeTests(unittest.TestCase):
def setUp(self):
Expand Down
1 change: 1 addition & 0 deletions Misc/NEWS.d/next/Windows/2019-09-04-14-01-08.bpo-38030._USdtk.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fixes :func:`os.stat` failing for block devices on Windows
19 changes: 13 additions & 6 deletions Modules/posixmodule.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1794,13 +1794,13 @@ win32_xstat_impl(const wchar_t *path, struct _Py_stat_struct *result,
case ERROR_INVALID_PARAMETER:
case ERROR_INVALID_FUNCTION:
case ERROR_NOT_SUPPORTED:
retval = -1;
/* Volumes and physical disks are block devices, e.g.
\\.\C: and \\.\PhysicalDrive0. */
memset(result, 0, sizeof(*result));
result->st_mode = 0x6000; /* S_IFBLK */
goto cleanup;
}
/* Volumes and physical disks are block devices, e.g.
\\.\C: and \\.\PhysicalDrive0. */
memset(result, 0, sizeof(*result));
result->st_mode = 0x6000; /* S_IFBLK */
retval = -1;
goto cleanup;
}
}
Expand All @@ -1827,7 +1827,14 @@ win32_xstat_impl(const wchar_t *path, struct _Py_stat_struct *result,

cleanup:
if (hFile != INVALID_HANDLE_VALUE) {
CloseHandle(hFile);
/* Preserve last error if we are failing */
error = retval ? GetLastError() : 0;
if (!CloseHandle(hFile)) {
retval = -1;
} else if (retval) {
Copy link
Contributor

@eryksun eryksun Sep 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Have you had a problem with this? The normal path for the internal implementation of CloseHandle is to call status = NtClose(handle). If the return status is successful, it just returns TRUE. Otherwise Windows sets the last error from the status code (i.e. RtlNtStatusToDosError, etc) and returns FALSE. It's not going to modify our thread's last error value if it succeeds.
Copy link
Member Author

@zooba zooba Sep 4, 2019

Choose a reason for hiding this comment

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

No, just being overly defensive. There are other functions that (incorrectly) change the last error, and it's easier to be consistent about preserving it than to rely on a not-strictly-followed convention.
Copy link
Member Author

@zooba zooba Sep 4, 2019

Choose a reason for hiding this comment

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

In particular, for others who come in and copy my code without necessarily checking severe edge cases like this :)
/* Restore last error */
SetLastError(error);
}
}

return retval;
Expand Down