diff --git a/core/internal/desktop/mimeapps.go b/core/internal/desktop/mimeapps.go index d208821c3..6896ac72b 100644 --- a/core/internal/desktop/mimeapps.go +++ b/core/internal/desktop/mimeapps.go @@ -133,6 +133,11 @@ func mergedAssociations() *MimeAssociations { return merged } +// isSafeIniField rejects values that would corrupt a key=value line in mimeapps.list +func isSafeIniField(s string) bool { + return !strings.ContainsAny(s, "\n\r[]") +} + func writeUserMimeapps(update func(*MimeAssociations)) error { mimeappsWriteMu.Lock() defer mimeappsWriteMu.Unlock() @@ -152,6 +157,7 @@ func writeUserMimeapps(update func(*MimeAssociations)) error { var buf bytes.Buffer w := bufio.NewWriter(&buf) + var writeErr error writeSection := func(name string, entries map[string]string) { fmt.Fprintf(w, "[%s]\n", name) keys := make([]string, 0, len(entries)) @@ -160,7 +166,14 @@ func writeUserMimeapps(update func(*MimeAssociations)) error { } sort.Strings(keys) for _, k := range keys { - fmt.Fprintf(w, "%s=%s\n", k, entries[k]) + v := entries[k] + if !isSafeIniField(k) || !isSafeIniField(v) { + if writeErr == nil { + writeErr = fmt.Errorf("invalid mimeapps.list field %q=%q", k, v) + } + continue + } + fmt.Fprintf(w, "%s=%s\n", k, v) } fmt.Fprintln(w) } @@ -177,6 +190,10 @@ func writeUserMimeapps(update func(*MimeAssociations)) error { writeSection(groupAdded, flatten(assoc.Added)) writeSection(groupRemoved, flatten(assoc.Removed)) + if writeErr != nil { + return writeErr + } + if err := w.Flush(); err != nil { return err } diff --git a/core/internal/geolocation/client_geoclue.go b/core/internal/geolocation/client_geoclue.go index a06aadf4d..90d68388d 100644 --- a/core/internal/geolocation/client_geoclue.go +++ b/core/internal/geolocation/client_geoclue.go @@ -132,7 +132,7 @@ func (c *GeoClueClient) startSignalPump() error { if err := c.dbusConn.AddMatchSignal( dbus.WithMatchObjectPath(c.clientPath), dbus.WithMatchInterface(dbusGeoClueClientInterface), - dbus.WithMatchSender(dbusGeoClueClientLocationUpdated), + dbus.WithMatchMember("LocationUpdated"), ); err != nil { return err } diff --git a/core/internal/greeter/installer.go b/core/internal/greeter/installer.go index d48546f11..5767db45c 100644 --- a/core/internal/greeter/installer.go +++ b/core/internal/greeter/installer.go @@ -529,11 +529,23 @@ func execFromDesktopFile(path string) (string, error) { if err != nil { return "", err } + inDesktopEntry := false for line := range strings.SplitSeq(string(data), "\n") { trimmed := strings.TrimSpace(line) - if strings.HasPrefix(trimmed, "Exec=") { - return strings.TrimSpace(trimmed[len("Exec="):]), nil + switch { + case trimmed == "" || strings.HasPrefix(trimmed, "#"): + continue + case strings.HasPrefix(trimmed, "[") && strings.HasSuffix(trimmed, "]"): + inDesktopEntry = trimmed == "[Desktop Entry]" + continue + case !inDesktopEntry: + continue } + key, value, found := strings.Cut(trimmed, "=") + if !found || strings.TrimSpace(key) != "Exec" { + continue + } + return strings.TrimSpace(value), nil } return "", fmt.Errorf("no Exec= line found in %s", path) } diff --git a/core/internal/greeter/session_launcher.go b/core/internal/greeter/session_launcher.go index daa5cd629..0fa33b3e4 100644 --- a/core/internal/greeter/session_launcher.go +++ b/core/internal/greeter/session_launcher.go @@ -3,6 +3,7 @@ package greeter import ( "fmt" "os" + "os/exec" "path/filepath" "strings" "syscall" @@ -93,18 +94,95 @@ func resolveSessionExecInDirs(sessionID string, dirs []string) (string, error) { return "", fmt.Errorf("session desktop file %q was not found", id) } +// parseExecString splits a Desktop Entry Exec= value into argv without +// involving a shell, mirroring quickshell's DesktopEntry::parseExecString +// (string quoting, value escapes, field code stripping). +func parseExecString(execLine string) []string { + var args []string + var cur strings.Builder + inString := false + escape := 0 + percent := false + + for _, c := range execLine { + switch { + case escape == 0 && c == '\\': + escape = 1 + case inString: + switch { + case c == '\\': + escape++ + if escape == 4 { + cur.WriteByte('\\') + escape = 0 + } + case escape == 2: + cur.WriteRune(c) + escape = 0 + case escape != 0: + switch c { + case 's': + cur.WriteByte(' ') + case 'n': + cur.WriteByte('\n') + case 't': + cur.WriteByte('\t') + case 'r': + cur.WriteByte('\r') + default: + cur.WriteRune(c) + } + escape = 0 + case c == '"' || c == '\'': + inString = false + default: + cur.WriteRune(c) + } + case escape != 0: + cur.WriteRune(c) + escape = 0 + case percent: + if c == '%' { + cur.WriteByte('%') + } + percent = false + case c == '%': + percent = true + case c == '"' || c == '\'': + inString = true + case c == ' ': + if cur.Len() > 0 { + args = append(args, cur.String()) + cur.Reset() + } + default: + cur.WriteRune(c) + } + } + if cur.Len() > 0 { + args = append(args, cur.String()) + } + return args +} + func LaunchSessionByID(sessionID string) error { execLine, err := ResolveSessionExec(sessionID) if err != nil { return err } - execLine = strings.TrimSpace(stripDesktopExecCodes(execLine)) - if execLine == "" { + + argv := parseExecString(strings.TrimSpace(execLine)) + if len(argv) == 0 { return fmt.Errorf("session %q has an empty Exec command", sessionID) } + resolved, err := exec.LookPath(argv[0]) + if err != nil { + return fmt.Errorf("session %q command %q not found: %w", sessionID, argv[0], err) + } + env := append(os.Environ(), "XDG_SESSION_TYPE=wayland") - return syscall.Exec("/bin/sh", []string{"sh", "-c", "exec " + execLine}, env) + return syscall.Exec(resolved, argv, env) } func LaunchSessionFromMemory(cacheDir, homeDir string) error { diff --git a/core/internal/greeter/session_launcher_test.go b/core/internal/greeter/session_launcher_test.go new file mode 100644 index 000000000..71dc05951 --- /dev/null +++ b/core/internal/greeter/session_launcher_test.go @@ -0,0 +1,57 @@ +package greeter + +import ( + "path/filepath" + "reflect" + "testing" +) + +func TestParseExecString(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + exec string + want []string + }{ + {"plain", "niri --session", []string{"niri", "--session"}}, + {"extra spaces", "niri --session", []string{"niri", "--session"}}, + {"double quoted arg", `env "with space" run`, []string{"env", "with space", "run"}}, + {"single quoted arg", `env 'with space' run`, []string{"env", "with space", "run"}}, + {"escaped quote in quotes", `sh "say \\"hi\\""`, []string{"sh", `say "hi"`}}, + {"field code dropped", "gnome-session %U", []string{"gnome-session"}}, + {"field code mid-arg", "app --url=%u --run", []string{"app", "--url=", "--run"}}, + {"literal percent", "app 100%% done", []string{"app", "100%", "done"}}, + {"shell metachars stay literal", "sh -c $(reboot); echo", []string{"sh", "-c", "$(reboot);", "echo"}}, + {"empty", "", nil}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := parseExecString(tt.exec); !reflect.DeepEqual(got, tt.want) { + t.Fatalf("parseExecString(%q) = %#v, want %#v", tt.exec, got, tt.want) + } + }) + } +} + +func TestExecFromDesktopFileOnlyReadsDesktopEntryGroup(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "example.desktop") + writeTestFile(t, path, `[Desktop Action other] +Exec=/wrong/binary + +[Desktop Entry] +Name=Example +Exec = /right/binary --flag +`) + + got, err := execFromDesktopFile(path) + if err != nil { + t.Fatalf("execFromDesktopFile returned error: %v", err) + } + if got != "/right/binary --flag" { + t.Fatalf("execFromDesktopFile = %q, want %q", got, "/right/binary --flag") + } +} diff --git a/core/internal/keybinds/providers/hyprland.go b/core/internal/keybinds/providers/hyprland.go index 1108a2b87..be6319e24 100644 --- a/core/internal/keybinds/providers/hyprland.go +++ b/core/internal/keybinds/providers/hyprland.go @@ -299,6 +299,9 @@ type hyprlandOverrideBind struct { Options map[string]any // Unbind: negative override (hl.unbind only, no rebind). Unbind bool + // RawLuaAction: Action is a custom hl.* Lua expression round-tripped from an + // existing Lua override; re-emit it verbatim instead of quoting it. + RawLuaAction bool } func (h *HyprlandProvider) ensureWritableConfig() error { @@ -1046,18 +1049,26 @@ func luaActionStringFromHyprlangAction(action string) string { if expr, ok := luaActionStringFromKnownHyprlandAction(action); ok { return expr } - return action + // Unrecognized dispatchers are freeform text, not Lua; forward them to + // hyprctl quoted so a stray `"` can't produce broken Lua output. + return luaHyprctlDispatchFunction(action) } -func luaExprToInternalAction(expr string) string { +// luaExprToInternalAction converts a parsed Lua bind expression back into +// "dispatcher params" text. isRawLua reports that expr matched no known hl.* +// shape and must be re-emitted verbatim as Lua on write-back. +func luaExprToInternalAction(expr string) (action string, isRawLua bool) { d, p := luaExprToDispatcherParams(expr) + if d == expr && p == "" { + return expr, true + } if d == "exec" && p != "" && !strings.HasPrefix(p, "hyprctl dispatch lua:") { - return "exec " + p + return "exec " + p, false } if p != "" { - return d + " " + p + return d + " " + p, false } - return d + return d, false } func luaBindOptions(bind *hyprlandOverrideBind) []string { @@ -1075,20 +1086,25 @@ func luaBindOptions(bind *hyprlandOverrideBind) []string { } func writeLuaBindLine(sb *strings.Builder, bind *hyprlandOverrideBind) { - key := formatLuaBindKey(bind.Key) + key := strconv.Quote(formatLuaBindKey(bind.Key)) if bind.Unbind { - fmt.Fprintf(sb, `hl.unbind("%s")`, key) + fmt.Fprintf(sb, `hl.unbind(%s)`, key) sb.WriteByte('\n') return } - expr := luaActionStringFromHyprlangAction(bind.Action) + var expr string + if bind.RawLuaAction { + expr = bind.Action + } else { + expr = luaActionStringFromHyprlangAction(bind.Action) + } opts := luaBindOptions(bind) - fmt.Fprintf(sb, `hl.unbind("%s")`, key) + fmt.Fprintf(sb, `hl.unbind(%s)`, key) sb.WriteByte('\n') if len(opts) > 0 { - fmt.Fprintf(sb, `hl.bind("%s", %s, { %s })`, key, expr, strings.Join(opts, ", ")) + fmt.Fprintf(sb, `hl.bind(%s, %s, { %s })`, key, expr, strings.Join(opts, ", ")) } else { - fmt.Fprintf(sb, `hl.bind("%s", %s)`, key, expr) + fmt.Fprintf(sb, `hl.bind(%s, %s)`, key, expr) } sb.WriteByte('\n') } @@ -1104,17 +1120,18 @@ func parseLuaBindOverrideLine(line string) (*hyprlandOverrideBind, bool) { } internalKey := luaKeyComboToInternalKey(kbc) - action := luaExprToInternalAction(actionExpr) + action, isRawLua := luaExprToInternalAction(actionExpr) flags := luaBindOptFlags(optSuffix) description := luaBindOptDescription(optSuffix) if description == "" { description = luaLineTrailingComment(line) } return &hyprlandOverrideBind{ - Key: internalKey, - Action: action, - Description: description, - Flags: flags, + Key: internalKey, + Action: action, + Description: description, + Flags: flags, + RawLuaAction: isRawLua, }, true } diff --git a/core/internal/keybinds/providers/hyprland_parser_test.go b/core/internal/keybinds/providers/hyprland_parser_test.go index afaff44ea..27c0c0edf 100644 --- a/core/internal/keybinds/providers/hyprland_parser_test.go +++ b/core/internal/keybinds/providers/hyprland_parser_test.go @@ -147,9 +147,10 @@ hl.bind("SUPER + N", hl.dsp.exec_cmd("dms ipc call notepad toggle"), { descripti func TestWriteLuaBindLineLeavesCustomLuaDispatcherRaw(t *testing.T) { var sb strings.Builder writeLuaBindLine(&sb, &hyprlandOverrideBind{ - Key: "Super+u", - Action: "hl.dsp.no_op()", - Description: "Custom Lua", + Key: "Super+u", + Action: "hl.dsp.no_op()", + Description: "Custom Lua", + RawLuaAction: true, }) want := `hl.unbind("SUPER + U") @@ -159,6 +160,24 @@ hl.bind("SUPER + U", hl.dsp.no_op(), { description = "Custom Lua" })` } } +func TestWriteLuaBindLineQuotesUnrecognizedActionWithoutRawLuaFlag(t *testing.T) { + var sb strings.Builder + writeLuaBindLine(&sb, &hyprlandOverrideBind{ + Key: "Super+u", + Action: `customdispatcher "),os.execute("id")--`, + }) + + got := sb.String() + if !strings.Contains(got, "hl.exec_cmd(") { + t.Fatalf("expected unrecognized action to go through the hyprctl-dispatch wrapper, got %q", got) + } + // an unpaired bare quote means the action broke out of its string literal + withoutEscapedQuotes := strings.ReplaceAll(got, `\"`, "") + if n := strings.Count(withoutEscapedQuotes, `"`); n%2 != 0 { + t.Fatalf("action broke out of its string literal (%d unpaired quotes): %q", n, got) + } +} + func TestLuaActionStringFromHyprlangActionUsesNativeDispatchers(t *testing.T) { tests := []struct { action string @@ -226,15 +245,12 @@ func TestParseLuaBindLineHandlesFunctionDispatcherFallback(t *testing.T) { } } -func TestLuaActionStringLeavesCustomLuaDispatcherRaw(t *testing.T) { +func TestLuaActionStringFromHyprlangActionAlwaysQuotesUnrecognizedText(t *testing.T) { got := luaActionStringFromHyprlangAction("hl.dsp.no_op()") - want := `hl.dsp.no_op()` + want := `function() hl.exec_cmd("hyprctl dispatch hl.dsp.no_op()") end` if got != want { t.Fatalf("luaActionStringFromHyprlangAction() = %q, want %q", got, want) } - if strings.Contains(got, "hl.dispatch") || strings.Contains(got, "hyprctl dispatch") { - t.Fatalf("expected custom Lua dispatcher expression to stay raw, got %q", got) - } } func TestReadLuaOverrideMigratesTrailingCommentToDescription(t *testing.T) { diff --git a/core/internal/plugins/manager.go b/core/internal/plugins/manager.go index 8ded155cd..37d229d68 100644 --- a/core/internal/plugins/manager.go +++ b/core/internal/plugins/manager.go @@ -64,7 +64,20 @@ func (m *Manager) findInstalledPath(pluginID string) (string, error) { return m.findInDir(systemDir, pluginID) } +// isSafePluginPathComponent rejects ids that aren't a single path component, +// so filepath.Join can't resolve (and later RemoveAll) outside the plugins dir +func isSafePluginPathComponent(s string) bool { + if s == "" || s == "." || s == ".." { + return false + } + return !strings.ContainsAny(s, "/\\") +} + func (m *Manager) findInDir(dir, pluginID string) (string, error) { + if !isSafePluginPathComponent(pluginID) { + return "", fmt.Errorf("invalid plugin id: %q", pluginID) + } + // First, check if folder with exact ID name exists exactPath := filepath.Join(dir, pluginID) if exists, _ := afero.DirExists(m.fs, exactPath); exists { @@ -507,6 +520,10 @@ func (m *Manager) findInstalledPathByIDOrName(idOrName string) (string, error) { } func (m *Manager) findInDirByIDOrName(dir, idOrName string) (string, error) { + if !isSafePluginPathComponent(idOrName) { + return "", fmt.Errorf("invalid plugin id/name: %q", idOrName) + } + // Check exact folder name match first exactPath := filepath.Join(dir, idOrName) if exists, _ := afero.DirExists(m.fs, exactPath); exists { diff --git a/core/internal/privesc/privesc.go b/core/internal/privesc/privesc.go index 2c4630022..d693452db 100644 --- a/core/internal/privesc/privesc.go +++ b/core/internal/privesc/privesc.go @@ -135,15 +135,14 @@ func EscapeSingleQuotes(s string) string { } // MakeCommand returns a bash command string that runs `command` with the -// detected tool. When the tool supports stdin passwords and password is -// non-empty, the password is piped in. Otherwise the tool is invoked with -// no non-interactive flag so that an interactive TTY prompt is still -// possible for CLI callers. +// detected tool, prompting interactively on a TTY where applicable. The +// sudo-with-password case lives in ExecCommand, which pipes the password via +// stdin so it never lands in argv. // // If detection fails, the returned shell string exits 1 with an error // message so callers that treat the *exec.Cmd as infallible still fail // deterministically. -func MakeCommand(password, command string) string { +func MakeCommand(command string) string { t, err := Detect() if err != nil { return failingShell(err) @@ -151,9 +150,6 @@ func MakeCommand(password, command string) string { switch t { case ToolSudo: - if password != "" { - return fmt.Sprintf("echo '%s' | sudo -S %s", EscapeSingleQuotes(password), command) - } return fmt.Sprintf("sudo %s", command) case ToolDoas: return fmt.Sprintf("doas sh -c '%s'", EscapeSingleQuotes(command)) @@ -166,9 +162,19 @@ func MakeCommand(password, command string) string { // ExecCommand builds an exec.Cmd that runs `command` as root via the // detected tool. Detection errors surface at Run() time as a failing -// command writing a clear error to stderr. +// command writing a clear error to stderr. A sudo password is piped via +// stdin (sudo -S) so it never appears in argv. func ExecCommand(ctx context.Context, password, command string) *exec.Cmd { - return exec.CommandContext(ctx, "bash", "-c", MakeCommand(password, command)) + t, err := Detect() + if err != nil { + return exec.CommandContext(ctx, "bash", "-c", failingShell(err)) + } + if t == ToolSudo && password != "" { + cmd := exec.CommandContext(ctx, "sudo", "-S", "sh", "-c", command) + cmd.Stdin = strings.NewReader(password + "\n") + return cmd + } + return exec.CommandContext(ctx, "bash", "-c", MakeCommand(command)) } // ExecArgv builds an exec.Cmd that runs argv as root via the detected tool. diff --git a/core/internal/screenshot/screenshot.go b/core/internal/screenshot/screenshot.go index 71b7fe591..43253a21a 100644 --- a/core/internal/screenshot/screenshot.go +++ b/core/internal/screenshot/screenshot.go @@ -747,12 +747,16 @@ func (s *Screenshoter) processFrame(frame *wlr_screencopy.ZwlrScreencopyFrameV1, bpp := format.BytesPerPixel() if int(e.Stride) < int(e.Width)*bpp { log.Error("invalid stride from compositor", "stride", e.Stride, "width", e.Width, "bpp", bpp) + // bail out here or the dispatch loop waits forever on a ready/failed + // event that never comes (frame.Copy is never called) + failed = true return } var err error buf, err = CreateShmBuffer(int(e.Width), int(e.Height), int(e.Stride)) if err != nil { log.Error("failed to create buffer", "err", err) + failed = true return } buf.Format = format @@ -771,6 +775,7 @@ func (s *Screenshoter) processFrame(frame *wlr_screencopy.ZwlrScreencopyFrameV1, pool, err = s.shm.CreatePool(buf.Fd(), int32(buf.Size())) if err != nil { log.Error("failed to create pool", "err", err) + failed = true return } @@ -779,6 +784,7 @@ func (s *Screenshoter) processFrame(frame *wlr_screencopy.ZwlrScreencopyFrameV1, pool.Destroy() pool = nil log.Error("failed to create wl_buffer", "err", err) + failed = true return } diff --git a/core/internal/server/clipboard/manager.go b/core/internal/server/clipboard/manager.go index 6486ecda5..664a29775 100644 --- a/core/internal/server/clipboard/manager.go +++ b/core/internal/server/clipboard/manager.go @@ -1839,21 +1839,34 @@ func (m *Manager) EntryToFile(entry *Entry) string { return "" } +func (m *Manager) dbusConnForFlatpak() (*dbus.Conn, error) { + m.dbusConnMutex.Lock() + defer m.dbusConnMutex.Unlock() + + if m.dbusConn != nil { + return m.dbusConn, nil + } + + conn, err := dbus.ConnectSessionBus() + if err != nil { + return nil, fmt.Errorf("connect session bus: %w", err) + } + if !conn.SupportsUnixFDs() { + conn.Close() + return nil, fmt.Errorf("D-Bus connection does not support Unix FD passing") + } + m.dbusConn = conn + return conn, nil +} + func (m *Manager) ExportFileForFlatpak(filePath string) (string, error) { if _, err := os.Stat(filePath); err != nil { return "", fmt.Errorf("file not found: %w", err) } - if m.dbusConn == nil { - conn, err := dbus.ConnectSessionBus() - if err != nil { - return "", fmt.Errorf("connect session bus: %w", err) - } - if !conn.SupportsUnixFDs() { - conn.Close() - return "", fmt.Errorf("D-Bus connection does not support Unix FD passing") - } - m.dbusConn = conn + dbusConn, err := m.dbusConnForFlatpak() + if err != nil { + return "", err } file, err := os.Open(filePath) @@ -1862,7 +1875,7 @@ func (m *Manager) ExportFileForFlatpak(filePath string) (string, error) { } fd := int(file.Fd()) - portal := m.dbusConn.Object("org.freedesktop.portal.Documents", "/org/freedesktop/portal/documents") + portal := dbusConn.Object("org.freedesktop.portal.Documents", "/org/freedesktop/portal/documents") var docIds []string var extra map[string]dbus.Variant diff --git a/core/internal/server/clipboard/types.go b/core/internal/server/clipboard/types.go index 2427badec..f883d2113 100644 --- a/core/internal/server/clipboard/types.go +++ b/core/internal/server/clipboard/types.go @@ -153,7 +153,9 @@ type Manager struct { notifierWg sync.WaitGroup lastState *State - dbusConn *dbus.Conn + // lazily created by dbusConnForFlatpak under dbusConnMutex + dbusConn *dbus.Conn + dbusConnMutex sync.Mutex } func (m *Manager) GetState() State { diff --git a/core/internal/server/cups/subscription.go b/core/internal/server/cups/subscription.go index 6b369b4ea..483a52bae 100644 --- a/core/internal/server/cups/subscription.go +++ b/core/internal/server/cups/subscription.go @@ -37,6 +37,9 @@ func (sm *SubscriptionManager) Start() error { return fmt.Errorf("subscription manager already running") } sm.running = true + // replace the channel closed by the previous Stop(); doing it here rather + // than in Stop() guarantees a lagging eventHandler still observes the close + sm.eventChan = make(chan SubscriptionEvent, 100) sm.mu.Unlock() subID, err := sm.createSubscription() @@ -206,6 +209,8 @@ func (sm *SubscriptionManager) parseEvent(attrs ipp.Attributes) SubscriptionEven } func (sm *SubscriptionManager) Events() <-chan SubscriptionEvent { + sm.mu.Lock() + defer sm.mu.Unlock() return sm.eventChan } @@ -228,6 +233,13 @@ func (sm *SubscriptionManager) Stop() { } sm.stopChan = make(chan struct{}) + + // the writer (notificationLoop) joined above, so closing is safe; without + // this close Manager.eventHandler never returns and Unsubscribe deadlocks + // on eventWG.Wait(). Start() allocates the replacement. + sm.mu.Lock() + close(sm.eventChan) + sm.mu.Unlock() } func (sm *SubscriptionManager) cancelSubscription() { diff --git a/core/internal/server/cups/subscription_dbus.go b/core/internal/server/cups/subscription_dbus.go index b205a6baf..951e54984 100644 --- a/core/internal/server/cups/subscription_dbus.go +++ b/core/internal/server/cups/subscription_dbus.go @@ -38,6 +38,8 @@ func (sm *DBusSubscriptionManager) Start() error { return fmt.Errorf("subscription manager already running") } sm.running = true + // replaced here rather than in Stop(); see SubscriptionManager.Start() + sm.eventChan = make(chan SubscriptionEvent, 100) sm.mu.Unlock() conn, err := dbus.ConnectSystemBus() @@ -252,6 +254,8 @@ func (sm *DBusSubscriptionManager) parseDBusSignal(sig *dbus.Signal) Subscriptio } func (sm *DBusSubscriptionManager) Events() <-chan SubscriptionEvent { + sm.mu.Lock() + defer sm.mu.Unlock() return sm.eventChan } @@ -278,6 +282,12 @@ func (sm *DBusSubscriptionManager) Stop() { } sm.stopChan = make(chan struct{}) + + // the writer (dbusListenerLoop) joined above, so closing is safe; see + // SubscriptionManager.Stop() + sm.mu.Lock() + close(sm.eventChan) + sm.mu.Unlock() } func (sm *DBusSubscriptionManager) cancelSubscription() { diff --git a/core/internal/server/dbus/handlers.go b/core/internal/server/dbus/handlers.go index e625d7963..cab6ade12 100644 --- a/core/internal/server/dbus/handlers.go +++ b/core/internal/server/dbus/handlers.go @@ -201,6 +201,10 @@ func handleListNames(conn net.Conn, req models.Request, m *Manager) { } func handleSubscribe(conn net.Conn, req models.Request, m *Manager, clientID string) { + if id := params.StringOpt(req.Params, "clientId", ""); id != "" { + clientID = id + } + bus, err := params.String(req.Params, "bus") if err != nil { models.RespondError(conn, req.ID, err.Error()) diff --git a/core/internal/server/freedesktop/manager.go b/core/internal/server/freedesktop/manager.go index c1d550191..510c78cb2 100644 --- a/core/internal/server/freedesktop/manager.go +++ b/core/internal/server/freedesktop/manager.go @@ -137,22 +137,25 @@ func (m *Manager) consumeSelfEcho(value uint32) bool { } func (m *Manager) watchSettingsChanges() { - conn, err := dbus.ConnectSessionBus() - if err != nil { - log.Warnf("color-scheme watcher: session bus connect: %v", err) + // reuse the shared session connection; a dedicated one was unreachable + // from Close() and leaked with this goroutine + if m.sessionConn == nil { return } + conn := m.sessionConn if err := conn.AddMatchSignal( dbus.WithMatchInterface(dbusPortalSettingsInterface), dbus.WithMatchMember("SettingChanged"), ); err != nil { log.Warnf("Failed to watch portal settings changes: %v", err) - conn.Close() return } signals := make(chan *dbus.Signal, 64) + m.stateMutex.Lock() + m.settingsSignals = signals + m.stateMutex.Unlock() conn.Signal(signals) for sig := range signals { @@ -309,6 +312,18 @@ func (m *Manager) Close() { m.systemConn.Close() } if m.sessionConn != nil { + m.sessionConn.RemoveMatchSignal( + dbus.WithMatchInterface(dbusPortalSettingsInterface), + dbus.WithMatchMember("SettingChanged"), + ) + m.stateMutex.Lock() + signals := m.settingsSignals + m.settingsSignals = nil + m.stateMutex.Unlock() + if signals != nil { + m.sessionConn.RemoveSignal(signals) + close(signals) + } m.sessionConn.Close() } } diff --git a/core/internal/server/freedesktop/types.go b/core/internal/server/freedesktop/types.go index 0d9d9c5ae..c68ff92a5 100644 --- a/core/internal/server/freedesktop/types.go +++ b/core/internal/server/freedesktop/types.go @@ -71,4 +71,6 @@ type Manager struct { screensaverGnomeClaimed bool selfEchoMu sync.Mutex selfEchoes []colorSchemeEcho + // registered on sessionConn by watchSettingsChanges; guarded by stateMutex + settingsSignals chan *dbus.Signal } diff --git a/core/internal/server/network/backend_networkmanager.go b/core/internal/server/network/backend_networkmanager.go index e292bdfe7..d4fcae2d4 100644 --- a/core/internal/server/network/backend_networkmanager.go +++ b/core/internal/server/network/backend_networkmanager.go @@ -2,6 +2,7 @@ package network import ( "fmt" + "maps" "sync" "github.com/AvengeMedia/DankMaterialShell/core/internal/log" @@ -57,6 +58,11 @@ type NetworkManagerBackend struct { wifiDev any wifiDevices map[string]*wifiDeviceInfo + // devMutex guards ethernetDevices/wifiDevices (written by the signal pump, + // read by request handlers). Not reentrant — never hold it across calls + // into other backend methods. + devMutex sync.RWMutex + dbusConn *dbus.Conn signals chan *dbus.Signal sigWG sync.WaitGroup @@ -185,12 +191,12 @@ func (b *NetworkManagerBackend) Initialize() error { } hwAddr, _ := w.GetPropertyHwAddress() - b.ethernetDevices[iface] = ðernetDeviceInfo{ + b.setEthernetDeviceInfo(iface, ðernetDeviceInfo{ device: dev, wired: w, name: iface, hwAddress: hwAddr, - } + }) if b.ethernetDevice == nil { b.ethernetDevice = dev @@ -214,12 +220,12 @@ func (b *NetworkManagerBackend) Initialize() error { } hwAddr, _ := w.GetPropertyHwAddress() - b.wifiDevices[iface] = &wifiDeviceInfo{ + b.setWifiDeviceInfo(iface, &wifiDeviceInfo{ device: dev, wireless: w, name: iface, hwAddress: hwAddr, - } + }) if b.wifiDevice == nil { b.wifiDevice = dev @@ -267,6 +273,80 @@ func (b *NetworkManagerBackend) Initialize() error { return nil } +func (b *NetworkManagerBackend) ethernetDevicesSnapshot() map[string]*ethernetDeviceInfo { + b.devMutex.RLock() + defer b.devMutex.RUnlock() + out := make(map[string]*ethernetDeviceInfo, len(b.ethernetDevices)) + maps.Copy(out, b.ethernetDevices) + return out +} + +func (b *NetworkManagerBackend) wifiDevicesSnapshot() map[string]*wifiDeviceInfo { + b.devMutex.RLock() + defer b.devMutex.RUnlock() + out := make(map[string]*wifiDeviceInfo, len(b.wifiDevices)) + maps.Copy(out, b.wifiDevices) + return out +} + +func (b *NetworkManagerBackend) ethernetDeviceByIface(iface string) (*ethernetDeviceInfo, bool) { + b.devMutex.RLock() + defer b.devMutex.RUnlock() + info, ok := b.ethernetDevices[iface] + return info, ok +} + +func (b *NetworkManagerBackend) wifiDeviceByIface(iface string) (*wifiDeviceInfo, bool) { + b.devMutex.RLock() + defer b.devMutex.RUnlock() + info, ok := b.wifiDevices[iface] + return info, ok +} + +func (b *NetworkManagerBackend) setEthernetDeviceInfo(iface string, info *ethernetDeviceInfo) { + b.devMutex.Lock() + b.ethernetDevices[iface] = info + b.devMutex.Unlock() +} + +func (b *NetworkManagerBackend) setWifiDeviceInfo(iface string, info *wifiDeviceInfo) { + b.devMutex.Lock() + b.wifiDevices[iface] = info + b.devMutex.Unlock() +} + +// removeEthernetDeviceByPath deletes the device and returns a snapshot of +// what's left so the caller can pick a replacement without holding devMutex +func (b *NetworkManagerBackend) removeEthernetDeviceByPath(path dbus.ObjectPath) (removed *ethernetDeviceInfo, remaining map[string]*ethernetDeviceInfo, found bool) { + b.devMutex.Lock() + defer b.devMutex.Unlock() + for iface, info := range b.ethernetDevices { + if info.device.GetPath() != path { + continue + } + delete(b.ethernetDevices, iface) + remaining = make(map[string]*ethernetDeviceInfo, len(b.ethernetDevices)) + maps.Copy(remaining, b.ethernetDevices) + return info, remaining, true + } + return nil, nil, false +} + +func (b *NetworkManagerBackend) removeWifiDeviceByPath(path dbus.ObjectPath) (removed *wifiDeviceInfo, remaining map[string]*wifiDeviceInfo, found bool) { + b.devMutex.Lock() + defer b.devMutex.Unlock() + for iface, info := range b.wifiDevices { + if info.device.GetPath() != path { + continue + } + delete(b.wifiDevices, iface) + remaining = make(map[string]*wifiDeviceInfo, len(b.wifiDevices)) + maps.Copy(remaining, b.wifiDevices) + return info, remaining, true + } + return nil, nil, false +} + func (b *NetworkManagerBackend) Close() { close(b.stopChan) b.StopMonitoring() diff --git a/core/internal/server/network/backend_networkmanager_ethernet.go b/core/internal/server/network/backend_networkmanager_ethernet.go index 47f785b4d..dd96a5132 100644 --- a/core/internal/server/network/backend_networkmanager_ethernet.go +++ b/core/internal/server/network/backend_networkmanager_ethernet.go @@ -323,7 +323,7 @@ func (b *NetworkManagerBackend) GetEthernetDevices() []EthernetDevice { } func (b *NetworkManagerBackend) DisconnectEthernetDevice(device string) error { - info, ok := b.ethernetDevices[device] + info, ok := b.ethernetDeviceByIface(device) if !ok { return fmt.Errorf("ethernet device %s not found", device) } @@ -345,9 +345,10 @@ func (b *NetworkManagerBackend) DisconnectEthernetDevice(device string) error { } func (b *NetworkManagerBackend) updateAllEthernetDevices() { - devices := make([]EthernetDevice, 0, len(b.ethernetDevices)) + ethernetDevices := b.ethernetDevicesSnapshot() + devices := make([]EthernetDevice, 0, len(ethernetDevices)) - for name, info := range b.ethernetDevices { + for name, info := range ethernetDevices { state, _ := info.device.GetPropertyState() connected := state == gonetworkmanager.NmDeviceStateActivated driver, _ := info.device.GetPropertyDriver() diff --git a/core/internal/server/network/backend_networkmanager_signals.go b/core/internal/server/network/backend_networkmanager_signals.go index 1ea52cba9..6fe525265 100644 --- a/core/internal/server/network/backend_networkmanager_signals.go +++ b/core/internal/server/network/backend_networkmanager_signals.go @@ -112,7 +112,7 @@ func (b *NetworkManagerBackend) startSignalPump() error { return err } - for _, info := range b.wifiDevices { + for _, info := range b.wifiDevicesSnapshot() { if err := conn.AddMatchSignal( dbus.WithMatchObjectPath(dbus.ObjectPath(info.device.GetPath())), dbus.WithMatchInterface(dbusPropsInterface), @@ -124,7 +124,7 @@ func (b *NetworkManagerBackend) startSignalPump() error { } } - for _, info := range b.ethernetDevices { + for _, info := range b.ethernetDevicesSnapshot() { if err := conn.AddMatchSignal( dbus.WithMatchObjectPath(dbus.ObjectPath(info.device.GetPath())), dbus.WithMatchInterface(dbusPropsInterface), @@ -227,7 +227,7 @@ func (b *NetworkManagerBackend) stopSignalPump() { dbus.WithMatchMember("StateChanged"), ) - for _, info := range b.wifiDevices { + for _, info := range b.wifiDevicesSnapshot() { b.dbusConn.RemoveMatchSignal( dbus.WithMatchObjectPath(dbus.ObjectPath(info.device.GetPath())), dbus.WithMatchInterface(dbusPropsInterface), @@ -235,7 +235,7 @@ func (b *NetworkManagerBackend) stopSignalPump() { ) } - for _, info := range b.ethernetDevices { + for _, info := range b.ethernetDevicesSnapshot() { b.dbusConn.RemoveMatchSignal( dbus.WithMatchObjectPath(dbus.ObjectPath(info.device.GetPath())), dbus.WithMatchInterface(dbusPropsInterface), @@ -550,12 +550,12 @@ func (b *NetworkManagerBackend) handleDeviceAdded(devicePath dbus.ObjectPath) { } hwAddr, _ := w.GetPropertyHwAddress() - b.ethernetDevices[iface] = ðernetDeviceInfo{ + b.setEthernetDeviceInfo(iface, ðernetDeviceInfo{ device: dev, wired: w, name: iface, hwAddress: hwAddr, - } + }) if b.ethernetDevice == nil { b.ethernetDevice = dev @@ -573,12 +573,12 @@ func (b *NetworkManagerBackend) handleDeviceAdded(devicePath dbus.ObjectPath) { } hwAddr, _ := w.GetPropertyHwAddress() - b.wifiDevices[iface] = &wifiDeviceInfo{ + b.setWifiDeviceInfo(iface, &wifiDeviceInfo{ device: dev, wireless: w, name: iface, hwAddress: hwAddr, - } + }) if b.wifiDevice == nil { b.wifiDevice = dev @@ -603,57 +603,49 @@ func (b *NetworkManagerBackend) handleDeviceRemoved(devicePath dbus.ObjectPath) ) } - for iface, info := range b.ethernetDevices { - if info.device.GetPath() == devicePath { - delete(b.ethernetDevices, iface) - - if b.ethernetDevice != nil { - dev := b.ethernetDevice.(gonetworkmanager.Device) - if dev.GetPath() == devicePath { - b.ethernetDevice = nil - for _, remaining := range b.ethernetDevices { - b.ethernetDevice = remaining.device - break - } + if _, remaining, found := b.removeEthernetDeviceByPath(devicePath); found { + if b.ethernetDevice != nil { + dev := b.ethernetDevice.(gonetworkmanager.Device) + if dev.GetPath() == devicePath { + b.ethernetDevice = nil + for _, r := range remaining { + b.ethernetDevice = r.device + break } } - - b.updateAllEthernetDevices() - b.updateEthernetState() - b.listEthernetConnections() - b.updatePrimaryConnection() - - if b.onStateChange != nil { - b.onStateChange() - } - return } + + b.updateAllEthernetDevices() + b.updateEthernetState() + b.listEthernetConnections() + b.updatePrimaryConnection() + + if b.onStateChange != nil { + b.onStateChange() + } + return } - for iface, info := range b.wifiDevices { - if info.device.GetPath() == devicePath { - delete(b.wifiDevices, iface) - - if b.wifiDevice != nil { - dev := b.wifiDevice.(gonetworkmanager.Device) - if dev.GetPath() == devicePath { - b.wifiDevice = nil - b.wifiDev = nil - for _, remaining := range b.wifiDevices { - b.wifiDevice = remaining.device - b.wifiDev = remaining.wireless - break - } + if _, remaining, found := b.removeWifiDeviceByPath(devicePath); found { + if b.wifiDevice != nil { + dev := b.wifiDevice.(gonetworkmanager.Device) + if dev.GetPath() == devicePath { + b.wifiDevice = nil + b.wifiDev = nil + for _, r := range remaining { + b.wifiDevice = r.device + b.wifiDev = r.wireless + break } } - - b.updateAllWiFiDevices() - b.updateWiFiState() - - if b.onStateChange != nil { - b.onStateChange() - } - return } + + b.updateAllWiFiDevices() + b.updateWiFiState() + + if b.onStateChange != nil { + b.onStateChange() + } + return } } diff --git a/core/internal/server/network/backend_networkmanager_state.go b/core/internal/server/network/backend_networkmanager_state.go index 348fed301..c1361cc0f 100644 --- a/core/internal/server/network/backend_networkmanager_state.go +++ b/core/internal/server/network/backend_networkmanager_state.go @@ -76,7 +76,7 @@ func (b *NetworkManagerBackend) updateEthernetState() error { var connectedIP string var anyConnected bool - for name, info := range b.ethernetDevices { + for name, info := range b.ethernetDevicesSnapshot() { state, err := info.device.GetPropertyState() if err != nil { continue diff --git a/core/internal/server/network/backend_networkmanager_wifi.go b/core/internal/server/network/backend_networkmanager_wifi.go index 4f4ae8301..d32c45fdc 100644 --- a/core/internal/server/network/backend_networkmanager_wifi.go +++ b/core/internal/server/network/backend_networkmanager_wifi.go @@ -973,7 +973,7 @@ func (b *NetworkManagerBackend) SetWiFiAutoconnect(ssid string, autoconnect bool } func (b *NetworkManagerBackend) ScanWiFiDevice(device string) error { - devInfo, ok := b.wifiDevices[device] + devInfo, ok := b.wifiDeviceByIface(device) if !ok { return fmt.Errorf("WiFi device not found: %s", device) } @@ -995,7 +995,7 @@ func (b *NetworkManagerBackend) ScanWiFiDevice(device string) error { } func (b *NetworkManagerBackend) DisconnectWiFiDevice(device string) error { - devInfo, ok := b.wifiDevices[device] + devInfo, ok := b.wifiDeviceByIface(device) if !ok { return fmt.Errorf("WiFi device not found: %s", device) } @@ -1047,7 +1047,7 @@ func (b *NetworkManagerBackend) updateAllWiFiDevices() { wifiConnected := b.state.WiFiConnected b.stateMutex.RUnlock() - for name, devInfo := range b.wifiDevices { + for name, devInfo := range b.wifiDevicesSnapshot() { state, _ := devInfo.device.GetPropertyState() connected := state == gonetworkmanager.NmDeviceStateActivated @@ -1211,7 +1211,7 @@ func (b *NetworkManagerBackend) updateAllWiFiDevices() { func (b *NetworkManagerBackend) getWifiDeviceForConnection(deviceName string) (*wifiDeviceInfo, error) { if deviceName != "" { - devInfo, ok := b.wifiDevices[deviceName] + devInfo, ok := b.wifiDeviceByIface(deviceName) if !ok { return nil, fmt.Errorf("WiFi device not found: %s", deviceName) } @@ -1224,7 +1224,7 @@ func (b *NetworkManagerBackend) getWifiDeviceForConnection(deviceName string) (* dev := b.wifiDevice.(gonetworkmanager.Device) iface, _ := dev.GetPropertyInterface() - if devInfo, ok := b.wifiDevices[iface]; ok { + if devInfo, ok := b.wifiDeviceByIface(iface); ok { return devInfo, nil } diff --git a/core/internal/server/server.go b/core/internal/server/server.go index 5d2480f91..02df7d073 100644 --- a/core/internal/server/server.go +++ b/core/internal/server/server.go @@ -8,6 +8,7 @@ import ( "net" "os" "path/filepath" + "runtime/debug" "strconv" "strings" "sync" @@ -41,7 +42,7 @@ import ( "github.com/AvengeMedia/DankMaterialShell/core/pkg/syncmap" ) -const APIVersion = 27 +const APIVersion = 28 var CLIVersion = "dev" @@ -398,6 +399,11 @@ func InitializeSysUpdateManager() error { func handleConnection(conn net.Conn) { defer conn.Close() + defer func() { + if r := recover(); r != nil { + log.Errorf("handleConnection panic recovered: panic=%v\n%s", r, debug.Stack()) + } + }() caps := getCapabilities() capsData, _ := json.Marshal(caps) @@ -415,10 +421,21 @@ func handleConnection(conn net.Conn) { continue } - go RouteRequest(conn, req) + go routeRequestRecovered(conn, req) } } +// routeRequestRecovered keeps a panicking handler from taking down the whole daemon +func routeRequestRecovered(conn net.Conn, req models.Request) { + defer func() { + if r := recover(); r != nil { + log.Errorf("RouteRequest panic recovered: method=%s panic=%v\n%s", req.Method, r, debug.Stack()) + models.RespondError(conn, req.ID, "internal server error") + } + }() + RouteRequest(conn, req) +} + func getCapabilities() Capabilities { caps := []string{"plugins"} @@ -581,6 +598,11 @@ func notifyCapabilityChange() { func handleSubscribe(conn net.Conn, req models.Request) { clientID := fmt.Sprintf("meta-client-%p", conn) + dbusClient := dbusClientID + if id, ok := models.Get[string](req, "clientId"); ok && id != "" { + dbusClient = id + } + var services []string if servicesParam, ok := models.Get[[]any](req, "services"); ok { for _, s := range servicesParam { @@ -1249,10 +1271,10 @@ func handleSubscribe(conn net.Conn, req models.Request) { if shouldSubscribe("dbus") && dbusManager != nil { wg.Add(1) - dbusChan := dbusManager.SubscribeSignals(dbusClientID) + dbusChan := dbusManager.SubscribeSignals(dbusClient) go func() { defer wg.Done() - defer dbusManager.UnsubscribeSignals(dbusClientID) + defer dbusManager.UnsubscribeSignals(dbusClient) for { select { diff --git a/core/pkg/go-wayland/wayland/client/context.go b/core/pkg/go-wayland/wayland/client/context.go index 4eee2cb64..7e48f6aaa 100644 --- a/core/pkg/go-wayland/wayland/client/context.go +++ b/core/pkg/go-wayland/wayland/client/context.go @@ -96,7 +96,7 @@ func (ctx *Context) GetDispatch() func() error { } } - return func() error { + return func() (dispatchErr error) { proxy, ok := ctx.objects.Load(senderID) if !ok { return nil // Proxy already deleted via delete_id, silently ignore @@ -111,6 +111,14 @@ func (ctx *Context) GetDispatch() func() error { return fmt.Errorf("%w (senderID=%d)", ErrDispatchSenderUnsupported, senderID) } + // generated Dispatch methods don't bounds-check wire data; surface a + // decoder panic as an error instead of crashing the process + defer func() { + if r := recover(); r != nil { + dispatchErr = fmt.Errorf("dispatch: panic handling opcode=%d senderID=%d: %v", opcode, senderID, r) + } + }() + sender.Dispatch(opcode, fd, data) return nil } diff --git a/quickshell/Services/DMSService.qml b/quickshell/Services/DMSService.qml index 4f99fc93e..331023837 100644 --- a/quickshell/Services/DMSService.qml +++ b/quickshell/Services/DMSService.qml @@ -246,13 +246,14 @@ Singleton { function sendSubscribeRequest() { const request = { - "method": "subscribe" + "method": "subscribe", + "params": { + "clientId": dbusClientId + } }; if (activeSubscriptions.length > 0) { - request.params = { - "services": activeSubscriptions - }; + request.params.services = activeSubscriptions; log.debug("Subscribing to services:", JSON.stringify(activeSubscriptions)); } else { log.debug("Subscribing to all services"); @@ -672,6 +673,7 @@ Singleton { signal dbusSignalReceived(string subscriptionId, var data) + readonly property string dbusClientId: "dms-qml-" + Date.now() + "-" + Math.floor(Math.random() * 0xffffffff) property var dbusSubscriptions: ({}) function dbusCall(bus, dest, path, iface, method, args, callback) { @@ -735,7 +737,8 @@ Singleton { "sender": sender || "", "path": path || "", "interface": iface || "", - "member": member || "" + "member": member || "", + "clientId": dbusClientId }, response => { if (!response.error && response.result?.subscriptionId) { dbusSubscriptions[response.result.subscriptionId] = true;