Repository navigation
Non-file file descriptors may be mishandled in ntpath #103924
Description
Activity
- changed the title
[-]Non-file files may be mishandled in ntpath[/-][+]Non-file file descriptors may be mishandled in ntpath[/+]on Apr 27, 2023 Also, a pipe file shouldn't be identified as a regular file, whether the test is by file descriptor or by name. Thus when checking
isfile(),GetFileType(hfile)has to be checked regardless of whether we opened the file. A regular file must be aFILE_TYPE_DISKfile, and itsFileAttributesmust be non-zero (e.g.FILE_ATTRIBUTE_NORMALmust be set if no other attributes are set).
On a related note,
os.Direntry.is_file()is also wrong for pipe files in case ofos.scandir('//./pipe/')oros.scandir('//?/pipe/'). Unfortunately all we have are the file attributes in this case, which don't help us because the named-pipe filesystem properly returnsFILE_ATTRIBUTE_NORMAL. However, it's unlikely that anyone would callos.scandir()on "//./pipe/" and care about the result ofis_file().Hi @zooba,
I’d be interested in investigating this.
From the issue description, it looks like the affected path is the Windows ntpath accelerator handling for file-descriptor inputs, especially where
_Py_get_osfhandle_noraise()can returnINVALID_HANDLE_VALUEwithout a reliable last-error value, and where non-disk handles should be rejected before calling file-information APIs.I’ll first try to reproduce the behavior with invalid descriptors and non-file handles such as pipes/character devices, then check whether the specialized
exists/isfile/isdir/islinkpaths can be fixed with a small targeted change.I’ll report findings before opening a PR.
Hi @zooba,
I investigated this on current
upstream/main.From what I can see, the original non-file descriptor behavior appears mostly addressed now:
- non-disk handles are rejected for
isfile()/isdir()/islink() - invalid handles no longer appear to fall through to pathname fallback logic
- pipes and character devices return
exists=Truebutisfile=False, which matches the existinggenericpath.exists(fd)style behavior
I tested:
- invalid positive fd
- negative fd
- closed fd
- anonymous pipe fd
NULfd- regular file fd
- socket fileno integer
Release build behavior looked correct.
One remaining issue I could still reproduce is narrower: on a Windows debug build,
ntpath.exists(123456)triggers a CRT debug assertion inside_get_osfhandle()before CPython can returnFalse.That path goes through
_Py_get_osfhandle_noraise()inPython/fileutils.c, so I’m hesitant to patch it only inntpathwithout guidance because the helper is shared.Would you prefer this remaining debug assertion case to be handled under this issue, or should it be treated as a separate/narrower bug?
- non-disk handles are rejected for
We can't handle the debug assertion, since it's more important that we pass the FD to the CRT to validate than for us to try and do it ourselves (since we can't). The CRT is designed assuming you're writing an application using it, not a library/runtime, and so an assertion makes sense. We already suppress the fast-exit for both debug and release builds, and the test suite disables assertion popups.
It sounds like there's nothing more to fix here. Thanks for confirming.
Thanks for the clarification, @zooba.
That makes sense. I’ll leave this as resolved and won’t pursue a patch for the debug CRT assertion case.
Thanks!
A couple of bugs slipped by when the new accelerator functions were added.
_Py_get_osfhandle_noraise()fails (e.g. a bad file descriptor), the result isINVALID_HANDLE_VALUE, but the thread's last error isn't set and is thus a random error code. It could be one of the errors that's handled by callingSTAT(), but in this case there is no_path.widevalue to check. It happens thatCreateFileW()doesn't raise an OS exception that crashes the process when passed a null pointer forlpFileName, but that's not documented and shouldn't be relied on.GetFileType(hfile)isn'tFILE_TYPE_DISK, we have to return a false result. Since we didn't open the file, we don't know whether or not it has a pending synchronous operation that's blocked indefinitely. For example, a pipe or character file could have a pending synchronous read that may never complete. In this case,GetFileInformationByHandleEx()would block.Originally posted by @eryksun in #103485 (comment)
This applies to the specialised
isfile/isdir/islink/existsfunctions inntpathwhen passed a file number.