May I vote against this patch?
Comments below...
----- David Maciejak <david.macie...@gmail.com> 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?
--
To unsubscribe, send mail to wmaker-dev-unsubscr...@lists.windowmaker.org.
