Code review and fixes.

This commit is contained in:
Storm Dragon committed 2026-11-02 23:37:45 -05:00
1 parent f2a35a9305
commit 60f7f7a2e9
12 files changed
+185 -34

No files matched your search

+19
View File
@@ -59,6 +59,25 @@ selection, staging, and environment forwarding are covered by
not establish compatibility with a real Proton runtime or audible
Wine output.
Helper readiness is published from bounded stderr input before diagnostic
throttling or log delivery, so a diagnostic burst or stalled log sink cannot
discard the exclusion count needed for fallback. Configuration, preferences,
and helper launch use the same executable-name validation, including rejection
of embedded control characters. Regression checks:
```sh
go test ./internal/wine ./internal/config ./internal/preferences -count=1
```
The private Wine preferences check also exercises pasting an embedded tab,
blocking Apply, deleting the invalid row, and applying valid entries. It needs
`xclip` for the private clipboard:
```sh
go build -o /tmp/cthulhu-wine-check ./cmd/cthulhu
CTHULHU_BINARY=/tmp/cthulhu-wine-check CTHULHU_PREFS_WINE_ONLY=1 python3 tests/native_preferences.py
```
Two statements in that parallel document described pre-stage
behaviour and are **not** carried forward: Wine settings no longer
require a reader restart (a global preferences Apply republishes
+9 -1
View File
@@ -33,6 +33,14 @@ import (
// Validate checks the complete configuration without touching the filesystem.
func Validate(settings Settings) error { return validate(settings) }
// ValidWineProgramName reports whether an exclusion is an executable basename
// accepted by settings, preferences, and the helper launch boundary.
func ValidWineProgramName(name string) bool {
return name != "" && len(name) <= 255 && strings.TrimSpace(name) == name &&
!strings.ContainsAny(name, `/\`) &&
!strings.ContainsFunc(name, func(character rune) bool { return character < ' ' })
}
func validate(settings Settings) error {
if len(settings.Wine.ExcludedPrograms) > 256 {
return errors.New("too many Wine hook exclusions")
@@ -40,7 +48,7 @@ func validate(settings Settings) error {
exclusionUnits := 0
for _, name := range settings.Wine.ExcludedPrograms {
exclusionUnits += len(utf16.Encode([]rune(name))) + 1
if name == "" || len(name) > 255 || strings.TrimSpace(name) != name || strings.ContainsAny(name, "\x00\r\n/\\") {
if !ValidWineProgramName(name) {
return errors.New("Wine hook exclusions must be executable basenames")
}
}
+13
View File
@@ -4,6 +4,7 @@
package config
import (
"fmt"
"path/filepath"
"reflect"
"testing"
@@ -39,3 +40,15 @@ func TestWineSettingsRoundTripAndValidation(t *testing.T) {
}
}
}
func TestWineSettingsRejectEmbeddedControlCharacters(t *testing.T) {
for character := rune(0); character < ' '; character++ {
t.Run(fmt.Sprintf("U+%04X", character), func(t *testing.T) {
settings := DefaultSettings()
settings.Wine.ExcludedPrograms = []string{"Game" + string(character) + ".exe"}
if err := Validate(settings); err == nil {
t.Fatalf("accepted control character in exclusion %q", settings.Wine.ExcludedPrograms[0])
}
})
}
}
+1 -1
View File
@@ -38,7 +38,7 @@ func validateWinePrograms(programs []string) error {
return fmt.Errorf("Wine program \"%s\" is a path. Type only the executable name, such as Game.exe, or delete the entry.", name)
case len(name) > 255:
return fmt.Errorf("Wine program \"%s\" is too long. Executable names may be at most 255 characters; fix or delete the entry.", name)
case strings.TrimSpace(name) != name || strings.ContainsAny(name, "\x00\r\n"):
case !config.ValidWineProgramName(name):
return fmt.Errorf("Wine program \"%s\" is not a valid executable name. Remove the surrounding spaces or control characters, or delete the entry.", name)
}
}
+20
View File
@@ -5,6 +5,7 @@
package preferences
import (
"fmt"
"reflect"
"strings"
"testing"
@@ -95,3 +96,22 @@ func TestWineValuesMergeRejectsInvalidEntriesByName(t *testing.T) {
})
}
}
func TestWineValuesRejectEmbeddedControlCharacters(t *testing.T) {
for character := rune(0); character < ' '; character++ {
t.Run(fmt.Sprintf("U+%04X", character), func(t *testing.T) {
settings := config.DefaultSettings()
settings.Wine.ExcludedPrograms = []string{"Kept.exe"}
values := FromSettings(settings)
entry := "Game" + string(character) + ".exe"
values.Wine.ExcludedPrograms = []string{entry}
merged, err := values.Merge(settings)
if err == nil || !strings.Contains(err.Error(), entry) {
t.Fatalf("invalid entry was not named and rejected: %v", err)
}
if !reflect.DeepEqual(merged.Wine, settings.Wine) {
t.Fatalf("rejected entry changed Wine settings: %+v", merged.Wine)
}
})
}
}
+2 -2
View File
@@ -21,8 +21,8 @@ import "strings"
const Name = "Cthulhu"
const Version = "2026.11.01"
const Channel = "testing"
const Version = "2026.11.02"
const Channel = "hunter"
var Build = Version
+20 -15
View File
@@ -23,9 +23,10 @@ type helperDiagnostics struct {
window time.Time
seen map[string]bool
suppressed atomic.Uint64
// onReady, when set before logRecords starts, receives each valid
// ready record so the manager can act on it. It runs on the logging
// worker and must only record state, never supervise processes.
// onReady is installed before the first Write and receives valid ready
// records before diagnostic throttling. It runs on the stderr copier
// (or finish for a final partial line) and must only publish bounded
// state, never log, block, or supervise processes.
onReady func(helperReady)
}
@@ -92,6 +93,15 @@ func (output *helperDiagnostics) queueLine(line string) {
if !strings.HasPrefix(line, "wine-access: ") {
return
}
// Readiness controls exclusion fallback and must survive a full queue,
// diagnostic throttling, and a slow log sink. The line buffer above
// bounds parsing independently of how many diagnostics are retained.
record := strings.TrimPrefix(line, "wine-access: ")
if output.onReady != nil {
if ready, valid := parseHelperReady(record); valid {
output.onReady(ready)
}
}
now := time.Now()
if now.Sub(output.window) >= 10*time.Second {
output.window = now
@@ -103,7 +113,7 @@ func (output *helperDiagnostics) queueLine(line string) {
}
output.seen[line] = true
select {
case output.lines <- strings.TrimPrefix(line, "wine-access: "):
case output.lines <- record:
default:
output.suppressed.Add(1)
}
@@ -136,18 +146,14 @@ func (output *helperDiagnostics) logRecords(logger *slog.Logger) {
reportSuppressed()
return
}
if ready, valid := logHelperRecord(logger, line); valid && output.onReady != nil {
output.onReady(ready)
}
logHelperRecord(logger, line)
case <-ticker.C:
reportSuppressed()
}
}
}
// logHelperRecord logs one helper diagnostic line and returns the parsed
// ready record when the line was a valid one.
func logHelperRecord(logger *slog.Logger, line string) (helperReady, bool) {
func logHelperRecord(logger *slog.Logger, line string) {
if body, found := strings.CutPrefix(line, "identity "); found {
var identity struct {
Version string `json:"version"`
@@ -156,17 +162,17 @@ func logHelperRecord(logger *slog.Logger, line string) (helperReady, bool) {
}
if err := json.Unmarshal([]byte(body), &identity); err != nil || identity.Version == "" || identity.Revision == "" {
logger.Warn("Invalid Wine helper identity record")
return helperReady{}, false
return
}
logger.Info("Wine accessibility helper identity", "version", identity.Version,
"revision", identity.Revision, "modified", identity.Modified)
return helperReady{}, false
return
}
if strings.HasPrefix(line, "ready ") {
status, valid := parseHelperReady(line)
if !valid {
logger.Warn("Invalid Wine helper initialization record")
return helperReady{}, false
return
}
logger.Info("Wine accessibility helper initialized", "reader", status.Reader,
"controls_enabled", status.ControlsEnabled, "controller", status.Controller,
@@ -174,7 +180,7 @@ func logHelperRecord(logger *slog.Logger, line string) (helperReady, bool) {
"win_events", status.WinEvents, "menu_hook", status.MenuHook,
"input_hook", status.InputHook, "host32_started", status.Host32Started,
"control_poll", status.ControlPoll, "hook_exclusions", status.HookExclusions)
return status, true
return
}
// The plain exclusion-count line is expected startup evidence (it is
// emitted even when the helper then exits early), while a rejected
@@ -187,7 +193,6 @@ func logHelperRecord(logger *slog.Logger, line string) (helperReady, bool) {
} else {
logger.Warn("Wine accessibility helper diagnostic", "diagnostic", line)
}
return helperReady{}, false
}
func hookExclusionCountLine(line string) bool {
+61
View File
@@ -4,6 +4,7 @@ import (
"bytes"
"context"
"encoding/json"
"fmt"
"log/slog"
"os"
"path/filepath"
@@ -77,6 +78,66 @@ func TestHelperDiagnosticsBoundsFloodAndRecoversNextWindow(t *testing.T) {
}
}
func TestHelperReadinessBypassesDiagnosticBackpressure(t *testing.T) {
for _, test := range []struct {
name string
windows int
}{
{name: "rate limit", windows: 1},
{name: "full queue", windows: 2},
} {
t.Run(test.name, func(t *testing.T) {
output := newHelperDiagnostics()
child := &process{launchedHelper: "helper", exclusions: map[string]bool{"game.exe": true}}
readyCount := 0
output.onReady = func(ready helperReady) {
readyCount++
child.readyExclusions.Store(int64(ready.HookExclusions))
child.readySeen.Store(true)
}
// Do not start a log consumer: readiness must arrive even if a
// sink cannot make progress. Fill successive rate windows to
// exercise both diagnostic throttling and a completely full queue.
for window := 0; window < test.windows; window++ {
output.window = time.Now().Add(-11 * time.Second)
for index := 0; index < 32; index++ {
fmt.Fprintf(output, "wine-access: startup warning %d/%d\n", window, index)
}
}
if test.windows == 2 {
if len(output.lines) != cap(output.lines) {
t.Fatal("diagnostic queue was not filled")
}
output.window = time.Now().Add(-11 * time.Second)
}
for _, line := range []string{
"wine-access: ready {broken}\n",
"unrelated: ready {\"reader\":\"org.example.Reader\"}\n",
} {
output.Write([]byte(line))
}
if readyCount != 0 {
t.Fatal("invalid or unrelated output published readiness")
}
// Older helpers omit hook_exclusions and therefore report zero.
output.Write([]byte("wine-access: rea"))
output.Write([]byte("dy {\"reader\":\"org.example.Reader\"}\r\n"))
if readyCount != 1 {
t.Fatalf("readiness publications before logging = %d, want 1", readyCount)
}
manager := New(nil)
if !manager.restart_running_helper(child, prefix{}, Settings{}) || !manager.helper_unconfirmed("helper") {
t.Fatal("lost exclusion mismatch fallback under diagnostic backpressure")
}
output.finish()
output.logRecords(slog.New(slog.DiscardHandler))
if readyCount != 1 {
t.Fatalf("logging published readiness again: count = %d", readyCount)
}
})
}
}
func TestManagerCapturesActualHelperDiagnosticsAndBoundsInheritedStderr(t *testing.T) {
root := t.TempDir()
helper := filepath.Join(root, "helper")
+1 -1
View File
@@ -37,7 +37,7 @@ type process struct {
controllerOnly bool
// launchedHelper and exclusions describe the most recent launch and
// are written only by the poll goroutine. The ready fields are written
// by the diagnostics worker for the launch generation that armed them
// by the stderr copier for the launch generation that armed them
// and read by the poll goroutine.
launchedHelper string
exclusions map[string]bool
+3 -3
View File
@@ -9,6 +9,8 @@ import (
"path/filepath"
"sort"
"strings"
"git.stormux.org/storm/cthulhu/internal/config"
)
func program_name(commandLine string) string {
@@ -76,9 +78,7 @@ func exclusion_names(settings Settings) []string {
seen := make(map[string]bool, len(settings.ExcludedPrograms))
var names []string
for _, name := range settings.ExcludedPrograms {
if name == "" || len([]rune(name)) > 255 || strings.TrimSpace(name) != name ||
strings.ContainsAny(name, "/\\") ||
strings.ContainsFunc(name, func(char rune) bool { return char < ' ' }) {
if !config.ValidWineProgramName(name) {
continue
}
lower := strings.ToLower(name)
+17
View File
@@ -7,6 +7,7 @@ import (
"bytes"
"os"
"path/filepath"
"reflect"
"strings"
"testing"
"time"
@@ -110,6 +111,22 @@ func TestListedProgramDoesNotForceWholesaleMode(t *testing.T) {
}
}
func TestHelperArgumentsValidateProgramNames(t *testing.T) {
settings := Settings{ExcludedPrograms: []string{
"Game.exe", "Another Game.exe", "Café.exe", "game.EXE",
"Game\t.exe", "Game\x01.exe", "Game\x1f.exe", `C:\Game.exe`,
strings.Repeat("é", 126) + ".exe",
}}
want := []string{
"--exclude-hook-program", "another game.exe",
"--exclude-hook-program", "café.exe",
"--exclude-hook-program", "game.exe",
}
if got := helper_arguments(false, settings); !reflect.DeepEqual(got, want) {
t.Fatalf("validated helper arguments = %q, want %q", got, want)
}
}
func TestProtonInfrastructureDoesNotKeepHelperAlive(t *testing.T) {
for _, program := range []string{`C:\windows\system32\steam.exe`, `C:\windows\syswow64\steam.exe`, "xalia.exe", "conhost.exe"} {
if applicationProcess(program) {
+19 -11
View File
@@ -578,23 +578,31 @@ def inside():
rows = table.nRows
focus(named("New")); key("space")
wait_for(lambda: table.nRows == rows + 1, "New did not add a Wine row")
subprocess.run(["xdotool", "type", "--clearmodifiers", text], check=True)
if "\t" in text:
# Paste into the draft: sending a Tab key would
# move keyboard focus instead of inserting the character.
subprocess.run(["xclip", "-selection", "clipboard", "-in"],
input=text, text=True, check=True, timeout=5)
key("ctrl+v")
else:
subprocess.run(["xdotool", "type", "--clearmodifiers", text], check=True)
key("Return")
wait_for(lambda: table.getAccessibleAt(rows, 0).name == text,
"inline Wine program editing failed")
focus(controller_only); key("space")
wait_for(lambda: controller_only.getState().contains(pyatspi.STATE_CHECKED),
"controller-only checkbox did not toggle")
add_program(r"C:\Games\Bad.exe")
wait_for(lambda: r"C:\Games\Bad.exe" in validation_text(),
"path entry was not flagged by name in the Wine tab")
wait_for(lambda: not named("Apply").getState().contains(pyatspi.STATE_SENSITIVE),
"invalid Wine entry did not block Apply")
focus(table.getAccessibleAt(1, 0))
focus(delete); key("space")
wait_for(lambda: table.nRows == 1, "Delete did not remove the Wine row")
wait_for(lambda: named("Apply").getState().contains(pyatspi.STATE_SENSITIVE),
"Apply stayed blocked after deleting the invalid entry")
for invalid in (r"C:\Games\Bad.exe", "Game\t.exe"):
add_program(invalid)
wait_for(lambda: invalid in validation_text(),
"invalid entry was not flagged by name in the Wine tab")
wait_for(lambda: not named("Apply").getState().contains(pyatspi.STATE_SENSITIVE),
"invalid Wine entry did not block Apply")
focus(table.getAccessibleAt(1, 0))
focus(delete); key("space")
wait_for(lambda: table.nRows == 1, "Delete did not remove the Wine row")
wait_for(lambda: named("Apply").getState().contains(pyatspi.STATE_SENSITIVE),
"Apply stayed blocked after deleting the invalid entry")
add_program("New Game.exe")
focus(named("Apply")); key("space")
applied = request()