On Sun, May 18, 2014 at 1:03 AM, Christophe <[email protected]> wrote: > May I vote against this patch? > Comments below... > > > ----- David Maciejak <[email protected]> a écrit : >> [...] >> >> diff --git a/src/dialog.c b/src/dialog.c >> index da126ae..d0d9179 100644 >> --- a/src/dialog.c >> +++ b/src/dialog.c >> @@ -899,7 +899,7 @@ static void keyPressHandler(XEvent * event, void *data) >> >> Bool wIconChooserDialog(WScreen *scr, char **file, const char >> *instance, const char *class) >> { >> - WWindow *wwin; >> + WWindow *wwin = NULL; > > Looking at the code, wwin is always assigned something at this line (1028 on > my side): > wwin = wManageInternalWindow(scr, parent, None, tmp, > center.x, center.y, 450, 280); > > so adding an initializer does not look like it has some effect. I don't know > what cppchecks complaints about, but adding some default value is generally > not a good idea because it hides some problems that the compiler (or > checkers) can detect otherwise. > > >> Window parent; >> IconPanel *panel; >> WMColor *color; >> @@ -1018,10 +1018,10 @@ Bool wIconChooserDialog(WScreen *scr, char >> **file, const char *instance, const c >> >> tmp = wmalloc(len); >> >> - if (tmp && (instance || class)) >> + if (instance || class) >> snprintf(tmp, len, "%s [%s.%s]", _("Icon Chooser"), instance, class); >> else >> - strcpy(tmp, _("Icon Chooser")); >> + strncpy(tmp, _("Icon Chooser"), len); >> > > Temporary fixing code that is otherwise all wrong may not be enough, I would > suggest to take the opportunity to fix: > - if either 'instance' or 'class' is NULL but not the other, the 'snprintf' > can crash due to NULL pointer dereference; > > - strncpy copies at most 'len' bytes... excluding addition of a final '\0'. > so it should be called with 'len - 1' and then explicitly add final '\0'. Or > use WUtil's wstrlcpy which was meant for that purpose? >
i did not know that function, but i completely agree we should use buildin functions when possible. -- To unsubscribe, send mail to [email protected].
