Date: Sat, 22 Sep 2007 14:58:52 -0500 (CDT) From: "Sean C. Farley" <scf@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: =?ISO-8859-15?Q?Dag-Erling_Sm=F8rgrav?= <des@des.no>, src-committers@FreeBSD.org, cvs-all@FreeBSD.org, cvs-src@FreeBSD.org Subject: Re: cvs commit: src/lib/libc/stdlib getenv.c Message-ID: <alpine.BSF.0.9999.0709220905390.52204@thor.farley.org> In-Reply-To: <20070922202914.B90809@besplex.bde.org> References: <200709220230.l8M2UiRK020609@repoman.freebsd.org> <86r6krqbrd.fsf@ds4.des.no> <20070922202914.B90809@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--0-1449508764-1190491117=:52204
Content-Type: TEXT/PLAIN; CHARSET=UTF-8; FORMAT=flowed
Content-Transfer-Encoding: 8BIT
Content-ID: <alpine.BSF.0.9999.0709221458431.52204@thor.farley.org>
On Sat, 22 Sep 2007, Bruce Evans wrote:
> On Sat, 22 Sep 2007, [utf-8] Dag-Erling Smørgrav wrote:
>
>> Sean Farley <scf@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@FreeBSD.org
--0-1449508764-1190491117=:52204--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.0.9999.0709220905390.52204>