Added color validation for BAR_STATUS2D_PATCH

[lainq]

I noticed that dwm exits the process if the color used in BAR_STATS2D_PATCH prompt is wrong. This is pretty bad since the stext can be set by any program running on the computer with xsetroot -name "...".

If the the prompt contains invalid color data such as hello ^cinvalidcolor^ uwu, drw_clr_create calls die which takes you back to your login manager.

This commit enables validation of the hex code before passing it into drw_clr_create. If the hex code is invalid, the hexcode is defaulted to BAR_STATUS2D_DEFAULT_COLOR which can be set in confg.h

[bakkeby]

If the the prompt contains invalid color data such as hello ^cinvalidcolor^ uwu, drw_clr_create calls die which takes you back to your login manager.

Yes if it calls die then it will exit dwm. You can just change this to a fprintf and it won't exit dwm when this happens. The effect of that is that it will simply keep the existing colour rather than defaulting to black.

[lainq]

Should I consider modifying the functionality or would it be more appropriate to close the pull request?

[bakkeby]

If we consider a bare dwm then we have that the colours are initialised in the setup function, making a call to drw_scm_create.

    /* init appearance */
    scheme = ecalloc(LENGTH(colors), sizeof(Clr *));
    for (i = 0; i < LENGTH(colors); i++)
        scheme[i] = drw_scm_create(drw, colors[i], 3);

drw_scm_create in turn will allocate enough space to hold three colours, then make a call to drw_clr_create for each colour to allocate values based on the names passed.

/* Wrapper to create color schemes. The caller has to call free(3) on the
 * returned color scheme when done using it. */
Clr *
drw_scm_create(Drw *drw, const char *clrnames[], size_t clrcount)
{
    size_t i;
    Clr *ret;

    /* need at least two colors for a scheme */
    if (!drw || !clrnames || clrcount < 2 || !(ret = ecalloc(clrcount, sizeof(XftColor))))
        return NULL;

    for (i = 0; i < clrcount; i++)
        drw_clr_create(drw, &ret[i], clrnames[i]);
    return ret;
}

and drw_clr_create will make a call to XftColorAllocName to parse the given name, and dwm will intentionally call die if the colour cannot be allocated.

void
drw_clr_create(Drw *drw, Clr *dest, const char *clrname)
{
    if (!drw || !dest || !clrname)
        return;

    if (!XftColorAllocName(drw->dpy, DefaultVisual(drw->dpy, drw->screen),
                           DefaultColormap(drw->dpy, drw->screen),
                           clrname, dest))
        die("error, cannot allocate color '%s'", clrname);
}

Now the colour name here can be a hex value, e.g. #005577, but it can also be the name of the colour:

static const char col_cyan[]        = "pink";

If you were to configure dwm with a name that does not work, e.g.

static const char col_cyan[]        = "fubar";

then dwm will exit with the error of:

error, cannot allocate color 'fubar'

As such I believe that we can conclude that any validation of the colour name, including validating hex codes, are being done within the XftColorAllocName function.

With the assumption that a failure to allocate a colour is not a critical issue we can just print a warning instead.

diff --git a/drw.c b/drw.c
index a58a2b4..8e7c010 100644
--- a/drw.c
+++ b/drw.c
@@ -189,7 +189,7 @@ drw_clr_create(Drw *drw, Clr *dest, const char *clrname)
        if (!XftColorAllocName(drw->dpy, DefaultVisual(drw->dpy, drw->screen),
                               DefaultColormap(drw->dpy, drw->screen),
                               clrname, dest))
-               die("error, cannot allocate color '%s'", clrname);
+               fprintf(stderr, "warning, cannot allocate color '%s'", clrname);
 }

Now we have that the call to XftColorAllocName failed. What does that actually mean then? It means that the dest colour data remains unaltered.

Testing this the colour became black in my case because the allocated space was just 0s. Given that the memory location is just allocated and not explicitly written to I suppose that the allocated space may have pre-existing data which may potentially lead to undefined behaviour.

Why does dwm call die in this case? I can only imagine that it is to give some quick feedback about configuration issues where someone has added something wrong in config.h.

In practice this is only beneficial for users using .xinitrc and startx to start dwm, as users using a display manager will just be kicked back out to the login screen.

I think that it makes more sense to just ignore XftColorAllocName failures in the case of status2d colours than to add separate validation as proposed here. It is also worth noting that you run the same risk when using Xresources to override colours.

I would argue that having bad / wrong colour data in the configuration file would be pretty rare compared to scenarios where colours can be dynamically changed during runtime, thus the "benefit" of calling die in this situation is practically negligible.

I can add that change but I would probably do it as a separate toggle rather than bundling it with existing patches (that do not touch this code).

[lainq]

Thank you for the explanation. Cleared up a lot of my doubts.