On Tue 27 Sep 2005 at 00:11:48 +0300, Nadav Har'El wrote:
> The first leak appears to be icon names which are allocated (when fetched
> from the WM property) but never freed. The call trace of this allocation
> is:
> GetWMPropertyString /tmp/ctwm-3.7/util.c:3919
> AddWindow /tmp/ctwm-3.7/add_window.c:1098
> HandleMapRequest /tmp/ctwm-3.7/events.c:2372
> DispatchEvent /tmp/ctwm-3.7/events.c:503
> HandleEvents /tmp/ctwm-3.7/events.c:547
> main /tmp/ctwm-3.7/ctwm.c:959
Preliminary analysis: icon names are allocated by either calling
GetWMPropertyString() from AddWindow(), or, if NO_LOCALE is #defined, by
calling XGetWindowProperty(3X11), and then it seems that sometimes the
value produced by it is overwritten and not used (why ?? I suppose this
is a sloppily written test for error).
The former (GetWMPropertyString()) claims that it returns a malloc(3)ed
string. The latter's manpage clains the returned memory must be freed
with XFree(3X11).
The icon names are freed by free_window_names() in events.c (of all
places) (called from HandleDestroyNotify() in events.c. Lots of scary
conditions in there. But in any case, XFree(3X11) is used to free it (if
it happens). I suppose that the same condition on NO_LOCALE should be
there too to use the correct deallocation function.
I see more places where NO_LOCALE is used in a similar way. I would
suggest removing those uses, and always call GetWMPropertyString(),
which can then have the condition on NO_LOCALE instead. Also introduce a
FreeWMPropertyString() which can free the string properly.
I attached a patch that I whipped together while I should have gone to
sleep. Can you test it?
> A second, more significant leak, is title highlight images which are
> allocated but never freed. The call trace of this allocation is:
>
> CreateHighlightWindows /tmp/ctwm-3.7/add_window.c:1718
> CreateWindowTitlebarButtons /tmp/ctwm-3.7/add_window.c:2005
> AddWindow /tmp/ctwm-3.7/add_window.c:1282
> HandleMapRequest /tmp/ctwm-3.7/events.c:2372
> DispatchEvent /tmp/ctwm-3.7/events.c:503
> HandleEvents /tmp/ctwm-3.7/events.c:547
> main /tmp/ctwm-3.7/ctwm.c:959
The result of CreateHighlightWindows() is put in tmp_win->HiliteImage -
which is simply never mentioned in HandleDestroyNotify(). A proper
deletion call should be added.
> These are just two example leaks I found - I can quite easily point to more.
> The question is - is anyone interested that I continue to point to more
> leaks as I find them?
Yes please! I tend to use ctwm for days/weeks on end, so it would be
very useful for me.
> Can someone look into the code that I accuse to be
> leaky, and check whether indeed it is such, and if so, how it can be fixed?
I'll try to have a look at your reports.
> Thanks,
> Nadav.
-Olaf.
--
___ Olaf 'Rhialto' Seibert -- You author it, and I'll reader it.
\X/ rhialto/at/xs4all.nl -- Cetero censeo "authored" delendum esse.
#
# patch "add_window.c"
# from [79ec0cb30571b095df320d19d752aea845ecafa5]
# to [d13b78df83f14382cf250bbc031c364b0040b8a6]
#
# patch "events.c"
# from [f5776d0db26af573cc1704b9d66115765dd0d72b]
# to [d67e4b757a12c3545ec2d81eb9e5a4a21d5d41de]
#
# patch "util.c"
# from [937af1aae5e51088d075a5661574482883d93482]
# to [00554facd8013044f8fcf9d5a9a06e975c094ac0]
#
# patch "util.h"
# from [d30fa13a107095ac626aa8d43f1628beefc283de]
# to [b1db6b221a3b4866cb15480992ab327304657177]
#
--- add_window.c
+++ add_window.c
@@ -207,11 +207,6 @@
unsigned long valuemask; /* mask for create windows */
XSetWindowAttributes attributes; /* attributes for create windows */
int width, height; /* tmp variable */
-#ifdef NO_LOCALE
- Atom actual_type;
- int actual_format;
- unsigned long nitems, bytesafter;
-#endif /* NO_LOCALE */
int ask_user; /* don't know where to put the window */
int gravx, gravy; /* gravity signs for positioning */
int namelen;
@@ -275,11 +270,7 @@
XSelectInput(dpy, tmp_win->w, PropertyChangeMask);
XGetWindowAttributes(dpy, tmp_win->w, &tmp_win->attr);
-#ifndef NO_LOCALE
tmp_win->name = (char*) GetWMPropertyString(tmp_win->w, XA_WM_NAME);
-#else /* NO_LOCALE */
- XFetchName(dpy, tmp_win->w, &tmp_win->name);
-#endif /* NO_LOCALE */
tmp_win->class = NoClass;
XGetClassHint(dpy, tmp_win->w, &tmp_win->class);
FetchWmProtocols (tmp_win);
@@ -1094,14 +1085,9 @@
tmp_win->title_width = tmp_win->attr.width;
-#ifndef NO_LOCALE
tmp_win->icon_name = (char*) GetWMPropertyString(tmp_win->w,
XA_WM_ICON_NAME);
-#else /* NO_LOCALE */
- if (XGetWindowProperty (dpy, tmp_win->w, XA_WM_ICON_NAME, 0L, 200L, False,
- XA_STRING, &actual_type, &actual_format, &nitems,
- &bytesafter,(unsigned char **)&tmp_win->icon_name))
+ if (!tmp_win->icon_name)
tmp_win->icon_name = tmp_win->name;
-#endif /* NO_LOCALE */
if (tmp_win->icon_name == NULL) tmp_win->icon_name = tmp_win->name;
#ifdef CLAUDE
--- events.c
+++ events.c
@@ -1343,27 +1343,35 @@
* XXX - are we sure that nobody ever sets these to another constant (check
* twm windows)?
*/
-#define isokay(v) ((v) && (v) != NoName)
if ((tmp->name == tmp->full_name) && (tmp->name == tmp->icon_name)) {
- if (nukefull && nukename && nukeicon && isokay(tmp->name)) XFree
(tmp->name);
+ if (nukefull && nukename && nukeicon)
+ FreeWMPropertyString(tmp->name);
} else
if (tmp->name == tmp->full_name) {
- if (nukename && nukefull && isokay(tmp->name)) XFree (tmp->name);
- if (nukeicon && isokay(tmp->icon_name)) XFree (tmp->icon_name);
+ if (nukename && nukefull)
+ FreeWMPropertyString(tmp->name);
+ if (nukeicon)
+ FreeWMPropertyString(tmp->icon_name);
} else
if (tmp->name == tmp->icon_name) {
- if (nukename && nukeicon && isokay(tmp->name)) XFree (tmp->name);
- if (nukefull && isokay(tmp->full_name)) XFree (tmp->full_name);
+ if (nukename && nukeicon)
+ FreeWMPropertyString(tmp->name);
+ if (nukefull)
+ FreeWMPropertyString(tmp->full_name);
} else
if (tmp->icon_name == tmp->full_name) {
- if (nukeicon && nukefull && isokay(tmp->icon_name)) XFree
(tmp->icon_name);
- if (nukename && isokay(tmp->name)) XFree (tmp->name);
+ if (nukeicon && nukefull)
+ FreeWMPropertyString(tmp->icon_name);
+ if (nukename)
+ FreeWMPropertyString(tmp->name);
} else {
- if (nukefull && isokay(tmp->full_name)) XFree (tmp->full_name);
- if (nukename && isokay(tmp->name)) XFree (tmp->name);
- if (nukeicon && isokay(tmp->icon_name)) XFree (tmp->icon_name);
+ if (nukefull)
+ FreeWMPropertyString(tmp->full_name);
+ if (nukename)
+ FreeWMPropertyString(tmp->name);
+ if (nukeicon)
+ FreeWMPropertyString(tmp->icon_name);
}
-#undef isokay
return;
}
--- util.c
+++ util.c
@@ -3885,7 +3885,6 @@
}
}
-#ifndef NO_LOCALE
/***********************************************************************
*
* Procedure:
@@ -3904,6 +3903,19 @@
unsigned char *GetWMPropertyString(Window w, Atom prop)
{
+#ifdef NO_LOCALE
+ Atom actual_type;
+ int actual_format;
+ unsigned long nitems, bytesafter;
+ unsigned char *string;
+
+ if (XGetWindowProperty (dpy, w, prop, 0L, 200L, False,
+ XA_STRING, &actual_type, &actual_format, &nitems,
+ &bytesafter, &string) != Success)
+ string = NULL;
+
+ return string;
+#else
XTextProperty text_prop;
char **text_list;
int text_list_count;
@@ -3970,10 +3982,20 @@
}
return stringptr;
+#endif /* NO_LOCALE */
}
+
+void FreeWMPropertyString(unsigned char *prop)
+{
+#ifdef NO_LOCALE
+ if (prop && (char *)prop != NoName)
+ XFree(prop);
+#else
+ if (prop && (char *)prop != NoName)
+ free(prop);
#endif /* NO_LOCALE */
+}
-
static void ConstrainLeftTop (int *value, int border)
{
if (*value < border) {
--- util.h
+++ util.h
@@ -140,9 +140,7 @@
extern Image *GetImage (char *name, ColorPair cp);
-#ifndef NO_LOCALE
extern unsigned char *GetWMPropertyString(Window w, Atom prop);
-#endif /* NO_LOCALE */
extern void ConstrainByBorders1 (int *left, int width, int *top, int height);
extern void ConstrainByBorders (TwmWindow *twmwin,
int *left, int width,
