GNU bug report logs - #13070
Use putenv+unsetenv instead of modifying environ directly

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 13070 in the body.
You can then email your comments to 13070 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: bug-gnu-emacs <at> gnu.org
Cc: Eli Zaretskii <eliz <at> gnu.org>, Fabrice Popineau <fabrice.popineau <at> gmail.com>
Subject: Use putenv+unsetenv instead of modifying environ directly
Date: Mon, 03 Dec 2012 12:32:50 -0800

[Message part 1 (text/plain, inline)]

Tags: patch

The attached patch is in response to last week's thread on emacs-devel
<http://lists.gnu.org/archive/html/emacs-devel/2012-11/msg00514.html>.
It's relative to trunk bzr 111078.  Tested on Fedora 17.  I don't see
any issues related to the Microsoft port but I'm CC:ing this to Eli
and Fabrice just in case, as the original bug seems to be Windows-related.

[setenv.txt (text/plain, attachment)]

Message #8 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Fabrice Popineau <fabrice.popineau <at> gmail.com>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: bug-gnu-emacs <at> gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: Use putenv+unsetenv instead of modifying environ directly
Date: Tue, 4 Dec 2012 14:51:06 +0100

[Message part 1 (text/plain, inline)]

Great.

Up to now, it seems to run ok. Provided I added some unsetenv() function
for w32.
(And it is much cleaner that the quick and dirty hack I applied for myself.)

Thanks,

Fabrice

2012/12/3 Paul Eggert <eggert <at> cs.ucla.edu>

> Tags: patch
>
> The attached patch is in response to last week's thread on emacs-devel
> <http://lists.gnu.org/archive/html/emacs-devel/2012-11/msg00514.html>.
> It's relative to trunk bzr 111078.  Tested on Fedora 17.  I don't see
> any issues related to the Microsoft port but I'm CC:ing this to Eli
> and Fabrice just in case, as the original bug seems to be Windows-related.
>

[Message part 2 (text/html, inline)]

Message #11 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Fabrice Popineau <fabrice.popineau <at> gmail.com>
Cc: bug-gnu-emacs <at> gnu.org, eggert <at> cs.ucla.edu
Subject: Re: Use putenv+unsetenv instead of modifying environ directly
Date: Tue, 04 Dec 2012 19:48:46 +0200

> From: Fabrice Popineau <fabrice.popineau <at> gmail.com>
> Date: Tue, 4 Dec 2012 14:51:06 +0100
> Cc: bug-gnu-emacs <at> gnu.org, Eli Zaretskii <eliz <at> gnu.org>
> 
> Up to now, it seems to run ok. Provided I added some unsetenv() function
> for w32.

If you can show your unsetenv, we can commit it in your name, in
preparation for Paul's commit.

Thanks.

Message #14 received at 13070 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Fabrice Popineau <fabrice.popineau <at> gmail.com>
Cc: 13070 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
Subject: Re: Use putenv+unsetenv instead of modifying environ directly
Date: Fri, 07 Dec 2012 20:02:33 -0800

[Message part 1 (text/plain, inline)]

On 12/04/2012 05:51 AM, Fabrice Popineau wrote:
> Provided I added some unsetenv() function for w32.

Can you just add $(BLD)/unsetenv.$(O) to GNULIBOBJS
in lib/makefile.w32-in, or something like that?  No point in
having two implementations of unsetenv.

I've attached a revised patch (sans any change to makefile.w32-in),
relative to trunk bzr 111151.

[setenv.txt (text/plain, attachment)]

Message #17 received at 13070 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 13070 <at> debbugs.gnu.org, fabrice.popineau <at> gmail.com
Subject: Re: Use putenv+unsetenv instead of modifying environ directly
Date: Sat, 08 Dec 2012 13:42:43 +0200

> Date: Fri, 07 Dec 2012 20:02:33 -0800
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> CC: 13070 <at> debbugs.gnu.org, Eli Zaretskii <eliz <at> gnu.org>
> 
> On 12/04/2012 05:51 AM, Fabrice Popineau wrote:
> > Provided I added some unsetenv() function for w32.
> 
> Can you just add $(BLD)/unsetenv.$(O) to GNULIBOBJS
> in lib/makefile.w32-in, or something like that?  No point in
> having two implementations of unsetenv.

