Skip to content

Stderr handling in SSH Transport can yield wrong results #74

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.

Sign up for GitHub

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

Jump to bottom
Closed
kadler opened this issue Sep 6, 2022 · 0 comments
Closed

Stderr handling in SSH Transport can yield wrong results #74

kadler opened this issue Sep 6, 2022 · 0 comments
Assignees
@kadler
Labels
bug Something isn't working

Comments

@kadler
Copy link
Member

kadler commented Sep 6, 2022

@jkyeung reported this on Ryvver when itoolkit-utils is not installed. Here's the relevant trace:

***********************
control Fri Aug 26 12:06:16 2022
 ipc(*na) ctl(*here *cdata)
input Fri Aug 26 12:06:16 2022
<?xml version='1.0'?>
<xmlservice><cmd exec="cmd" error="fast" var="cmd"><![CDATA[sndmsg tousr(jyeung) msg('Hello')]]></cmd></xmlservice>

output Fri Aug 26 12:06:16 2022
b'ry\n'
parse (fail) Fri Aug 26 12:06:16 2022
parse step: 3 (1-ok, 2-*BADPARSE, 3-*NOPARSE)

When I print out xml_out we get from the transport, I get a longer string:

b'tory in the path name does not exist.\n'

This is because our handling of reading stdout/stderr is not correct here: https://github.com/IBM/python-itoolkit/blob/main/src/itoolkit/transport/ssh.py#L97-L102

In this case, stdout contains no data. The first iteration of the loop we read the first 64 bytes from stderr, then read from stdout. Since stdout is empty, we close it. Next iteration, we read the next 64 bytes from stderr. Since stdout is closed, we skip reading it, but this leaves newData still set to the stderr data, so this gets assigned to stdout as well.

Dumping out stderr gives a much more sensible output:

b'bash: line 1: /QOpenSys/pkgs/bin/xmlservice-cli: A file or directory in the path name does not exist.\n'
@kadler kadler added the bug Something isn't working label Sep 6, 2022
@kadler kadler self-assigned this Sep 6, 2022
kadler added a commit that referenced this issue Sep 6, 2022
@kadler
fix: Handle stderr properly in SSH transport
a555584 
Instead of reading a small amount of data in a blocking way from the
stdout and stderr streams, enable non-blocking I/O and attempt to read
as much data as possible. In addition, make a convenience function to
handle the stream being closed and timeouts so that we don't need to
store a temp variable in the loop. This prevents the situation that lead
to issue #74 where the stderr data is written to the output XML if
stdout is already closed.

Fixes #74
@kadler kadler mentioned this issue Sep 6, 2022
Closed
kadler added a commit that referenced this issue Sep 6, 2022
@kadler
fix: Handle stderr properly in SSH transport
17460d7 
Instead of reading a small amount of data in a blocking way from the
stdout and stderr streams, enable non-blocking I/O and attempt to read
as much data as possible. In addition, make a convenience function to
handle the stream being closed and timeouts so that we don't need to
store a temp variable in the loop. This prevents the situation that lead
to issue #74 where the stderr data is written to the output XML if
stdout is already closed.

Fixes #74
@kadler kadler closed this as completed in e16b965 Feb 3, 2023
kadler added a commit that referenced this issue Feb 3, 2023
@kadler
fix: Handle stderr properly in SSH transport
dae3db6 
Instead of reading a small amount of data in a blocking way from the
stdout and stderr streams, enable non-blocking I/O and attempt to read
as much data as possible. In addition, make a convenience function to
handle the stream being closed and timeouts so that we don't need to
store a temp variable in the loop. This prevents the situation that lead
to issue #74 where the stderr data is written to the output XML if
stdout is already closed.

Fixes #74
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kadler kadler

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Handle stderr properly in SSH transport IBM/python-itoolkit
1 participant
@kadler