Bug 3603 - [BROKEN BUG] XcursorScanTheme does not handle circular icon theme inheritence.
Summary: [BROKEN BUG] XcursorScanTheme does not handle circular icon theme inheritence.
Status: RESOLVED FIXED
Alias: None
Product: xorg
Classification: Unclassified
Component: Lib/Xcursor (show other bugs)
Version: unspecified
Hardware: Other Linux (All)
: high normal
Assignee: Keith Packard
QA Contact: Xorg Project Team
URL:
Whiteboard: 2011BRB_Reviewed
Keywords: patch
Depends on:
Blocks:
 
Reported: 2005-06-22 17:20 UTC by Daniel Stone
Modified: 2018-05-13 04:13 UTC (History)
2 users (show)

See Also:
i915 platform:
i915 features:


Attachments
Fix for handling themes with circular dependency (637 bytes, patch)
2017-01-22 12:23 UTC, Philipp Ludwig
no flags Details | Splinter Review
Updated patch with proper indentation. (1.08 KB, patch)
2017-01-22 13:24 UTC, Philipp Ludwig
no flags Details | Splinter Review

Description FreeDesktop Bugzilla Database Corruption Fix User 2005-06-22 17:20:09 UTC
If your icon themes have circular inheritance, XcursorScanTheme recurses  
infinitely (until it eventually stack-overflows).   
    
We noticed this bug because the KDE mouse control panel dies when you try to  
launch it (if you have circular icon theme inheritence) -- the rest of KDE  
works fine.   
   
I hacked up a little patch that checks for circular Inherits. A quick overview  
of my patch:   
   
 ~ renamed XcursorScanTheme to XcursorScanThemeAccum and added a third  
parameter, seen, which is a list of all the themes that have already been  
checked.   
   
 ~ Added a new XcursorScanTheme that is a wrapper around XcursorScanThemeAccum  
so that the existing calls to XcursorScanTheme do not have to be changed. (To  
keep the patch simple).   
   
 ~ Only recursively call XcursorScanThemeAccum if the inherited theme has not  
already been checked.   
   
 ~ Note that I prepend new themes to the list, rather than append them, because  
it flows naturally with the recursion that way.   
  
This is the patch that we use internally, I am sure a few changes would be  
needed if it was applied to the official source. Please feel free to email if 
you have questions or flames. 
    
$ tla changes --diffs    
* looking for tos@lindows.com--2004/xcursor--circular-inherits--1.1.3--base-0    
to compare with    
* comparing to tos@lindows.com--2004/xcursor--circular-inherits--1.1.3--base-0    
M  library.c    
M  xcursorint.h    
    
* modified files    
    
--- orig/library.c    
+++ mod/library.c    
@@ -196,10 +196,26 @@    
     return result;    
 }    
     
+int ListMem(StringList *seen, const char *theme)    
+{    
+    if (!seen)    
+	return 0;    
+    
+    while (seen->next != 0) {    
+	printf("(%s,%s)\n",seen->theme,theme);    
+	if (strcmp(seen->theme, theme) == 0)    
+	    return 1;    
+	seen=seen->next;    
+    }    
+    if (strcmp(seen->theme, theme) == 0)    
+	return 1;    
+    return 0;    
+}    
+    
 #define XCURSOR_SCAN_CORE   ((FILE *) 1)    
     
 static FILE *    
-XcursorScanTheme (const char *theme, const char *name)    
+XcursorScanThemeAccum (const char *theme, const char *name, StringList *seen)    
 {    
     FILE	*f = 0;    
     char	*full;    
@@ -248,12 +264,28 @@    
      * Recurse to scan inherited themes    
      */    
     for (i = inherits; i && f == 0; i = _XcursorNextPath (i))    
-	f = XcursorScanTheme (i, name);    
+    {    
+	if (!ListMem(seen, i))    
+	{    
+	    StringList *newSeen = (struct list_struct    
*)malloc(sizeof(StringList));    
+	    newSeen->next  = seen;    
+	    newSeen->theme = i;    
+	    f = XcursorScanThemeAccum (i, name, newSeen);    
+	    free(newSeen);    
+	}    
+    }    
+    
     if (inherits)    
 	free (inherits);    
     return f;    
 }    
     
