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

Fix test-driver output parsing crash on Ubuntu #87

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

am-on
Copy link
Contributor

@am-on am-on commented Feb 18, 2025

When running tests on Ubuntu VMs, the test-driver can crash if the executed command calls sudo and writes to stderr. For example:

vm.execute("sudo bash -c \"echo 'Created foo → bar.\n' >&2 && echo 'foo' \"")

sudo spawns a new TTY for stderr on Ubuntu that gets read by the test-driver's output parsing. However, the test-driver only expects base64-encoded stdout and fails when stderr data is read.

  • Normal command (note stderr usees /dev/ttyS0)
> bash -c 'echo $$; ls -l /proc/$$/fd'
lr-x------ 1 root root 64 Feb 18 06:10 0 -> /dev/hvc0
l-wx------ 1 root root 64 Feb 18 06:10 1 -> pipe:[20942]
l-wx------ 1 root root 64 Feb 18 06:10 2 -> /dev/ttyS0
lr-x------ 1 root root 64 Feb 18 06:10 3 -> /proc/664/fd
  • Using sudo (note stderr uses /dev/pts/0)
> sudo bash -c 'echo $$; ls -l /proc/$$/fd
lrwx------ 1 root root 64 Feb 18 06:10 0 -> /dev/pts/0
l-wx------ 1 root root 64 Feb 18 06:10 1 -> pipe:[20943]
lrwx------ 1 root root 64 Feb 18 06:10 2 -> /dev/pts/0
lr-x------ 1 root root 64 Feb 18 06:10 3 -> /proc/670/fd

Because stderr is now read along with the stdout, the test-driver's base64 decoding code crashes on unexpected data:

A minimal reproducible example is provided in
am-on/nix-test-driver-ubuntu-bug.

Related issues:

(Worked on this during Thaiger sprint: Thaigersprint/thaigersprint-2025#1)

When running tests on Ubuntu VMs, the `test-driver` can crash if the
executed command calls `sudo` and writes to `stderr`. For example:

```python
vm.execute("sudo bash -c \"echo 'Created foo → bar.\n' >&2 && echo 'foo' \"")
```

`sudo` spawns a new TTY for `stderr` on Ubuntu that gets read by the
`test-driver`'s output parsing. However, the `test-driver` only expects
base64-encoded `stdout` and fails when `stderr` data is read.

- Normal command (note  `stderr` usees `/dev/ttyS0`)
```
> bash -c 'echo $$; ls -l /proc/$$/fd'
lr-x------ 1 root root 64 Feb 18 06:10 0 -> /dev/hvc0
l-wx------ 1 root root 64 Feb 18 06:10 1 -> pipe:[20942]
l-wx------ 1 root root 64 Feb 18 06:10 2 -> /dev/ttyS0
lr-x------ 1 root root 64 Feb 18 06:10 3 -> /proc/664/fd
```

- Using `sudo` (note `stderr` uses `/dev/pts/0`)
```
> sudo bash -c 'echo $$; ls -l /proc/$$/fd
lrwx------ 1 root root 64 Feb 18 06:10 0 -> /dev/pts/0
l-wx------ 1 root root 64 Feb 18 06:10 1 -> pipe:[20943]
lrwx------ 1 root root 64 Feb 18 06:10 2 -> /dev/pts/0
lr-x------ 1 root root 64 Feb 18 06:10 3 -> /proc/670/fd
```

Because `stderr` is now read along with the `stdout`, the
`test-driver`'s base64 decoding code crashes on unexpected data:
- [encoding step](https://github.com/NixOS/nixpkgs/blob/8a24fbd0f3b4f47f01c897a95b1b65d6a5576c01/nixos/lib/test-driver/src/test_driver/machine.py#L578),
- [receiving the data](https://github.com/NixOS/nixpkgs/blob/8a24fbd0f3b4f47f01c897a95b1b65d6a5576c01/nixos/lib/test-driver/src/test_driver/machine.py#L515),
- [decoding step](https://github.com/NixOS/nixpkgs/blob/8a24fbd0f3b4f47f01c897a95b1b65d6a5576c01/nixos/lib/test-driver/src/test_driver/machine.py#L588).

A minimal reproducible example is provided in
[am-on/nix-test-driver-ubuntu-bug](https://github.com/am-on/nix-test-driver-ubuntu-bug).

Related issues:
- numtide#84
- numtide#5
@am-on
Copy link
Contributor Author

am-on commented Feb 18, 2025

There's a PR that could fix the issue on the test-driver side - NixOS/nixpkgs#382260 but it has a downside of hiding the stderr of the executing command from the machine logs.

Any opinions on which side should the issue be fixed?

Copy link
Member

@picnoir picnoir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

I think this is an improvement over the previous behavior.

Let's merge that as it is and improve upon it if we need to.

@picnoir picnoir merged commit d94797b into numtide:main Feb 19, 2025
30 checks passed
@picnoir
Copy link
Member

picnoir commented Feb 19, 2025

Any opinions on which side should the issue be fixed?

Forgot to respond about that. Ideally, we'd fix that upstream I think.

We'll revert this PR if you manage to fix that upstream.

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 this pull request may close these issues.

2 participants