Fix cursor icon search paths to follow XDG Icon Theme Specification#4731
Fix cursor icon search paths to follow XDG Icon Theme Specification#4731
Conversation
|
@copilot it is poor style to declare variables before they can be initialized |
There was a problem hiding this comment.
Pull request overview
This PR fixes cursor icon search paths in src/miral/xcursor.c to comply with the XDG Icon Theme Specification. Previously, Mir used a hardcoded XCURSORPATH that omitted $XDG_DATA_DIRS/icons. The fix introduces a dynamic path builder that constructs the correct XDG-spec-compliant search order at runtime, falling back to the legacy path.
Changes:
- Added
_XcursorBuildXdgPath()that dynamically constructs the cursor search path per the XDG Icon Theme spec:$HOME/.icons,$XDG_DATA_DIRS/icons(expanded),/usr/share/pixmaps, then legacyXCURSORPATHentries XcursorLibraryPath()updated to call_XcursorBuildXdgPath()whenXCURSOR_PATHenv var is not set, withXCURSORPATHmacro as last resort fallback
Co-authored-by: AlanGriffiths <9048879+AlanGriffiths@users.noreply.github.com>
…lization Co-authored-by: AlanGriffiths <9048879+AlanGriffiths@users.noreply.github.com>
…nly path Co-authored-by: AlanGriffiths <9048879+AlanGriffiths@users.noreply.github.com>
790607b to
5f8ccb4
Compare
| for (char const *p = xdg_data_dirs; p && *p; ) | ||
| { | ||
| char const *colon = strchr (p, ':'); | ||
| int const written = colon | ||
| ? snprintf (p_out, remaining, "%.*s/icons:", (int)(colon - p), p) | ||
| : snprintf (p_out, remaining, "%s/icons:", p); | ||
| p = colon ? colon + 1 : NULL; | ||
| if (written < 0 || (size_t)written >= remaining) goto error; |
There was a problem hiding this comment.
When XDG_DATA_DIRS contains empty components (e.g. "::" or a leading ':'), this loop will emit "/icons:" because (colon - p) can be 0. That adds a bogus search directory. Consider skipping empty components (advance past ':' and continue) before calling snprintf, and apply the same logic in the length-calculation loop above.
Co-authored-by: AlanGriffiths <9048879+AlanGriffiths@users.noreply.github.com> Agent-Logs-Url: https://github.com/canonical/mir/sessions/e1039739-4f60-4451-ac58-bd396023af5d
Agent-Logs-Url: https://github.com/canonical/mir/sessions/7aeed8fc-05c1-4f04-98bb-22ad6f6170de Co-authored-by: AlanGriffiths <9048879+AlanGriffiths@users.noreply.github.com>
| char const *xdg_data_dirs = getenv ("XDG_DATA_DIRS"); | ||
|
|
||
| if (!xdg_data_dirs || xdg_data_dirs[0] == '\0') | ||
| xdg_data_dirs = "/usr/local/share:/usr/share"; | ||
|
|
||
| size_t len = 0; | ||
|
|
||
| /* $HOME/.icons: and $HOME/.cursors: */ | ||
| if (home) | ||
| len += strlen (home) + strlen ("/.icons") + 1 /* +1 for ':' */ | ||
| + strlen (home) + strlen ("/.cursors") + 1; |
There was a problem hiding this comment.
_XcursorBuildXdgPath() builds an XDG-based icon search path from $HOME and $XDG_DATA_DIRS, but it never includes $XDG_DATA_HOME/icons (defaulting to $HOME/.local/share/icons). The XDG Icon Theme search order includes that directory, so cursor themes installed under ~/.local/share/icons won’t be discovered with the generated path. Consider adding an $XDG_DATA_HOME/icons entry (with the correct default when XDG_DATA_HOME is unset) between $HOME/.icons and $XDG_DATA_DIRS/icons in both the length calculation and snprintf construction.
| char const *xdg_data_dirs = getenv ("XDG_DATA_DIRS"); | |
| if (!xdg_data_dirs || xdg_data_dirs[0] == '\0') | |
| xdg_data_dirs = "/usr/local/share:/usr/share"; | |
| size_t len = 0; | |
| /* $HOME/.icons: and $HOME/.cursors: */ | |
| if (home) | |
| len += strlen (home) + strlen ("/.icons") + 1 /* +1 for ':' */ | |
| + strlen (home) + strlen ("/.cursors") + 1; | |
| char const *xdg_data_home = getenv ("XDG_DATA_HOME"); | |
| char const *xdg_data_dirs = getenv ("XDG_DATA_DIRS"); | |
| char *default_xdg_data_home = NULL; | |
| if ((!xdg_data_home || xdg_data_home[0] == '\0') && home) | |
| { | |
| size_t const default_xdg_data_home_len = strlen (home) + strlen ("/.local/share") + 1; | |
| default_xdg_data_home = malloc (default_xdg_data_home_len); | |
| if (!default_xdg_data_home) | |
| return NULL; | |
| snprintf (default_xdg_data_home, default_xdg_data_home_len, "%s/.local/share", home); | |
| xdg_data_home = default_xdg_data_home; | |
| } | |
| if (!xdg_data_dirs || xdg_data_dirs[0] == '\0') | |
| xdg_data_dirs = "/usr/local/share:/usr/share"; | |
| size_t len = 0; | |
| /* $HOME/.icons:, $XDG_DATA_HOME/icons:, and $HOME/.cursors: */ | |
| if (home) | |
| len += strlen (home) + strlen ("/.icons") + 1; /* +1 for ':' */ | |
| if (xdg_data_home) | |
| len += strlen (xdg_data_home) + strlen ("/icons") + 1; | |
| if (home) | |
| len += strlen (home) + strlen ("/.cursors") + 1; |
|
@copilot apply changes based on the comments in this thread Also, the commit body seems to have lost the overall purpose of this PR and just contains the latest change |
Agent-Logs-Url: https://github.com/canonical/mir/sessions/a94cbe37-6168-45a6-bc04-dfdd8d73ea5e Co-authored-by: AlanGriffiths <9048879+AlanGriffiths@users.noreply.github.com>
Done in 3e99acf. |
| { | ||
| fprintf (stderr, "xcursor: XDG_DATA_DIRS component length exceeds INT_MAX\n"); | ||
| abort (); | ||
| } |
There was a problem hiding this comment.
Calling fprintf(stderr, ...) + abort() when an XDG_DATA_DIRS component exceeds INT_MAX makes the compositor/library abort based on untrusted environment input (user-controllable denial-of-service). Prefer handling this case gracefully (e.g., treat it as a build-path failure and return NULL so XcursorLibraryPath() falls back to XCURSORPATH, or skip the offending component) rather than terminating the process.
| { | |
| fprintf (stderr, "xcursor: XDG_DATA_DIRS component length exceeds INT_MAX\n"); | |
| abort (); | |
| } | |
| goto error; |
RAOF
left a comment
There was a problem hiding this comment.
The INT_MAX -> PATH_MAX change is an easy one.
I think it'd be good to factor out the appending code, but I won't block on that.
| p = colon ? colon + 1 : NULL; | ||
| if (component_len == 0) | ||
| continue; | ||
| if (component_len > INT_MAX) |
There was a problem hiding this comment.
I don't know why this is comparing against INT_MAX; being paranoid here is maybe reasonable, but the thing to be comparing against is PATH_MAX.
(I know POSIX doesn't mandate that PATH_MAX exists; it does on Linux and I don't think we care about portability to GNU Hurd)
There was a problem hiding this comment.
Changed to PATH_MAX in 4db18aa. Oversized components are now skipped with a fprintf(stderr, ...) warning rather than aborting.
| int const written = snprintf (p_out, remaining, "%s/.icons:", home); | ||
| if (written < 0 || (size_t)written >= remaining) goto error; | ||
| p_out += written; | ||
| remaining -= (size_t)written; |
There was a problem hiding this comment.
This is crying out to be factored into a function char* format_into_buffer(char* start, char const* end, char const* format, ...) that takes the insertion point, (one past) the end of the buffer, the format string and formatting arguments and returns a pointer to the null byte written or NULL on error (including insufficient buffer space).
Then all these become
if (!(p_out = format_into_buffer(p_out, end, "%s/icons:", xdg_data_home)))
goto error;
And, actually, since this is only called once at startup, we could make this dramatically simpler by just repeatedly reallocing and having the function be char* append_into_buffer(char* buffer, char* end, char const* format, ...). That way we don't have this duplicated, somewhat gnarly code that also needs to be kept in sync with the length calculation above.
There was a problem hiding this comment.
Done in 4db18aa. Introduced _xcursor_path_append(char **path_ptr, char const *str) which realloc-grows the buffer and appends str. The duplicated length-calculation loop is gone — _XcursorBuildXdgPath now just calls _xcursor_path_append for each entry in a single forward pass.
|
@copilot please address the concerns raised in this thread: #4731 (review) |
Agent-Logs-Url: https://github.com/canonical/mir/sessions/a2336e81-0f85-4fe7-980d-77478269e2c9 Co-authored-by: AlanGriffiths <9048879+AlanGriffiths@users.noreply.github.com>
Done in 4db18aa. Both concerns from the review are addressed:
|
|
@copilot the changes to error handling introduced in the last commit are unwanted: if the configuration is incorrect we prefer not to start (i.e. abort) rather than carry on in an unexpected configuration |
Agent-Logs-Url: https://github.com/canonical/mir/sessions/fa656584-a756-4791-9083-8f937c694fac Co-authored-by: AlanGriffiths <9048879+AlanGriffiths@users.noreply.github.com>
Fixed in 1bf5ee4. The |
| /* Temporary buffer for formatted entries; PATH_MAX covers any valid path | ||
| * component and "/icons:" suffix. */ | ||
| char buf[PATH_MAX + 8]; | ||
|
|
||
| if (home) | ||
| { | ||
| snprintf (buf, sizeof (buf), "%s/.icons:", home); | ||
| if (!_xcursor_path_append (&path, buf)) goto done; | ||
| } | ||
|
|
||
| if (xdg_data_home) | ||
| { | ||
| snprintf (buf, sizeof (buf), "%s/icons:", xdg_data_home); | ||
| if (!_xcursor_path_append (&path, buf)) goto done; |
There was a problem hiding this comment.
snprintf() return values are ignored when formatting entries into the fixed-size buf. If $HOME or $XDG_DATA_HOME is long enough to truncate the formatted string, a truncated (and likely invalid) path element will be appended silently, changing the search path in unexpected ways. Consider either (a) checking snprintf() for truncation and treating it as an error (skip/fallback), or (b) avoiding the fixed buffer by appending pieces directly with the realloc-growing helper.
| fprintf (stderr, "xcursor: XDG_DATA_DIRS component length exceeds PATH_MAX\n"); | ||
| abort (); |
There was a problem hiding this comment.
XDG_DATA_DIRS with an oversized component currently triggers fprintf(...); abort();. Because this value is user-controlled environment input, this introduces a new hard-crash path that can take down a Mir-based compositor just by setting an invalid env var. Consider handling this more defensively (e.g. skip the oversized component and continue, or fall back to the default XDG dirs) rather than aborting the process.
| fprintf (stderr, "xcursor: XDG_DATA_DIRS component length exceeds PATH_MAX\n"); | |
| abort (); | |
| fprintf (stderr, "xcursor: ignoring XDG_DATA_DIRS component length exceeding PATH_MAX\n"); | |
| continue; |
Fixes cursor icon search paths in
src/miral/xcursor.cto comply with the XDG Icon Theme Specification. Previously, Mir used a hardcodedXCURSORPATHthat omitted the XDG-specified directories. This introduces a dynamic path builder that constructs the correct XDG-spec-compliant search order at runtime.What's new?
_XcursorBuildXdgPath()that dynamically constructs the cursor search path per the XDG Icon Theme spec in the following order:$HOME/.icons$XDG_DATA_HOME/icons(defaults to$HOME/.local/share/iconswhenXDG_DATA_HOMEis unset or empty)$HOME/.cursors$XDG_DATA_DIRS/iconsentries (defaults to/usr/local/share:/usr/share)/usr/share/pixmapsXCURSORLEGACYPATHentries (/usr/share/cursors/xorg-x11,ICONDIR)XcursorLibraryPath()usesXCURSOR_PATHenv var when set, otherwise calls_XcursorBuildXdgPath(), withXCURSORPATHmacro as last-resort fallbackXDG_DATA_DIRS(e.g. leading/trailing:or::) are skipped to avoid bogus/iconsentriescomponent_len > PATH_MAXguard aborts with a message — an oversized component inXDG_DATA_DIRSis treated as a fatal misconfiguration_xcursor_path_append()realloc-based helper that grows the path buffer on demand — eliminates the duplicated pre-calculation phase, building the path in a single forward passpthread_onceXCURSORLEGACYPATHexcludes paths already covered by the XDG prefix entriesHow to test
~/.local/share/icons/<theme-name>/cursors/and verify it is picked up by a Mir-based compositorXDG_DATA_HOMEto a custom path, install a cursor theme under$XDG_DATA_HOME/icons/, and verify it is foundXCURSOR_PATHto override the path entirely and verify the override is respectedXDG_DATA_DIRSfalls back to/usr/local/share:/usr/shareChecklist