diff --git a/core/cmd/dms/commands_icc.go b/core/cmd/dms/commands_icc.go index 2f15696a7..34010ba1d 100644 --- a/core/cmd/dms/commands_icc.go +++ b/core/cmd/dms/commands_icc.go @@ -189,7 +189,14 @@ func runICCList(cmd *cobra.Command, args []string) { fmt.Printf(" %-40s (parse error: %v)\n", filepath.Base(path), err) continue } - fmt.Printf(" %-40s %s\n", filepath.Base(path), profile.Description) + // A profile without a vcgt table has no video card gamma ramp, so the + // daemon refuses to apply it: say so here instead of letting the user + // find out from a rejected `dms icc apply`. + note := "" + if !profile.HasVCGT { + note = " (no vcgt table: cannot be applied)" + } + fmt.Printf(" %-40s %s%s\n", filepath.Base(path), profile.Description, note) } } } @@ -271,11 +278,20 @@ func runICCApply(cmd *cobra.Command, args []string) { filePath = abs } - // Validate file exists and is a valid ICC profile + // Validate file exists and is a valid ICC profile. The rules the daemon + // enforces are checked here too: a profile it would refuse must not be copied + // into the profile directory first, or `dms icc list` keeps offering a file + // that can never be applied. profile, err := icc.ParseFile(filePath) if err != nil { log.Fatalf("Invalid ICC profile: %v", err) } + if profile.ColorSpace != "RGB" { + log.Fatalf("Unsupported color space %q, only RGB profiles can be applied", profile.ColorSpace) + } + if !profile.HasVCGT { + log.Fatalf("%s has no vcgt table, so it has no gamma ramp to apply", filepath.Base(filePath)) + } // Copy the ICC file to config dir for persistence configDir := getICCConfigDir() @@ -284,7 +300,11 @@ func runICCApply(cmd *cobra.Command, args []string) { } destPath := filepath.Join(configDir, filepath.Base(filePath)) + copied := false if filepath.Clean(filePath) != filepath.Clean(destPath) { + if _, err := os.Stat(destPath); err != nil { + copied = true + } data, err := os.ReadFile(filePath) if err != nil { log.Fatalf("Failed to read ICC file: %v", err) @@ -302,19 +322,31 @@ func runICCApply(cmd *cobra.Command, args []string) { "path": destPath, }, } - resp, err := sendServerRequest(req) if err != nil { + dropCopiedProfile(destPath, copied) log.Fatalf("Failed to apply ICC profile: %v", err) } - if resp.Error != "" { + dropCopiedProfile(destPath, copied) log.Fatalf("Server error: %s", resp.Error) } fmt.Printf("Applied ICC profile '%s' to output '%s'\n", profile.Description, outputName) } +// dropCopiedProfile removes the copy this command just made, so a rejected apply +// does not leave a profile in the config directory that was never applied (a +// file that was already there is left alone). +func dropCopiedProfile(path string, copied bool) { + if !copied { + return + } + if err := os.Remove(path); err != nil && !os.IsNotExist(err) { + log.Errorf("Failed to remove %s: %v", path, err) + } +} + func runICCRemove(cmd *cobra.Command, args []string) { outputName := args[0] diff --git a/core/internal/server/wayland/manager.go b/core/internal/server/wayland/manager.go index 14d8ecc84..aa4e75a75 100644 --- a/core/internal/server/wayland/manager.go +++ b/core/internal/server/wayland/manager.go @@ -8,7 +8,9 @@ import ( "io" "maps" "os" + "path/filepath" "slices" + "strings" "syscall" "time" @@ -1551,10 +1553,43 @@ func (m *Manager) applyConfiguredICCForOutput(out *outputState, outputName strin return changed } +// validateOutputName rejects names no wl_output can report, so a typo or an +// empty argument cannot persist an entry in wayland.json that nothing will ever +// match. +func validateOutputName(name string) error { + switch { + case name == "": + return errors.New("icc: output name is required") + case strings.TrimSpace(name) != name: + return fmt.Errorf("icc: output name %q has surrounding whitespace", name) + } + return nil +} + +// validateICCProfilePath rejects paths that cannot be resolved later: the path +// is persisted and read again on the next start, when the daemon no longer has +// the working directory the caller had. +func validateICCProfilePath(path string) error { + switch { + case path == "": + return errors.New("icc: profile path is required") + case !filepath.IsAbs(path): + return fmt.Errorf("icc: profile path %q must be absolute", path) + } + return nil +} + // ApplyICC loads and applies an ICC profile to a specific output. // outputName is the wl_output name (e.g., "DP-1"). // iccPath is the path to the .icm/.icc profile file. func (m *Manager) ApplyICC(outputName, iccPath string) error { + if err := validateOutputName(outputName); err != nil { + return err + } + if err := validateICCProfilePath(iccPath); err != nil { + return err + } + // 1. Parse and validate the ICC profile profile, err := icc.ParseFile(iccPath) if err != nil { @@ -1624,6 +1659,10 @@ func (m *Manager) ApplyICC(outputName, iccPath string) error { // RemoveICC removes the ICC profile from a specific output, reverting to temperature-based gamma. func (m *Manager) RemoveICC(outputName string) error { + if err := validateOutputName(outputName); err != nil { + return err + } + m.post(func() { defer m.updateStateFromSchedule() @@ -1745,6 +1784,9 @@ func (m *Manager) ListOutputs() []string { // SetOutputTemp sets a per-output color temperature (1000K-10000K). // A value of 0 resets to the global default. func (m *Manager) SetOutputTemp(outputName string, temp int) error { + if err := validateOutputName(outputName); err != nil { + return err + } if temp != 0 && (temp < 1000 || temp > 10000) { return fmt.Errorf("temperature %d out of range (1000-10000)", temp) } diff --git a/core/internal/server/wayland/manager_test.go b/core/internal/server/wayland/manager_test.go index e77f60ddb..5703d36b6 100644 --- a/core/internal/server/wayland/manager_test.go +++ b/core/internal/server/wayland/manager_test.go @@ -915,3 +915,30 @@ func TestManager_SetOutputTempWithoutControlsPublishesValue(t *testing.T) { } assert.Equal(t, 7000, saved.OutputTemps["DP-1"], "and persisted to the config file") } + +// Arguments that no wl_output or file can satisfy are rejected before anything is +// written: an empty name would persist an entry in wayland.json that nothing can +// match, and a relative profile path cannot be resolved by the next start, when +// the daemon no longer has the caller's working directory. +func TestManager_RejectsInvalidICCArguments(t *testing.T) { + m := &Manager{config: DefaultConfig()} + + t.Run("empty output name", func(t *testing.T) { + assert.Error(t, m.ApplyICC("", "/tmp/profile.icc")) + assert.Error(t, m.RemoveICC("")) + assert.Error(t, m.SetOutputTemp("", 7000)) + assert.Empty(t, m.config.ICCProfiles, "nothing may be stored for a name that cannot exist") + assert.Empty(t, m.config.OutputTemps) + }) + + t.Run("output name with whitespace", func(t *testing.T) { + assert.Error(t, m.SetOutputTemp(" DP-1", 7000)) + assert.Empty(t, m.config.OutputTemps) + }) + + t.Run("relative profile path", func(t *testing.T) { + assert.Error(t, m.ApplyICC("DP-1", "profile.icc")) + assert.Error(t, m.ApplyICC("DP-1", "")) + assert.Empty(t, m.config.ICCProfiles) + }) +}