Skip to content
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

Write output corruption when spawning pwsh #210

Closed
segevfiner opened this issue Feb 2, 2022 · 4 comments · Fixed by andfoy/winpty-rs#16
Closed

Write output corruption when spawning pwsh #210

segevfiner opened this issue Feb 2, 2022 · 4 comments · Fixed by andfoy/winpty-rs#16

Comments

@segevfiner
Copy link

segevfiner commented Feb 2, 2022

import time
from winpty import PtyProcess

proc = PtyProcess.spawn(["pwsh"])
time.sleep(1)
print(proc.read(1024), end='')
proc.write("e")
time.sleep(1)
print(proc.read(1024), end='')
time.sleep(5)
proc.close(force=True)

(With starship prompt installed)

I'm getting the following:

PowerShell 7.2.1
Copyright (c) Microsoft Corporation.

https://aka.ms/powershell
Type 'help' to get help.

Loading personal and system profiles took 732ms.

…\qt-embedded-terminal on  master [!?] is 📦 v0.1.0 via ⬢ via 🐍 v3.10.2 (venv)
 
❯ e
Display all 131 possibilities? (y or n) _

That Display all 131 possibilities shouldn't be there, it's like someone pressed tab, but at no point in the code did I send a tab character, it seems like the string is corrupted on output, missing null terminator perhaps? Or encoding error?

@segevfiner
Copy link
Author

segevfiner commented Feb 2, 2022

Yep. Right here: https://github.com/andfoy/winpty-rs/blob/3b08c65244630fb5c1b7772b0295f317e92e98b9/src/pty/base.rs#L444-L454

encode_wide does not add a null terminator, but you are calling WideCharToMultiByte with -1 which tells it to treat it as a null terminated string. Undefined Behavior! as the Rust books like to proclaim. 😝

EDIT: But you did manually add a null terminator, so that's not the issue... time to keep digging...

EDIT 2: So actually caused by writing the nul byte through to the PTY. I will send a PR.

@andfoy

@segevfiner
Copy link
Author

segevfiner commented Feb 2, 2022

The fix was merged. Thanks @andfoy. But winpty-rs and this module still need to be released for the fix to be picked up. (Just an FYI in case anyone hits this).

P.S. This module seems to have not committed the Cargo.lock which means it will build with whatever happens to be the latest matching versions of winpty-rs and other libraries. I think this is a case where you should commit the Cargo.lock for reproducible builds (As this is a cdylib case), just remember to update it when needed.

@ccordoba12
Copy link
Contributor

@andfoy is the maintainer of this project as well (as you can see in the commit history).

@segevfiner
Copy link
Author

@andfoy is the maintainer of this project as well (as you can see in the commit history).

Yeah. I know. It's up to him when to make a new release of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants