pty: add GetPtyFromFile as safer GetPty #86
Merged
Conversation
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
v1.1.0 was released in 2022, and since then there have been some project renames that cause the old version to no longer work, breaking CI: go: github.com/kunalkushwaha/ltag@latest: github.com/kunalkushwaha/[email protected]: parsing go.mod: module declares its path as: github.com/containerd/ltag but was required as: github.com/kunalkushwaha/ltag Signed-off-by: Aleksa Sarai <[email protected]>
In a future patch we will split GetPty() so that users can pass a File as the already-opened pty master, but in order for us to be able to use the same File interface as our other external APIs we need to migrate our internal handlers to use File rather than *os.File. This change has no user-visible impact, as the underlying types used are identical. Signed-off-by: Aleksa Sarai <[email protected]>
Security-conscious callers may wish to have more control over opening /dev/ptmx, and so GetPty is not suitable for them. Previously it was not possible for such users to open /dev/ptmx and then use this package to create new ptys and manage the console. On Linux, the intended usage would be for a daemon to get an O_PATH handle to /dev/ptmx that can then be re-opened (through procfs) to get a new handle to /dev/ptmx. I suspect on FreeBSD you could use O_EMPTY_PATH to accomplish something similar. Signed-off-by: Aleksa Sarai <[email protected]>
AkihiroSuda
reviewed
May 20, 2025
| @@ -31,7 +30,7 @@ const ( | |||
|
|
|||
| // unlockpt unlocks the slave pseudoterminal device corresponding to the master pseudoterminal referred to by f. | |||
| // unlockpt should be called before opening the slave side of a pty. | |||
| func unlockpt(f *os.File) error { | |||
| func unlockpt(f File) error { | |||
There was a problem hiding this comment.
Wondering if we can rather drop the File interface and just use *os.File, assuming that it is the only implementation of the interface
There was a problem hiding this comment.
hmm it was introduced due to the GC stuff
There was a problem hiding this comment.
Yeah this is an remarkably common issue to run into with *os.File. It's a shame there isn't a properly supported way of doing this other than hacking around with runtime.SetFinalizer which is (imho) incredibly dodgy.
To be fair, even Rust only really solved this issue in 1.63 with OwnedFd and BorrowedFd...
AkihiroSuda
approved these changes
May 20, 2025
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.
Security-conscious callers may wish to have more control over opening
/dev/ptmx, and so GetPty is not suitable for them. Previously it was not
possible for such users to open /dev/ptmx and then use this package to
create new ptys and manage the console.
On Linux, the intended usage would be for a daemon to get an O_PATH
handle to /dev/ptmx that can then be re-opened (through procfs) to get a
new handle to /dev/ptmx. I suspect on FreeBSD you could use O_EMPTY_PATH
to accomplish something similar.
Signed-off-by: Aleksa Sarai [email protected]