-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Fix osproc so that it doesn't close pipe/process/thread handles twice #16385
Conversation
|
||
# You may NOT close outputStream and errorStream. | ||
assert p.outStream == nil or FileHandleStream(p.outStream).handle != INVALID_HANDLE_VALUE | ||
assert p.errStream == nil or FileHandleStream(p.errStream).handle != INVALID_HANDLE_VALUE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doAssert
because is an important check, and add a descriptive error message to it IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be doAssert
.
As far as I know, these asserts fails only when close(p: Process)
was called or outputStream/errorStream was closed before close(p: Process)
is called.
These errors can be detected if assert was on.
If no asserts failed when assert was on, these error should never happen in -d:danger build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these error should never happen in -d:danger build.
they could, because a different code path can be executed based on runtime conditions that would only be run in production. Generally, there's no point in skipping some assert if the cost of the check is dwarfed by the cost of the proc it lives in, which is the case here (the syscall would dwarf a simple check)
If possible, please provide ways to close I/O handles from user code, as it is needed for some use cases (like |
If you gonna tweak a |
That would be a refactoring, for another time where we patched the stdlib to use destructors... :-) |
Thank you for fixing this :) |
In osproc's Posix code, it seems closing Stream before calling This PR fix the problem in windows implementation in the same way and you can close input stream safely before closing process. |
raiseOSError(osLastError()) | ||
|
||
proc fileClose[T: Handle | FileHandle](h: var T) {.inline.} = | ||
if h > 4: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@demotomohiro pre-existing but where does this magic h > 4
comes from? shouldn't that at least be h > 2 ?
eg assuming stdin=0,stdout=1,stderr=2, what about files with fileno equal to 3 or 4 ?
at very least we need a comment explanining where this comes from
…nim-lang#16385) [backport] * Add error check to closeHandle and fix closing handle twice in osproc * Fix compile error on Linux
…nim-lang#16385) [backport] * Add error check to closeHandle and fix closing handle twice in osproc * Fix compile error on Linux
I added error check code to
closeHandle
call used to implement osproc procedures for windows.And I got invalid handle error because pipe handles and process/thread handles are closed twice when
execCmdEx
proc in osproc module is called.This PR adds error check code to
closeHandle
, doesn't close handles twice and set INVALID_HANDLE_VALUE or 0 after the handle is closed so that finding handle use after close is easier.According to this blog post, invalid handle value of a file handle and a process/thread handle is different.
https://devblogs.microsoft.com/oldnewthing/20040302-00/?p=40443