Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions cmds/dutctl/rpc.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
package main

import (
"bufio"
"context"
"errors"
"fmt"
Expand All @@ -17,6 +16,7 @@ import (

"connectrpc.com/connect"
"github.com/BlindspotSoftware/dutctl/internal/output"
"golang.org/x/sys/unix"
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

The use of golang.org/x/sys/unix package makes this code Unix-specific (Linux, macOS, BSD, etc.) and will fail to compile on Windows. The setRawInput function uses Unix-specific terminal ioctl calls (TCGETS, TCSETS) that are not available on Windows. If cross-platform support is a goal, consider adding build tags (e.g., //go:build unix) to restrict this file to Unix systems, and provide a Windows-specific implementation or gracefully handle the lack of raw mode on Windows. If Windows support is not needed, this is acceptable but should be documented.

Copilot uses AI. Check for mistakes.

pb "github.com/BlindspotSoftware/dutctl/protobuf/gen/dutctl/v1"
)
Expand Down Expand Up @@ -92,10 +92,45 @@ func (app *application) detailsRPC(device, command, keyword string) error {
return nil
}

// setRawInput puts the terminal into raw input mode: disables local echo and
// canonical (line-buffered) mode so each keystroke is available immediately.
// It returns a restore function, or nil if the fd is not a terminal.
func setRawInput(fileDescriptor int) func() {
termios, err := unix.IoctlGetTermios(fileDescriptor, unix.TCGETS)
if err != nil {
return nil
}
Comment thread
llogen marked this conversation as resolved.

old := *termios

termios.Iflag &^= unix.ICRNL
termios.Lflag &^= unix.ECHO | unix.ICANON
termios.Cc[unix.VMIN] = 1
termios.Cc[unix.VTIME] = 0

err = unix.IoctlSetTermios(fileDescriptor, unix.TCSETS, termios)
if err != nil {
return nil
}

return func() {
_ = unix.IoctlSetTermios(fileDescriptor, unix.TCSETS, &old)
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

Terminal state cleanup issue: If setRawInput succeeds but the restore function is deferred, and then an error occurs before the RPC streaming setup completes (e.g., stream.Send fails at line 150), the terminal will be properly restored. However, if the terminal restoration itself fails (line 117), the error is silently ignored. This could leave the user's terminal in an unusable state. Consider logging a warning if terminal restoration fails so users know to manually reset their terminal with 'reset' or 'stty sane'.

Suggested change
_ = unix.IoctlSetTermios(fileDescriptor, unix.TCSETS, &old)
if err := unix.IoctlSetTermios(fileDescriptor, unix.TCSETS, &old); err != nil {
log.Printf("warning: failed to restore terminal settings; you may need to run 'reset' or 'stty sane': %v", err)
}

Copilot uses AI. Check for mistakes.
}
}
Comment thread
llogen marked this conversation as resolved.

//nolint:funlen,cyclop,gocognit
func (app *application) runRPC(device, command string, cmdArgs []string) error {
const numWorkers = 2 // The send and receive worker goroutines

// Set raw input mode: disable local echo and canonical mode so each
// keystroke is sent immediately. Interactive modules like serial rely
// on the remote side to echo input.
if f, ok := app.stdin.(*os.File); ok {
if restore := setRawInput(int(f.Fd())); restore != nil {
defer restore()
Comment thread
llogen marked this conversation as resolved.
}
}
Comment thread
llogen marked this conversation as resolved.

runCtx, cancel := context.WithCancel(context.Background())
defer cancel()

Expand Down Expand Up @@ -230,7 +265,9 @@ func (app *application) runRPC(device, command string, cmdArgs []string) error {
go func() {
defer cancel()

reader := bufio.NewReader(app.stdin)
const stdinBufSize = 256

buf := make([]byte, stdinBufSize)

for {
select {
Expand All @@ -241,7 +278,7 @@ func (app *application) runRPC(device, command string, cmdArgs []string) error {
default:
}

text, err := reader.ReadString('\n')
nRead, err := app.stdin.Read(buf)
if err != nil {
if !errors.Is(err, io.EOF) {
errChan <- fmt.Errorf("reading stdin: %w", err)
Expand All @@ -254,7 +291,7 @@ func (app *application) runRPC(device, command string, cmdArgs []string) error {
Msg: &pb.RunRequest_Console{
Console: &pb.Console{
Data: &pb.Console_Stdin{
Stdin: []byte(text),
Stdin: buf[:nRead],
Comment on lines 281 to +294
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

Potential bug: The buffer is reused across iterations, but buf[:nRead] is sent directly in the protobuf message. If the protobuf library doesn't copy the data immediately, this could cause data corruption when the buffer is overwritten by the next Read call. The serial module correctly copies the data (lines 160-161 in serial.go), but this code directly slices the buffer. Verify that the protobuf Send operation copies the data, or allocate a new slice for each send.

Copilot uses AI. Check for mistakes.
},
Comment on lines 291 to 295
Copy link

Copilot AI Feb 18, 2026

Choose a reason for hiding this comment

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

The protobuf message stores buf[:nRead], but buf is reused on the next loop iteration. If the RPC implementation marshals/sends asynchronously, later reads could mutate the slice before it’s fully sent, corrupting stdin data. Safer is to copy the read bytes into a new slice per message before calling stream.Send.

Copilot uses AI. Check for mistakes.
},
},
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ require (
github.com/tarm/serial v0.0.0-20180830185346-98f6abe2eb07
golang.org/x/crypto v0.48.0
golang.org/x/net v0.50.0
golang.org/x/sys v0.41.0
google.golang.org/protobuf v1.36.11
gopkg.in/yaml.v3 v3.0.1
)
Expand All @@ -32,6 +33,5 @@ require (
github.com/olekukonko/tablewriter v1.0.9 // indirect
github.com/rivo/uniseg v0.2.0 // indirect
github.com/rogpeppe/go-internal v1.6.1 // indirect
golang.org/x/sys v0.41.0 // indirect
golang.org/x/text v0.34.0 // indirect
)
2 changes: 1 addition & 1 deletion internal/dutagent/broker.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@
// for module execution and a channel signaling worker termination or errors.
// Multiple calls are idempotent; subsequent calls return the already initialized session and channel.
//
//nolint:ireturn // returning interface module.Session is intentional for abstraction boundary

Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

Start returns module.Session (an interface type) and this repository enables the ireturn linter. Removing the //nolint:ireturn suppression here will likely cause golangci-lint to fail unless Session is added to the ireturn.allow list. Either restore the nolint directive or update the linter config to permit returning module.Session.

Suggested change
//nolint:ireturn // Start intentionally returns module.Session (an interface) as part of the public API.

Copilot uses AI. Check for mistakes.
func (b *Broker) Start(ctx context.Context, s Stream) (module.Session, <-chan error) {

Check failure on line 49 in internal/dutagent/broker.go

View workflow job for this annotation

GitHub Actions / Lint

Start returns interface (github.com/BlindspotSoftware/dutctl/pkg/module.Session) (ireturn)
b.once.Do(func() {
b.init()
b.stream = s
Expand Down
6 changes: 3 additions & 3 deletions internal/dutagent/broker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,8 @@ func TestBroker_StdinForwarding(t *testing.T) {
b := &Broker{}
stdinPayload := []byte("user input")
req := &pb.RunRequest{Msg: &pb.RunRequest_Console{Console: &pb.Console{Data: &pb.Console_Stdin{Stdin: stdinPayload}}}}
stream := &testStream{recvReqs: []*pb.RunRequest{req}, recvErrs: []error{nil}} // after first req, EOF
// No recvErrs: testStream returns reqs in order then EOF by default.
stream := &testStream{recvReqs: []*pb.RunRequest{req}}
ctx, cancel := context.WithCancel(context.Background())
sess, errCh := b.Start(ctx, stream)

Expand All @@ -144,8 +145,7 @@ func TestBroker_StdinForwarding(t *testing.T) {
t.Fatalf("stdin mismatch: got %q want %q", string(data), string(stdinPayload))
}
case <-time.After(200 * time.Millisecond):
// Expected to fail until refactor ensures proper sequencing / closure.
// (Current code may work but channel closure semantics will still fail later.)
t.Fatal("timeout: stdin payload was not forwarded to stdinCh")
}

cancel() // simulate module completion
Expand Down
4 changes: 4 additions & 0 deletions internal/dutagent/worker.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,10 @@ func toClientWorker(ctx context.Context, stream Stream, s *session) error {
//
//nolint:cyclop,funlen,gocognit
func fromClientWorker(ctx context.Context, stream Stream, s *session) error {
// Close stdinCh on exit so ChanReader.Read returns io.EOF and any module
// goroutine blocked on stdin (e.g. the serial inner goroutine) unblocks cleanly.
defer close(s.stdinCh)
Comment on lines +103 to +105
Copy link

Copilot AI Feb 22, 2026

Choose a reason for hiding this comment

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

With raw-mode stdin forwarding, fromClientWorker will now receive and forward very frequent stdin chunks (often single keystrokes). Logging the stdin payload (currently done later in this function) will both flood logs and potentially record sensitive input (e.g., passwords typed into a serial console). Consider removing stdin content logging entirely, or only logging metadata like byte length behind a debug flag.

Copilot uses AI. Check for mistakes.

type recvResult struct {
req *pb.RunRequest
err error
Expand Down
Loading