Ah, good catch. I've attached a cleanup patch for that. Thomas
On Wed, Sep 23, 2015 at 11:38:02AM -0700, Jasper St. Pierre wrote: > I was imagining that it might be used like: > > char *foo = ExpandFilename(name); > ... > if (foo != name) > free(foo); > > ... which would still work, but the != is now dead code. If callers > were unconditionally freeing before, I heavily suspect something more > serious here. > > On Wed, Sep 23, 2015 at 9:30 AM, Thomas Klausner <[email protected]> wrote: > > Well, yes, but for context, here is the full function after the change: > > > > char * > > ExpandFilename(const char *name) > > { > > char *newname; > > > > if (name[0] != '~') return strdup(name); > > > > newname = malloc (HomeLen + strlen(name) + 2); > > if (!newname) { > > fprintf (stderr, > > "%s: unable to allocate %ld bytes to expand filename > > %s/%s\n", > > ProgramName, HomeLen + (unsigned long)strlen(name) + 2, > > Home, &name[1]); > > } else { > > (void) sprintf (newname, "%s/%s", Home, &name[1]); > > } > > > > return newname; > > } > > > > So in other words, now the function is consistent in returning a > > malloc()ed string when it succeeds. > > Thomas > > > > On Wed, Sep 23, 2015 at 08:59:22AM -0700, Jasper St. Pierre wrote: > >> Should also mention that it also adds a strdup -- I feel that the code > >> calling this might conditionally free. Would be nice to double-check > >> callers. > >> > >> On Wed, Sep 23, 2015 at 1:58 AM, Thomas Klausner <[email protected]> wrote: > >> > From: Christos Zoulas <[email protected]> > >> > > >> > Signed-off-by: Thomas Klausner <[email protected]> > >> > --- > >> > src/util.c | 4 ++-- > >> > src/util.h | 2 +- > >> > 2 files changed, 3 insertions(+), 3 deletions(-) > >> > > >> > diff --git a/src/util.c b/src/util.c > >> > index 4b94051..5e8f0db 100644 > >> > --- a/src/util.c > >> > +++ b/src/util.c > >> > @@ -256,11 +256,11 @@ Zoom(Window wf, Window wt) > >> > * \param name the filename to expand > >> > */ > >> > char * > >> > -ExpandFilename(char *name) > >> > +ExpandFilename(const char *name) > >> > { > >> > char *newname; > >> > > >> > - if (name[0] != '~') return name; > >> > + if (name[0] != '~') return strdup(name); > >> > > >> > newname = malloc (HomeLen + strlen(name) + 2); > >> > if (!newname) { > >> > diff --git a/src/util.h b/src/util.h > >> > index 7f4527c..4b2d3a8 100644 > >> > --- a/src/util.h > >> > +++ b/src/util.h > >> > @@ -64,7 +64,7 @@ in this Software without prior written authorization > >> > from The Open Group. > >> > extern void MoveOutline ( Window root, int x, int y, int width, int > >> > height, > >> > int bw, int th ); > >> > extern void Zoom ( Window wf, Window wt ); > >> > -extern char * ExpandFilename ( char *name ); > >> > +extern char * ExpandFilename ( const char *name ); > >> > extern void GetUnknownIcon ( const char *name ); > >> > extern Pixmap FindBitmap ( const char *name, unsigned int *widthp, > >> > unsigned int *heightp ); > >> > -- > >> > 2.5.2 > >> > > >> > _______________________________________________ > >> > [email protected]: X.Org development > >> > Archives: http://lists.x.org/archives/xorg-devel > >> > Info: http://lists.x.org/mailman/listinfo/xorg-devel > >> > >> > >> > >> -- > >> Jasper > >> > > > > -- > Jasper >
>From e8cd9fe47d1458d6a5ea17addb5cef7026742c82 Mon Sep 17 00:00:00 2001 From: Thomas Klausner <[email protected]> Date: Wed, 23 Sep 2015 20:53:24 +0200 Subject: [PATCH:twm 2/2] Adapt callers to ExpandFilename change. It now always allocates memory, so remove some unnecessary checks. While here, improve handling of an error case. --- src/menus.c | 32 ++++++++++++++++++-------------- src/util.c | 4 ++-- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/menus.c b/src/menus.c index e23b5ff..ada9c41 100644 --- a/src/menus.c +++ b/src/menus.c @@ -2020,7 +2020,7 @@ ExecuteFunction(int func, const char *action, Window w, TwmWindow *tmp_win, "%s: unable to open cut file \"%s\"\n", ProgramName, tmp); } - if (ptr != tmp) free (ptr); + free (ptr); } } else { XFree(ptr); @@ -2171,21 +2171,25 @@ ExecuteFunction(int func, const char *action, Window w, TwmWindow *tmp_win, case F_FILE: ptr = ExpandFilename(action); - fd = open(ptr, O_RDONLY); - if (fd >= 0) - { - count = read(fd, buff, MAX_FILE_SIZE - 1); - if (count > 0) - XStoreBytes(dpy, buff, count); + if (ptr) { + fd = open(ptr, O_RDONLY); + if (fd >= 0) + { + count = read(fd, buff, MAX_FILE_SIZE - 1); + if (count > 0) + XStoreBytes(dpy, buff, count); - close(fd); - } - else - { - fprintf (stderr, "%s: unable to open file \"%s\"\n", - ProgramName, ptr); + close(fd); + } + else + { + fprintf (stderr, "%s: unable to open file \"%s\"\n", + ProgramName, ptr); + } + free(ptr); + } else { + fprintf (stderr, "%s: error expanding filename\n", ProgramName); } - if (ptr != action) free(ptr); break; case F_REFRESH: diff --git a/src/util.c b/src/util.c index 5e8f0db..8e9dab9 100644 --- a/src/util.c +++ b/src/util.c @@ -350,7 +350,7 @@ FindBitmap (const char *name, unsigned *widthp, unsigned *heightp) pm = XmuLocateBitmapFile (ScreenOfDisplay(dpy, Scr->screen), bigname, NULL, 0, (int *)widthp, (int *)heightp, &HotX, &HotY); if (pm == None && Scr->IconDirectory && bigname[0] != '/') { - if (bigname != name) free (bigname); + free (bigname); /* * Attempt to find icon in old IconDirectory (now obsolete) */ @@ -367,7 +367,7 @@ FindBitmap (const char *name, unsigned *widthp, unsigned *heightp) pm = None; } } - if (bigname != name) free (bigname); + free (bigname); if (pm == None) { fprintf (stderr, "%s: unable to find bitmap \"%s\"\n", ProgramName, name); -- 2.5.2
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
