New issue
Advanced search Search tips

Issue 885255 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 20
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

vsh does not handle EOF on stdin properly

Project Member Reported by jkardatzke@chromium.org, Sep 18

Issue description

When an EOF is sent on stdin over a vsh connection, the vsh_client is just ignoring the EOF.

As a test, just run this (after replacing owner_id) from a Chrome OS command line:

echo foo | vsh --vm_name=termina --target_container=penguin --owner_id=533dd0c6a9276e2629fa1eac6a5ccbb8eeb24d59 -- tee bar

and it won't return like it should.

This looks like the offending code where it just ignores the EOF read:
https://cs.corp.google.com/chromeos_public/src/platform2/vm_tools/vsh/vsh_client.cc?l=230

 
Summary: vsh does not handle EOF on stdin properly (was: vhs does not handle EOF on stdin properly)
Components: OS>Systems>Containers
Labels: M-71
Status: Started (was: Assigned)
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 20

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/b19ef813adca6ad2ad1af1905fe0e7b5c1603476

commit b19ef813adca6ad2ad1af1905fe0e7b5c1603476
Author: Stephen Barber <smbarber@chromium.org>
Date: Thu Sep 20 12:15:20 2018

vm_tools: vsh: handle EOF from client

Update VshClient to forward EOF from stdin to vshd, and to stop
polling stdin after reaching EOF. VshClient must still remain active
to print output from vshd.

Also update VshForwarder to interpret a zero-length data message as EOF,
and forward that to the target process via the tty driver.

BUG= chromium:885255 
TEST=repro case in bug

Change-Id: If8b0e8449418b13fd82a792ee13d6541f4da8044
Reviewed-on: https://chromium-review.googlesource.com/1232935
Commit-Ready: Stephen Barber <smbarber@chromium.org>
Tested-by: Stephen Barber <smbarber@chromium.org>
Reviewed-by: Jeffrey Kardatzke <jkardatzke@google.com>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>

[modify] https://crrev.com/b19ef813adca6ad2ad1af1905fe0e7b5c1603476/vm_tools/vsh/vsh_forwarder.cc
[modify] https://crrev.com/b19ef813adca6ad2ad1af1905fe0e7b5c1603476/vm_tools/vsh/vsh_client.cc
[modify] https://crrev.com/b19ef813adca6ad2ad1af1905fe0e7b5c1603476/vm_tools/vsh/vsh_client.h

Status: Fixed (was: Started)
Fixed, preparing a new component with this change now.

Both the component and host need to be updated before the echo command above will work.

Comment 5 by cylee@chromium.org, Jan 19 (3 days ago)

Cc: cywang@chromium.org
I'm not sure whether it's still the issue. But I encountered some problem while trying to fix the issue in 
https://cs.corp.google.com/chromeos_public/src/platform/tast-tests/src/chromiumos/tast/local/vm/container.go?type=cs&q=PushFile+file:%5Esrc/platform/tast-tests/src/chromiumos/tast/+package:%5Echromeos_public$&g=0&l=214

The original code doesn't work well because I need to copy a file whose size exceeds shell ARG_MAX. I tried to feed the payload to vsh via stdin, but the following code hang forever. It looks vsh couldn't get data from stdin or EOF is not handled correctly.

        args := []string{"--vm_name=" + c.VM.name,
                "--target_container=" + c.containerName,
                "--owner_id=" + c.VM.Concierge.ownerID,
                "--", "sh", "-c", "base64 --decode >"+containerPath}
        cmd := testexec.CommandContext(ctx, "vsh", args...)
        cmd.Stdin = bytes.NewBufferString(base64Data)
        testing.ContextLog(ctx, "Before running cmd.Run()")
        err = cmd.Run()
        if err != nil {
                cmd.DumpLog(ctx)
                return err
        }
        testing.ContextLog(ctx, "After running cmd.Run()")

However, doing it on the host via shell works
$ base64 /usr/local/bin/sysbench | vsh --vm_name=termina --target_container=penguin --owner_id=2c5859c2a92932288ce902f6c2a0746270229178 -- sh -c 'base64 --decode >/home/testuser/sysbench'

Here's a patch to reproduce the problem:
https://chromium-review.googlesource.com/c/chromiumos/platform/tast-tests/+/1423958

Sign in to add a comment