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

[Sean C. Farley]

On Sat, 22 Sep 2007, Bruce Evans wrote:

> On Sat, 22 Sep 2007, [utf-8] Dag-Erling Smørgrav wrote:
>
>> Sean Farley <scf at FreeBSD.org> writes:
>>>   Log:
>>>   The precision for a string argument in a call to warnx() needs to
>>>   be cast to an int to remove the warning from using a size_t
>>>   variable on 64-bit platforms.
>> 
>> s/to remove the warning/to actually work/

I do agree with this; I consider warnings to be nests for bugs to hide.
I also dislike casts in general since they can hide bugs too.

This is why I wish the specification for the printf() family of
functions, which warnx uses, required the precision to be size_t instead
of int to match the output type of strlen().  Unfortunately, it would be
ssize_t since it accepts negative values.

> Please be precise :-).
>
> s/to remove the warning ... on 64-bit platforms/to avoid undefined
> behaviour on platforms where size_t is not u_int, and to avoid having
> to make a delicate analysis to show that the behaviour is defined and
> correct on all other platforms/.

Thank you for the analysis below.  You are a wealth of
standard/specification knowledge.

> Delicate analysis:
> - size_t is always an unsigned type, but the required type is int, so
>   size_t is never compatible with the required type.
> - on platforms where size_t is smaller than int, the arg type is
>   nevertheless compatible with int, since warnx() is variadic and the
>   arg is one of the variadic args; the default promotions thus apply
>   and the arg is passed as an int whether or not you cast it
>   explicitly to int (but casting it to a type larger than int would
>   break it).  FreeBSD doesn't support any platforms in this class.
> - on platforms where size_t is u_int, the arg is passed as a u_int.
>   The analysis for this case is too delicate to give in full here.
> 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.

>  - the behaviour seems to have been undefined in C90, since va_arg()
>    requires strict type compatibility in C90 and warnx() is
>    implemented using va_arg(ap, int) which gave UB on u_int's.
>    Similarly for function calls, except the wording is less
>    clear/strict.
>  - UB in C90 was a bug in C90.  This is fixed in C99.  Now both
>    va_arg() and function call args are specifically required to work
>    if one type is a signed integer type, the [promotion of the] other
>    type is the corresponding unsigned integer type, and the value is
>    representable in both types.  Compatibility of the representation
>    of integers and unsigned integers probably also requires this, but
>    the specification of this in C90 is probably to fuzzy to override
>    the parts that specify UB.  Everyone just knows that this case has
>    to work.

I must be slow today; it took me awhile to see UB as undefined behavior.

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
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.

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

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;
  		}

Sean
-- 
scf at FreeBSD.org