From: Anselm R Garbe [garb...@gmail.com]
Sent: Wednesday, September 6, 2017 7:38 PM
To: hackers mail list
Subject: Re: [hackers] [dwm][PATCH] move config data to read-only sections
On 6 September 2017 at 19:03, <joachim.he...@t-systems.com> wrote:
> From: Hiltjo Posthuma [hil...@codemadness.org]
> Sent: Wednesday, September 6, 2017 5:32 PM
> To: hackers mail list
> Subject: Re: [hackers] [dwm][PATCH] move config data to read-only sections
>
> On Wed, Sep 06, 2017 at 05:07:06PM +0200, Anselm R Garbe wrote:
>> Hi Joachim,
>>
>> On 6 September 2017 at 17:02, <joachim.he...@t-systems.com> wrote:
>> > commit 6a5056d4c919bb5ae0222b2fde0ed787d50092cf
>> > Author: Joachim Henke <joachim.he...@t-systems.com>
>> > AuthorDate: Wed, 6 Sep 2017 16:26:42 +0200
>> >
>> > The configuration data is just used read-only. So making it immutable
>> > might improve security. Testing on x86_64 showed that the .data section
>> > shrunk considerably: by ~2500 bytes.
>> >
>> > diff --git a/config.def.h b/config.def.h
>> > index a9ac303..a43a03c 100644
>> > --- a/config.def.h
>> > +++ b/config.def.h
>> > @@ -5,14 +5,14 @@ static const unsigned int borderpx = 1; /*
>> > border pixel of windows */
>> > static const unsigned int snap = 32; /* snap pixel */
>> > static const int showbar = 1; /* 0 means no bar */
>> > static const int topbar = 1; /* 0 means bottom bar */
>> > -static const char *fonts[] = { "monospace:size=10" };
>> > +static const char *const fonts[] = { "monospace:size=10" };
>> > static const char dmenufont[] = "monospace:size=10";
>> > static const char col_gray1[] = "#222222";
>> > static const char col_gray2[] = "#444444";
>> > static const char col_gray3[] = "#bbbbbb";
>> > static const char col_gray4[] = "#eeeeee";
>> > static const char col_cyan[] = "#005577";
>> > -static const char *colors[][3] = {
>> > +static const char *const colors[][3] = {
>> > /* fg bg border */
>> > [SchemeNorm] = { col_gray3, col_gray1, col_gray2 },
>> > [SchemeSel] = { col_gray4, col_cyan, col_cyan },
>> > @@ -56,10 +56,10 @@ static const Layout layouts[] = {
>> >
>> > /* commands */
>> > static char dmenumon[2] = "0"; /* component of dmenucmd, manipulated in
>> > spawn() */
>> > -static const char *dmenucmd[] = { "dmenu_run", "-m", dmenumon, "-fn",
>> > dmenufont, "-nb", col_gray1, "-nf", col_gray3, "-sb", col_cyan, "-sf",
>> > col_gray4, NULL };
>> > -static const char *termcmd[] = { "st", NULL };
>> > +static const char *const dmenucmd[] = { "dmenu_run", "-m", dmenumon,
>> > "-fn", dmenufont, "-nb", col_gray1, "-nf", col_gray3, "-sb", col_cyan,
>> > "-sf", col_gray4, NULL };
>> > +static const char *const termcmd[] = { "st", NULL };
>> >
>> > -static Key keys[] = {
>> > +static const Key keys[] = {
>> > /* modifier key function
>> > argument */
>> > { MODKEY, XK_p, spawn, {.v =
>> > dmenucmd } },
>> > { MODKEY|ShiftMask, XK_Return, spawn, {.v =
>> > termcmd } },
>> > @@ -98,7 +98,7 @@ static Key keys[] = {
>> >
>> > /* button definitions */
>> > /* click can be ClkLtSymbol, ClkStatusText, ClkWinTitle, ClkClientWin, or
>> > ClkRootWin */
>> > -static Button buttons[] = {
>> > +static const Button buttons[] = {
>> > /* click event mask button function
>> > argument */
>> > { ClkLtSymbol, 0, Button1, setlayout,
>> > {0} },
>> > { ClkLtSymbol, 0, Button3, setlayout,
>> > {.v = &layouts[2]} },
>> > diff --git a/drw.c b/drw.c
>> > index 319eb6b..902976f 100644
>> > --- a/drw.c
>> > +++ b/drw.c
>> > @@ -153,7 +153,7 @@ xfont_free(Fnt *font)
>> > }
>> >
>> > Fnt*
>> > -drw_fontset_create(Drw* drw, const char *fonts[], size_t fontcount)
>> > +drw_fontset_create(Drw* drw, const char *const fonts[], size_t fontcount)
>> > {
>> > Fnt *cur, *ret = NULL;
>> > size_t i;
>> > @@ -194,7 +194,7 @@ drw_clr_create(Drw *drw, XftColor *dest, const char
>> > *clrname)
>> > /* Wrapper to create color schemes. The caller has to call free(3) on the
>> > * returned color scheme when done using it. */
>> > Scm
>> > -drw_scm_create(Drw *drw, const char *clrnames[], size_t clrcount)
>> > +drw_scm_create(Drw *drw, const char *const clrnames[], size_t clrcount)
>> > {
>> > size_t i;
>> > Scm ret;
>> > diff --git a/drw.h b/drw.h
>> > index ff4355b..2de6a6f 100644
>> > --- a/drw.h
>> > +++ b/drw.h
>> > @@ -32,14 +32,14 @@ void drw_resize(Drw *drw, unsigned int w, unsigned int
>> > h);
>> > void drw_free(Drw *drw);
>> >
>> > /* Fnt abstraction */
>> > -Fnt *drw_fontset_create(Drw* drw, const char *fonts[], size_t fontcount);
>> > +Fnt *drw_fontset_create(Drw* drw, const char *const fonts[], size_t
>> > fontcount);
>> > void drw_fontset_free(Fnt* set);
>> > unsigned int drw_fontset_getwidth(Drw *drw, const char *text);
>> > void drw_font_getexts(Fnt *font, const char *text, unsigned int len,
>> > unsigned int *w, unsigned int *h);
>> >
>> > /* Colorscheme abstraction */
>> > void drw_clr_create(Drw *drw, XftColor *dest, const char *clrname);
>> > -Scm drw_scm_create(Drw *drw, const char *clrnames[], size_t clrcount);
>> > +Scm drw_scm_create(Drw *drw, const char *const clrnames[], size_t
>> > clrcount);
>> >
>> > /* Cursor abstraction */
>> > Cur *drw_cur_create(Drw *drw, int shape);
>>
>> I like your patch, at first glance it makes config.h more consistent
>> with the other const declarations, however I need to browse through
>> the patches if there were any places, where the non-const declaration
>> was on purpose for some reason prior to accepting this upstream.
>
> Please no const char *const. I'm ok with the other const changes though.
Nice spot! I only glanced at it and didn't notice the pointer
qualifiers. This syntax really sucks from a readability point of view.
> Well, C might not have the most beautiful syntax in this regard, but I see no
> other way to make a pointer itself read-only (and especially the ...cmd[]
> pointer arrays really should be). Would using a typedef for that be more
> comfortable to you?
Are you suggesting to typedef const char *const coChar; or something
similar and then using coChar in those places? I would still prefer
the old way for readability reasons. There is no real impact either
imho.
BR,
Anselm
In information security, it is good practice to use the principle of least
privilege. Setting data 'const' could not only help to spot future programming
errors, but essentially keeps this data in a read-only memory page during
run-time! While I agree that the C syntax for read-only pointers is not that
readable, I don't understand, that you're seriously trading this against
security.
Regarding 'typedef' I had something like the following in mind (you might want
to use another name):
typedef const char *stringPtr;
Then a declaration could look like this:
static const stringPtr termcmd[] = { "st", NULL };
Maybe not the most beautiful, but IMHO it makes clear that this is an array of
constant string pointers. – Both, gcc and clang, then properly put this in
read-only ELF sections.
Regards,
Jo.
