Date: Wed, 2 Dec 2009 12:16:03 -0500 From: Brian Fundakowski Feldman <green@FreeBSD.org> To: "Sean C. Farley" <scf@FreeBSD.org> Cc: src-committers@FreeBSD.org, svn-src-all@FreeBSD.org, rwatson@FreeBSD.org, cperciva@FreeBSD.org, svn-src-head@FreeBSD.org, "M. Warner Losh" <imp@bsdimp.com> Subject: Re: svn commit: r199983 - in head: lib/libc/stdlib tools/regression/environ Message-ID: <20091202171603.GE35660@green.homeunix.org> In-Reply-To: <alpine.BSF.2.00.0912020905440.43469@thor.farley.org> References: <200912010504.nB154VnS053167@svn.freebsd.org> <4B14B32C.3060409@freebsd.org> <alpine.BSF.2.00.0912011514510.84941@fledge.watson.org> <20091201.193518.387188323.imp@bsdimp.com> <alpine.BSF.2.00.0912020905440.43469@thor.farley.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Dec 02, 2009 at 09:33:29AM -0600, Sean C. Farley wrote:
> On Tue, 1 Dec 2009, M. Warner Losh wrote:
>
>> In message: <alpine.BSF.2.00.0912011514510.84941@fledge.watson.org>
>> Robert Watson <rwatson@FreeBSD.org> writes:
>> : On Mon, 30 Nov 2009, Colin Percival wrote:
>> :
>> : > Brian Feldman wrote:
>> : >> Do not gratuitously fail *env(3) operations due to corrupt ('='-less)
>> : >> **environ entries. This puts non-getenv(3) operations in line with
>> : >> getenv(3) in that bad environ entries do not cause all operations to
>> : >> fail. There is still some inconsistency in that getenv(3) in the
>> : >> absence of any environment-modifying operation does not emit corrupt
>> : >> environ entry warnings.
>> : >>
>> : >> I also fixed another inconsistency in getenv(3) where updating the
>> : >> global environ pointer would not be reflected in the return values.
>> : >> It would have taken an intermediary setenv(3)/putenv(3)/unsetenv(3)
>> : >> in order to see the change.
>> : >
>> : > The FreeBSD Security Team is currently dealing with a security : >
>> issue relating to this code. Please back out your change (at : > least to
>> getenv.c; I don't particularly care about the regression : > tests) until
>> we've finished, and then submit the patch to us for : > review along with
>> a detailed explanation of what it does.
>> : >
>> : > We've already had two major security issues arising out of : >
>> getenv.c in the past year, and I'd like to make sure we don't have : > a
>> third.
>> :
>> : I think it's fair to say that the POSIXization of the environment : code
>> has been an unmitigated disaster, and speaks to the necessity : for
>> careful review of those sorts of code changes.
>>
>> Why we're not just reverting the whole thing as a bad idea is beyond me.
>> Clearly the tiny incremental benefits have been far overshadowed by this
>> fiasco.
>
> Which "whole thing"? The code or the POSIX-compliance? Technically, it is
> not pure compliance because the code has a few BSD requirements in it such
> as keeping old name=value entries even when new ones are created.
>
> It was my fault for not checking how unsetenv() was used in all of base.
> The change to use unsetenv() in rtld.c was committed just prior to my
> change which introduced a version of unsetenv() that returned an int to
> allow checking.
>
> I am testing a change to have unsetenv() not stop in its attempt to unset a
> variable which should mimic the old behavior.
>
> One difference between our man page and IEEE Std 1003.1-2008 is the part
> concerning that the "environment shall be unchanged" which I will introduce
> to the man page:
>
> Upon successful completion, zero shall be returned. Otherwise, -1
> shall be returned, errno set to indicate the error, and the
> environment shall be unchanged.
>
> After my change, unsetenv() will not return an error if environ is corrupt.
> It will wipe the variable. The only time errors will be returned are when
> the name passed to unsetenv() is invalid or when memory allocation fails.
>
> One question is whether the code should abort() when it detects a corrupt
> environ array or do its best to complete the request from the caller.
The consensus seems to be that either of those two behaviors is okay. I lean
more heavily toward repairing environ and moving on because it seems likely
that as often as not, *env(3) will not occur immediately after (unintentional)
environ corruption and abort() won't make debugging much easier, only make
lives harder. Of course, we could also see what Solaris, Linux, NetBSD and
friends do...
Using abort() and assert() against data provided directly by the user
(that is, data that is not supposed to be already "owned" by the library)
is somewhat evil. It annoys me to no end as a developer when libraries
do that (one of the guiltiest examples being libdbus.)
--
Brian Fundakowski Feldman \'[ FreeBSD ]''''''''''\
<> green@FreeBSD.org \ The Power to Serve! \
Opinions expressed are my own. \,,,,,,,,,,,,,,,,,,,,,,\
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20091202171603.GE35660>