I suspect the gnulib implementation won't work for Windows, for the
same reasons the original code you are replacing didn't: it messes
with the environ block, which is allocated on a heap that is different
from the heap used by Emacs.  Even if we don't use the gnulib putenv
implementation, moving pointers in the environ block that belongs to
the Windows runtime library is something I'd like to avoid if
possible.

However, I have just installed in trunk revision 111156 changes that
add to w32.c an implementation of unsetenv, and a wrapper for putenv
that makes it more Posix-compliant.  So you can now commit your
changes, and I believe they will work.

> I've attached a revised patch (sans any change to makefile.w32-in),
> relative to trunk bzr 111151.

Thanks.

> +void
> +xputenv (char const *string)
> +{
> +  if (putenv ((char *) string) != 0)
> +    memory_full (0);
> +}

Shouldn't we refrain from signaling memory_full when errno is EINVAL?
I'd suggest an eassert in that case.  memory_full will emit a
misleading diagnostic.

Message #22 received at 13070-done <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 13070-done <at> debbugs.gnu.org, fabrice.popineau <at> gmail.com
Subject: Re: Use putenv+unsetenv instead of modifying environ directly
Date: Sat, 08 Dec 2012 09:20:57 -0800

On 12/08/2012 03:42 AM, Eli Zaretskii wrote:
> Shouldn't we refrain from signaling memory_full when errno is EINVAL?
> I'd suggest an eassert in that case.  memory_full will emit a
> misleading diagnostic.

errno cannot be EINVAL, at least not on a POSIXish host:
all strings are allowed as arguments to putenv.
If Microsoft platforms are different it would
make sense to put in an eassert, in w32.c presumably.

I did see a minor problem in the w32.c implementation of unsetenv:

  /* MS docs says an environment variable cannot be longer than 32K.  */
  if (name_len > 32767)
    {
      errno = ENOMEM;
      return -1;
    }

unsetenv should return 0 in that case, not -1, since
the variable cannot possibly be in the environment.
This bug doesn't affect Emacs so it's not a pressing one.

I committed the patch as trunk bzr 111158 and am marking
this bug as fixed.

Message #25 received at 13070 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Paul Eggert <eggert <at> cs.ucla.edu>
Cc: 13070 <at> debbugs.gnu.org, fabrice.popineau <at> gmail.com
Subject: Re: Use putenv+unsetenv instead of modifying environ directly
Date: Sat, 08 Dec 2012 20:31:18 +0200

> Date: Sat, 08 Dec 2012 09:20:57 -0800
> From: Paul Eggert <eggert <at> cs.ucla.edu>
> CC: fabrice.popineau <at> gmail.com, 13070-done <at> debbugs.gnu.org
> 
> On 12/08/2012 03:42 AM, Eli Zaretskii wrote:
> > Shouldn't we refrain from signaling memory_full when errno is EINVAL?
> > I'd suggest an eassert in that case.  memory_full will emit a
> > misleading diagnostic.
> 
> errno cannot be EINVAL, at least not on a POSIXish host:
> all strings are allowed as arguments to putenv.

What about NULL pointers?  Or strings without a '='?  IMO, it's silly
to rely on unspecified behavior, but suit yourself.

> I did see a minor problem in the w32.c implementation of unsetenv:
> 
>   /* MS docs says an environment variable cannot be longer than 32K.  */
>   if (name_len > 32767)
>     {
>       errno = ENOMEM;
>       return -1;
>     }
> 
> unsetenv should return 0 in that case, not -1, since
> the variable cannot possibly be in the environment.

Right, I fixed that.

Message #28 received at 13070 <at> debbugs.gnu.org (full text, mbox):

From: Paul Eggert <eggert <at> cs.ucla.edu>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 13070 <at> debbugs.gnu.org, fabrice.popineau <at> gmail.com
Subject: Re: Use putenv+unsetenv instead of modifying environ directly
Date: Sat, 08 Dec 2012 11:58:57 -0800

On 12/08/2012 10:31 AM, Eli Zaretskii wrote:

> What about NULL pointers?

putenv typically dumps core when passed NULL,
so checking errno afterwards wouldn't help.

> Or strings without a '='?

Typically that's valid; it's equivalent to unsetenv.
So typically, checking errno afterwards wouldn't help.

> it's silly to rely on unspecified behavior

Emacs doesn't rely on unspecified behavior, since it never
passes NULL or '='-less strings to xputenv.

Emacs has many, many places where we could
put easserts, but we don't bother because the disadvantages
of code clutter outweigh any advantages of catching bugs,
and this appears to be one of those places.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.