gh-139184: set O_CLOEXEC for master_fd when call os.forkpty#139408
Conversation
Signed-off-by: Manjusaka <me@manjusaka.me>
Co-authored-by: Shamil <ashm.tech@proton.me>
Signed-off-by: Manjusaka <me@manjusaka.me>
|
@vstinner Thanks for your time. PTAL when you get time |
Signed-off-by: Manjusaka <me@manjusaka.me>
Signed-off-by: Manjusaka <me@manjusaka.me>
|
@vstinner Done |
picnixz
left a comment
There was a problem hiding this comment.
os.* functions are meant to mimic libc ones. As such, I'm not sure we should forcibly change inheritance (or does forkpty actually sets O_CLOEXEC as well?). Instead I would suggest one of the two:
- add an additional parameter to
forkptyto allow settingO_CLOEXECat the same time. - add a separate function (I don't have a name for it) that does it.
| if (pid == -1) { | ||
| return posix_error(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't think you need this change.
There was a problem hiding this comment.
I like this change, it helps readability :-)
There was a problem hiding this comment.
Considering the function is already very compact, I don't think it helps much. But I wouldn't mind having more blank lines in the entire function to really help readability (the return is usually sufficiently clear IMO; I'm more worried about the all those ifs before actually calling forkpty)
There is PEP 446 "Make newly created file descriptors non-inheritable" which requires Python to create new file descriptors as non-inheritable. I see this change as a bugfix, but I prefer to not backport it to avoid bad surprises to users. |
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
…o manjusaka/fix-139184
Signed-off-by: Manjusaka <me@manjusaka.me>
Signed-off-by: Manjusaka <me@manjusaka.me>
Signed-off-by: Manjusaka <me@manjusaka.me>
|
Merged, thank you. |
Uh oh!
There was an error while loading. Please reload this page.