Re: [patch] doas(1): use a #define for the environment variable name length limit

[bytevolcano]

On Wed, 05 Apr 2017 15:07:40 -0400
"Ted Unangst"  wrote:

> bytevolc...@safe-mail.net wrote:
> > No functional or user-visible changes here.
> > On that note, where did the "1024" (1023-char) come from? Is there
> > anyone who has environment variables whose name goes near 1023
> > chars?  
> 
> Probably not, but there's not much benefit to be artifically limiting
> here.

I guess in this case it's better than strdup'ing a super short-lived
variable like this. I was thinking in the lines of having such a large
variable on the stack where the space is unused.

> 
> > @@ -95,7 +97,7 @@ createenv(struct rule *rule)
> > if ((eq = strchr(e, '=')) == NULL || eq ==
> > e) continue;
> > len = eq - e;
> > -   if (len > sizeof(name) - 1)
> > +   if (len > VARNAME_MAX)
> > continue;
> > memcpy(name, e, len);
> > name[len] = '\0';  
> 
> I don't like changes like this because if the size ever changes
> again, it's possible for the check to become decoupled.
> 

I see where you are coming from here. Would it be worth just keeping
the "#define" change? The 1024 limit is in 2 places and I can see a
situation where they end up out of sync.

Also I was thinking about this:

if (len > (sizeof(name) - 1))

I guess the C order of operations will not require the inner
parentheses, but the intent seems clearer this way without having to
get caught up with the specifics of C.

Re: [patch] doas(1): use a #define for the environment variable name length limit

[Ted Unangst]

bytevolc...@safe-mail.net wrote:
> No functional or user-visible changes here.
> On that note, where did the "1024" (1023-char) come from? Is there
> anyone who has environment variables whose name goes near 1023 chars?

Probably not, but there's not much benefit to be artifically limiting here.

> @@ -95,7 +97,7 @@ createenv(struct rule *rule)
>   if ((eq = strchr(e, '=')) == NULL || eq == e)
>   continue;
>   len = eq - e;
> - if (len > sizeof(name) - 1)
> + if (len > VARNAME_MAX)
>   continue;
>   memcpy(name, e, len);
>   name[len] = '\0';

I don't like changes like this because if the size ever changes again, it's
possible for the check to become decoupled.

[patch] doas(1): use a #define for the environment variable name length limit

[bytevolcano]

No functional or user-visible changes here.
On that note, where did the "1024" (1023-char) come from? Is there
anyone who has environment variables whose name goes near 1023 chars?

Index: usr.bin/doas/env.c
===
RCS file: /cvs/src/usr.bin/doas/env.c,v
retrieving revision 1.5
diff -u -p -u -r1.5 env.c
--- usr.bin/doas/env.c  15 Sep 2016 00:58:23 -  1.5
+++ usr.bin/doas/env.c  5 Apr 2017 05:36:39 -
@@ -27,6 +27,8 @@
 
 #include "doas.h"
 
+#define VARNAME_MAX 1023
+
 struct envnode {
RB_ENTRY(envnode) node;
const char *key;
@@ -87,7 +89,7 @@ createenv(struct rule *rule)
struct envnode *node;
const char *e, *eq;
size_t len;
-   char name[1024];
+   char name[VARNAME_MAX + 1];
 
e = environ[i];
 
@@ -95,7 +97,7 @@ createenv(struct rule *rule)
if ((eq = strchr(e, '=')) == NULL || eq == e)
continue;
len = eq - e;
-   if (len > sizeof(name) - 1)
+   if (len > VARNAME_MAX)
continue;
memcpy(name, e, len);
name[len] = '\0';
@@ -139,7 +141,7 @@ fillenv(struct env *env, const char **en
struct envnode *node, key;
const char *e, *eq;
const char *val;
-   char name[1024];
+   char name[VARNAME_MAX + 1];
u_int i;
size_t len;
 
@@ -151,7 +153,7 @@ fillenv(struct env *env, const char **en
len = strlen(e);
else
len = eq - e;
-   if (len > sizeof(name) - 1)
+   if (len > VARNAME_MAX)
continue;
memcpy(name, e, len);
name[len] = '\0';