cvs commit: src/lib/libc/stdlib getenv.c

[Bruce Evans]

On Sat, 22 Sep 2007, Sean C. Farley wrote:

> On Sat, 22 Sep 2007, Bruce Evans wrote:
>> ...
>> Partial analysis:
>>  - the size_t variable must have a small value that is representable
>>    as an int (else casting it to int would be a bug and/or printing a
>>    line of that length would be a style bug).
>
> What would be a good maximum that would fit style?  Although still
> fairly big, NL_TEXTMAX for the entire line looks plausible.

79 less the length of all other text on the line :-).

> Would the best solution be to place a cap on the value?  If the value is
> less than INT_MAX, cast it to an int else pass it INT_MAX.  Actually, it

I don't remember where the value comes from.  If it can be from user input
then it needs to be restricted somewhere, to INT_MAX as a last resort

> looks like the value should never be greater than ARG_MAX if wanting to
> be able to call exec since according to SUSv3 that is the:
>    Maximum length of argument to the exec functions including
>    environment data.

ARG_MAX can in theory be enormous (too large to be returned in a long
by sysconf()), but in practice it will be much smaller than INT_MAX.

> Hopefully, no environment variables (name=value string) are anywhere
> close in size to size_t.  :)

Ah I see where the value comes from.  A malicous user could probably
put > INT_MAX bytes in a single string in the environment on machines
with 32-bit ints, 64 bit address space and lots of RAM, and then fork()
but not exec().  That's close enough to user input for me.

> Here is a patch (untested) to at least cast safely.  How does this look?
>
> Index: getenv.c
> ===================================================================
> RCS file: /home/ncvs/src/lib/libc/stdlib/getenv.c,v
> retrieving revision 1.12
> diff -u -r1.12 getenv.c
> --- getenv.c	22 Sep 2007 02:30:44 -0000	1.12
> +++ getenv.c	22 Sep 2007 19:05:51 -0000
> @@ -356,8 +356,8 @@
> 		activeNdx = envVarsTotal - 1;
> 		if (__findenv(envVars[envNdx].name, nameLen, &activeNdx,
> 		    false) == NULL) {
> -			warnx(CorruptEnvFindMsg, (int)nameLen,
> -			    envVars[envNdx].name);
> +			warnx(CorruptEnvFindMsg, nameLen > INT_MAX ? INT_MAX
> :
> +			    (int)nameLen, envVars[envNdx].name);
> 			errno = EFAULT;
> 			goto Failure;
> 		}

OK (I think one line is only too long/split due to misquoting).

A more refined version would use something like strvis(), and could
use a smaller limit (with long corrupt strings indicated something
likje debuggers print long binary strings) since this this is only
debugging code, but *env.c is already too large for me.

Bruce