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

ExecStartAndAttach is impossible to use in concurrent environments #25090

Open
james-lawrence opened this issue Jan 22, 2025 · 0 comments
Open
Labels
kind/bug Categorizes issue or PR as related to a bug. remote Problem is in podman-remote

Comments

@james-lawrence
Copy link

james-lawrence commented Jan 22, 2025

Issue Description

due to its mangling of stdout (by setting it into raw mode) and reading from stdin ExecStartAndAttach cannot be called safely concurrently or in code that is controlling stdin/stdout.

the code almost acknowledges the problem with a simple comment:

	// TODO: Make this configurable (can't use streams' InputStream as it's
	// buffered)

Steps to reproduce the issue

run multiple ExecStartAndAttach's within the same process or on a process that is managing stdin/stdout. the terminal will break eventually.

Describe the results you received

terminal periodically gets managled.

Describe the results you expected

I expected output to be passed to my io.Writers/io.Readers to work correctly.

podman info output

latest.

Podman in a container

Yes

Privileged Or Rootless

Rootless

Upstream Latest Release

Yes

Additional environment details

No response

Additional information

implement ExecAttach similar to Container attach resolves the issue. I don't believe podman should necessarily restricted to the improperly generated ExecStartAndAttach options when it results in broken / error prone usage.

func execAttach(ctx context.Context, sessionID string, stdin io.Reader, stdout io.Writer, stderr io.Writer) error {
	isSet := struct {
		stdin  bool
		stdout bool
		stderr bool
	}{
		stdin:  !(stdin == nil || reflect.ValueOf(stdin).IsNil()),
		stdout: !(stdout == nil || reflect.ValueOf(stdout).IsNil()),
		stderr: !(stderr == nil || reflect.ValueOf(stderr).IsNil()),
	}
	// Ensure golang can determine that interfaces are "really" nil
	if !isSet.stdin {
		stdin = (io.Reader)(nil)
	}
	if !isSet.stdout {
		stdout = (io.Writer)(nil)
	}
	if !isSet.stderr {
		stderr = (io.Writer)(nil)
	}

	conn, err := bindings.GetClient(ctx)
	if err != nil {
		return err
	}

	// Unless all requirements are met, don't use "stdin" is a terminal
	file, ok := stdin.(*os.File)
	_, outOk := stdout.(*os.File)
	needTTY := ok && outOk && xterm.IsTerminal(int(file.Fd()))
	if needTTY {
		state, err := xterm.MakeRaw(int(file.Fd()))
		if err != nil {
			return err
		}
		defer func() {
			if err := xterm.Restore(int(file.Fd()), state); err != nil {
				log.Println("unable to restore terminal state", err)
			}
		}()
	}

	body := struct {
		Detach bool   `json:"Detach"`
		TTY    bool   `json:"Tty"`
		Height uint16 `json:"h"`
		Width  uint16 `json:"w"`
	}{
		Detach: false,
		TTY:    needTTY,
	}
	bodyJSON, err := json.Marshal(body)
	if err != nil {
		return err
	}

	var socket net.Conn
	socketSet := false
	dialContext := conn.Client.Transport.(*http.Transport).DialContext
	t := &http.Transport{
		DialContext: func(ctx context.Context, network, address string) (net.Conn, error) {
			c, err := dialContext(ctx, network, address)
			if err != nil {
				return nil, err
			}
			if !socketSet {
				socket = c
				socketSet = true
			}
			return c, err
		},
		IdleConnTimeout: time.Duration(0),
	}
	conn.Client.Transport = t
	// We need to inspect the exec session first to determine whether to use
	// -t.
	resp, err := conn.DoRequest(ctx, bytes.NewReader(bodyJSON), http.MethodPost, "/exec/%s/start", nil, nil, sessionID)
	if err != nil {
		return err
	}
	defer resp.Body.Close()

	if !(resp.IsSuccess() || resp.IsInformational()) {
		defer resp.Body.Close()
		return resp.Process(nil)
	}

	if needTTY {
		winChange := make(chan os.Signal, 1)
		winCtx, winCancel := context.WithCancel(ctx)
		defer winCancel()
		signal.Notify(winChange, syscall.SIGWINCH)
		attachHandleResize(ctx, winCtx, winChange, sessionID, file)
	}

	stdoutChan := make(chan error)
	stdinChan := make(chan error, 1) // stdin channel should not block

	if isSet.stdin {
		go func() {
			_, err := detach.Copy(socket, stdin, []byte{})
			if err != nil && err != define.ErrDetach {
				log.Println("failed to write input to service:", err)
			}
			if err == nil {
				if closeWrite, ok := socket.(containers.CloseWriter); ok {
					if err := closeWrite.CloseWrite(); err != nil {
						debugx.Printf("Failed to close STDIN for writing: %v", err)
					}
				}
			}
			stdinChan <- err
		}()
	}

	buffer := make([]byte, 1024)
	if needTTY {
		go func() {
			// If not multiplex'ed, read from server and write to stdout
			_, err := io.Copy(stdout, socket)
			stdoutChan <- err
		}()

		for {
			select {
			case err := <-stdoutChan:
				if err != nil {
					return err
				}

				return nil
			case err := <-stdinChan:
				if err != nil {
					return err
				}

				return nil
			}
		}
	} else {
		for {
			// Read multiplexed channels and write to appropriate stream
			fd, l, err := containers.DemuxHeader(socket, buffer)
			if err != nil {
				if errors.Is(err, io.EOF) || errors.Is(err, io.ErrUnexpectedEOF) {
					return nil
				}
				return err
			}
			frame, err := containers.DemuxFrame(socket, buffer, l)
			if err != nil {
				return err
			}

			switch {
			case fd == 0:
				if isSet.stdout {
					if _, err := stdout.Write(frame[0:l]); err != nil {
						return err
					}
				}
			case fd == 1:
				if isSet.stdout {
					if _, err := stdout.Write(frame[0:l]); err != nil {
						return err
					}
				}
			case fd == 2:
				if isSet.stderr {
					if _, err := stderr.Write(frame[0:l]); err != nil {
						return err
					}
				}
			case fd == 3:
				return fmt.Errorf("from service from stream: %s", frame)
			default:
				return fmt.Errorf("unrecognized channel '%d' in header, 0-3 supported", fd)
			}
		}
	}
}
@james-lawrence james-lawrence added the kind/bug Categorizes issue or PR as related to a bug. label Jan 22, 2025
@Luap99 Luap99 added the remote Problem is in podman-remote label Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. remote Problem is in podman-remote
Projects
None yet
Development

No branches or pull requests

2 participants