Hello all, Please find attached a new patch (replacing my previous) with Tamas' feedback.
Tamas, see my answers below. On Monday 25 July 2011 19:01, Tamas TEVESZ wrote: > On Sun, 17 Jul 2011, Christophe CURIS wrote: > > hi, > > > --- a/WINGs/proplist.c > > +++ b/WINGs/proplist.c > > @@ -1677,7 +1677,7 @@ int wmkdirhier(const char *path) > > while (p++ < plen && thePath[p] != '/') > > ; > > > > - strncpy(buf, thePath, p); > > + strncpy(buf, thePath, p); // TODO: This is not great. No > guarranty > > that p < sizeof(buf) > > are you sure about this? i've been tryng to get back in the mindset i > was in when i wrote this (not much success, though), but i don't see > where or how can this overflow. can you describe a situation when it > does? I will re-look at this to see what I had in mind when I added the comment and let you know. (Although if you don't see anything coming, that may just mean it was too late in the night when I first added the comment ;-) ) > > --- a/WINGs/wcolorpanel.c > > +++ b/WINGs/wcolorpanel.c > > @@ -3400,9 +3400,8 @@ char *generateNewFilename(char *curName) > > i have no idea what this function really does, but i'm pretty sure > that > > - it should be static (its only caller is) > - i really don't like the look of it ;) I would tend to agree with you on this... > what does this _do_ and how is it's result actually used? Will have a look on my side, if I find something I'll share my finds. > > --- a/src/defaults.c > > +++ b/src/defaults.c > > @@ -2006,6 +2006,7 @@ static int getKeybind(WScreen * scr, WDefaultEntry > > * entry, WMPropList * value, } > > > > strncpy(buf, val, MAX_SHORTCUT_LENGTH); > > + buf[MAX_SHORTCUT_LENGTH-1] = '\0'; > > please don't. convert src/ to use wstrlcpy (and wstrlcat, where > applicable). Sounds so sane now that you can see it in the new patch! Thanks for your feedback, Regards, Christophe.
From 868ebc40f19289ab8b68d18bb67764732f802a96 Mon Sep 17 00:00:00 2001 From: Christophe CURIS <[email protected]> Date: Fri, 29 Jul 2011 22:38:09 +0200 Subject: [PATCH] Fix possible missing NUL at end of string There are were a few uses of 'strncpy' that could lead to a missing NUL, resulting in possible garbage being displayed. As suggested by Tamas, use 'wstrlcpy' instead --- WINGs/wcolorpanel.c | 3 +-- src/defaults.c | 2 +- src/rootmenu.c | 2 +- src/usermenu.c | 4 ++-- src/workspace.c | 2 +- 5 files changed, 6 insertions(+), 7 deletions(-) diff --git a/WINGs/wcolorpanel.c b/WINGs/wcolorpanel.c index 7ebe8ba..c86dfdd 100644 --- a/WINGs/wcolorpanel.c +++ b/WINGs/wcolorpanel.c @@ -3400,9 +3400,8 @@ char *generateNewFilename(char *curName) newName = wmalloc(baseLen + 16); strncpy(newName, curName, baseLen); - newName[baseLen] = 0; - sprintf(&newName[baseLen], " {%i}", n + 1); + snprintf(&newName[baseLen], 16, " {%i}", n + 1); return newName; } diff --git a/src/defaults.c b/src/defaults.c index 300a0bb..dab5e64 100644 --- a/src/defaults.c +++ b/src/defaults.c @@ -2005,7 +2005,7 @@ static int getKeybind(WScreen * scr, WDefaultEntry * entry, WMPropList * value, return True; } - strncpy(buf, val, MAX_SHORTCUT_LENGTH); + wstrlcpy(buf, val, MAX_SHORTCUT_LENGTH); b = (char *)buf; diff --git a/src/rootmenu.c b/src/rootmenu.c index 5592f73..0ecd3b4 100644 --- a/src/rootmenu.c +++ b/src/rootmenu.c @@ -424,7 +424,7 @@ static Bool addShortcut(char *file, char *shortcutDefinition, WMenu * menu, WMen ptr = wmalloc(sizeof(Shortcut)); - strncpy(buf, shortcutDefinition, MAX_SHORTCUT_LENGTH); + wstrlcpy(buf, shortcutDefinition, MAX_SHORTCUT_LENGTH); b = (char *)buf; /* get modifiers */ diff --git a/src/usermenu.c b/src/usermenu.c index 0339bbb..49473c8 100644 --- a/src/usermenu.c +++ b/src/usermenu.c @@ -157,9 +157,9 @@ static WUserMenuData *convertShortcuts(WScreen * scr, WMPropList * shortcut) for (i = 0, j = 0; i < keycount; i++) { data->key[j].modifier = 0; if (WMIsPLArray(shortcut)) { - strncpy(buf, WMGetFromPLString(WMGetFromPLArray(shortcut, i)), MAX_SHORTCUT_LENGTH); + wstrlcpy(buf, WMGetFromPLString(WMGetFromPLArray(shortcut, i)), MAX_SHORTCUT_LENGTH); } else { - strncpy(buf, WMGetFromPLString(shortcut), MAX_SHORTCUT_LENGTH); + wstrlcpy(buf, WMGetFromPLString(shortcut), MAX_SHORTCUT_LENGTH); } b = (char *)buf; diff --git a/src/workspace.c b/src/workspace.c index b726f7f..c0493f3 100644 --- a/src/workspace.c +++ b/src/workspace.c @@ -749,7 +749,7 @@ void wWorkspaceMenuUpdate(WScreen * scr, WMenu * menu) i = scr->workspace_count - (menu->entry_no - 2); ws = menu->entry_no - 2; while (i > 0) { - strncpy(title, scr->workspaces[ws]->name, MAX_WORKSPACENAME_WIDTH); + wstrlcpy(title, scr->workspaces[ws]->name, MAX_WORKSPACENAME_WIDTH); entry = wMenuAddCallback(menu, title, switchWSCommand, (void *)ws); entry->flags.indicator = 1; -- 1.7.2.5