+static FILE *    
+XcursorScanTheme (const char *theme, const char *name)    
+{    
+    return (XcursorScanThemeAccum (theme, name, 0));    
+}    
+    
 XcursorImage *    
 XcursorLibraryLoadImage (const char *file, const char *theme, int size)    
 {    
    
    
--- orig/xcursorint.h    
+++ mod/xcursorint.h    
@@ -104,5 +104,12 @@    
     
 Cursor    
 _XcursorCreateFontCursor (Display *dpy, unsigned int shape);    
+    
+typedef struct _StringList    
+{    
+    const char *theme;    
+    struct _StringList *next;    
+} StringList;    
+    
         
 #endif /* _XCURSORINT_H_ */
Comment 1 FreeDesktop Bugzilla Database Corruption Fix User 2005-06-22 17:22:34 UTC
oops, I forgot the drop this debug line: 
 
+       printf("(%s,%s)\n",seen->theme,theme); 
 
Comment 2 Keith Packard 2005-06-22 19:18:43 UTC
Why not just a simple max link depth?  That's a whole lot simpler and should be
sufficient.
Comment 3 FreeDesktop Bugzilla Database Corruption Fix User 2005-06-23 11:30:34 UTC
Short Answer:  
 
That works for me. 
 
Long Answer: 
 
The solution I presented is the simplest solution I could think of that could 
detect loops of arbitrary size. (It also roughly matches what KDE does). 
 
Unless, I misunderstand, the max depth solution will just guess that it is in a 
loop and bail out. I personally hate picking arbitrary limits, so I took the 
"easy" way out... 
 
On Linspire, we intentionally have the inheritance loop, clear-e <- crystalsvg 
<- hicolor <- clear-e <- ... If a user installs some other icon theme that 
inherits hicolor, then they would have the loop, new-theme <- hicolor <- 
clear-e <- crystalsvg <- hicolor <- ... 
 
So, as long as MAX_DEPTH is at least 4, that should work for us for now. The 
loop seems to recurse very quickly, so I think even a relatively large 
MAX_DEPTH will not have any significant performance impact... 
 
 
 
Comment 4 Benjamin Close 2008-01-11 02:37:03 UTC
Bugzilla Upgrade Mass Bug Change

NEEDSINFO state was removed in Bugzilla 3.x, reopening any bugs previously listed as NEEDSINFO.

  - benjsc
    fd.o Wrangler
Comment 5 Jeremy Huddleston Sequoia 2011-09-24 23:33:25 UTC
Is this still applicable?  If so, let's fix this.
Comment 6 Jan de Groot 2014-06-05 09:35:28 UTC
This is still an issue. I recently added a fallback to Adwaita for the default theme, same thing that Fedora has been doing for a while.

What happens now is that once a cursor is missing in the Adwaita theme, libxcursor will recurse until death like described in the bugreport. This is reported by our Archlinux users at https://bugs.archlinux.org/task/40658
Comment 7 Jan de Groot 2014-06-10 15:53:08 UTC
Diagnosed the problem a bit further, libxcursor will not loop when the default theme inherits some other theme. What goes into a loop is users who symlink /usr/share/icons/Adwaita to /usr/share/icons/default and a package manager that puts index.theme in there, causing Adwaita to inherit Adwaita.

libxcursor could add guards against this, but it's a typical user configuration error.
Comment 8 Philipp Ludwig 2017-01-22 12:23:51 UTC
Created attachment 129094 [details] [review]
Fix for handling themes with circular dependency

Attached is a recommendation for a patch which fixes this bug.

I recently ran into this bug. I agree that this is caused by a configuration error by the user, however, this misconfiguration rendered me to be not be able to open any application which uses libXcursor - which are, as you can imagine, quite many.

I think especially new users, who try to customize their desktop by some instructions found online, could be thrown-off by this bug, leaving them unable to use their system. And to be honest, which non tech-savvy user would think that her or his invalid cursor theme config would cause all applications to crash?

Therefore I propose to include the attached patch or add a similar guard to avoid this.
Comment 9 Philipp Ludwig 2017-01-22 13:24:52 UTC
Created attachment 129095 [details] [review]
Updated patch with proper indentation.
Comment 10 Alan Coopersmith 2018-05-13 04:13:31 UTC
Thanks, pushed to git master:

To ssh://git.freedesktop.org/git/xorg/lib/libXcursor
   4828abe..f64a8cc  master -> master


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.