Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

os: windows Setenv("AN_ENV_VAR", "") should set (not delete) AN_ENV_VAR to empty #5610

Closed
gopherbot opened this issue Jun 1, 2013 · 16 comments
Closed

Comments

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 1, 2013

by jon.forums:

What steps will reproduce the problem?

Original spelunking:
https://groups.google.com/forum/?fromgroups#!topic/golang-nuts/H-3yxhonR_M

To reproduce, run this snippet: http://play.golang.org/p/y_pB0QSPkh

What is the expected output?

---> set `FAKE_ENV_VAR` envar to empty string
---> set `NOEXIST_ENV_VAR` envar to default `*uint16` value

What do you see instead?

---> set `FAKE_ENV_VAR` envar to empty string
setenv: The system could not find the environment option that was entered.

Which compiler are you using (5g, 6g, 8g, gccgo)?

8g

Which operating system are you using?

Win7 32bit (working) and WinXP SP3 32bit (failing)

Which version are you using?  (run 'go version')

Any recent version including:
go version devel +1f7fdf4ad92d Fri May 31 21:44:32 2013 -0700 windows/386

Please provide any additional information below.

I suspect differences in MSFT's SetEnvironmentVariableW impl for Win7 and XP handle v's
default nil value from syscall.Setenv differently.

I will be trying patches similar to the following and report back if appropriate:

func Setenv(key, value string) error {
  ...
  if value == `` {
    a := utf16.Encode([]rune(value + "\x00"))
    v = &a[0]
  }
  if len(value) > 0 {
    ...
  }
  ...
  e := SetEnvironmentVariable(keyp, v)
  }
}
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Jun 1, 2013

Comment 1 by jon.forums:

Quick testing on win7 and xp sp3 (32bit)  with this patch
diff --git a/src/pkg/syscall/env_windows.go b/src/pkg/syscall/env_windows.go
--- a/src/pkg/syscall/env_windows.go
+++ b/src/pkg/syscall/env_windows.go
@@ -37,6 +37,10 @@ func Getenv(key string) (value string, f
 func Setenv(key, value string) error {
    var v *uint16
    var err error
+   if value == `` {
+       a := utf16.Encode([]rune("\x00"))
+       v = &a[0]
+   }
    if len(value) > 0 {
        v, err = UTF16PtrFromString(value)
        if err != nil {
appears to be working on win7 and xp sp3 (32bit) for my affected hobby project:
  https://bitbucket.org/jonforums/uru/
  https://bitbucket.org/jonforums/uru/issue/32/windows-xp-uru-gem-ruby-version-fails
Will be regression testing on win8 (64bit), arch (32/64bit), ubuntu (32/64bit), and snow
leopard (32bit). I have no xp sp3 64bit to test on.
@peterGo

This comment has been minimized.

Copy link
Contributor

@peterGo peterGo commented Jun 1, 2013

Comment 2:

The Windows function call SetEnvironmentVariable("NOEXIST_ENV_VAR\x00", nil) asks for
the variable "NOEXIST_ENV_VAR" to be deleted from the current process's environment.
SetEnvironmentVariable function
http://msdn.microsoft.com/en-us/library/windows/desktop/ms686206.aspx
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Jun 1, 2013

Comment 3 by jon.forums:

Peter, please clarify; I don't yet see your point. Do you think the example patch is
wrong?
While it's true the documented behavior of C/C++
SetEnvironmentVariableW(L"NOEXIST_ENV_VAR", NULL) is to request deletion, it's a bit of
a red herring. The key point is that this Go code
  os.Setenv("NOEXIST_ENV_VAR", "")
performs differently on Win7 and XP even though the marshalled Go code is (I suspect)
the same.
What's interesting to me (a Go source noob) is that pushing a Go "\x00" as the envar
value works for Win7 and XP, while pushing a Go nil fails on XP but works on Win7.
I don't yet understand how Go nil's are marshalled, and whether the marshalled result is
viewed the same as a NULL from SetEnvironmentVariableW's perspective. For example, what
if the marshalled nil is not viewed as a NULL but some corrupted LPWSTR value? Could
this explain the Win7/XP difference if the Win7's impl improved error checking? Who
knows, but I'm hoping to find time to hack some C Win32 code to find out more.
To be clear, I currently think this is a MSFT implementation bug that Go should
compensate for as simply and cleanly as possible.
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jun 2, 2013

Comment 4:

Jon,
Reading this issue, I can see that you are not happy with the way
syscall.SetEnvironmentVariable works: syscall.SetEnvironmentVariable(keyp, v) fails with
keyp=`NOEXIST_ENV_VAR` and v=nil. The call fails with ERROR_ENVVAR_NOT_FOUND (I have
used panic(int(err.(syscall.Errno))) to print error number).
I don't see a problem here.
I don't see a problem with Windows telling you that it cannot delete non-existing
environment variable. It thinks you want to delete existing environment variable. As
described in SetEnvironmentVariable doco
http://msdn.microsoft.com/en-au/library/windows/desktop/ms686206(v=vs.85).aspx: "... If
this parameter is NULL, the variable is deleted from the current process's environment.".
I don't see problem with Go syscall package passing all that information as expected. 
Could you, please, explain again what your problem is? Thank you.
Alex

Status changed to WaitingForReply.

@robpike

This comment has been minimized.

Copy link
Contributor

@robpike robpike commented Jun 2, 2013

Comment 5:

Jon has a point. The os package should hide this difference in the way the systems work.

Labels changed: added priority-soon, packagebug, os-windows, removed priority-triage.

Status changed to Accepted.

@peterGo

This comment has been minimized.

Copy link
Contributor

@peterGo peterGo commented Jun 2, 2013

Comment 6:

The os package interface is intended to be uniform across all operating systems. The
Windows syscall.Getenv function should be equivalent to the Unix syscall.Getenv function.
The os.Setenv function calls syscall.Setenv which, on Windows, calls
sycall.SetEnvironmentVariable.
SetEnvironmentVariable function
http://msdn.microsoft.com/en-us/library/windows/desktop/ms686206.aspx
On Windows, syscall.Setenv wrongly converts an empty value string to a nil pointer
before calling sycall.SetEnvironmentVariable. For example, os.Setenv("ENV_VAR", "")
calls syscall.Setenv("ENV_VAR", "") which calls
syscall.SetEnvironmentVariable("ENV_VAR\x00", nil) which asks for the variable "ENV_VAR"
to be deleted from the current process's environment.
The 5610.diff file contains this patch to fix the issue.
diff --git a/src/pkg/syscall/env_windows.go b/src/pkg/syscall/env_windows.go
--- a/src/pkg/syscall/env_windows.go
+++ b/src/pkg/syscall/env_windows.go
@@ -35,13 +35,9 @@
 }
 
 func Setenv(key, value string) error {
-   var v *uint16
-   var err error
-   if len(value) > 0 {
-       v, err = UTF16PtrFromString(value)
-       if err != nil {
-           return err
-       }
+   v, err := UTF16PtrFromString(value)
+   if err != nil {
+       return err
    }
    keyp, err := UTF16PtrFromString(key)
    if err != nil {
The 5610.go file illustrates and tests for the error.
Windows 7 64-bit
go version devel +8c7d0793f4d5 Sun Jun 02 11:36:09 2013 +0200 windows/386
go version devel +8c7d0793f4d5 Sun Jun 02 11:36:09 2013 +0200 windows/amd64
Error: FAKE_ENV_VAR_EXISTS="": deleted
Error: FAKE_ENV_VAR="": not created
Windows XP 32-bit
go version devel +31ab5b976437 Sun Jun 02 01:45:26 2013 +0400 windows/386
Error: FAKE_ENV_VAR_EXISTS="": deleted
FAKE_ENV_VAR="": setenv: The system could not find the environment option that was
entered.
Error: FAKE_ENV_VAR="": not created

Attachments:

  1. 5610.diff (477 bytes)
  2. 5610.go (1787 bytes)
@peterGo

This comment has been minimized.

Copy link
Contributor

@peterGo peterGo commented Jun 2, 2013

Comment 7:

Jon,
I posted the link to the SetEnvironmentVariable function to point out that your analysis
of the issue is flawed. Therefore, your tests do not identify the general Os.Setenv
problem on Windows; environment variables are being deleted and are not being created.
I don't like your patch because it tries to identify when the bug occurs then fudges
around it.
Peter
@peterGo

This comment has been minimized.

Copy link
Contributor

@peterGo peterGo commented Jun 2, 2013

Comment 8:

Started: https://golang.org/cl/9945044
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Jun 2, 2013

Comment 9 by jon.forums:

Peter, thank you, and understood.
Your explanation is spot on for the failure I was getting. Conceptually, syscall.Setenv
incorrectly turned my "set an envar to an empty string" request into a "delete an envar"
request.
Your patch is cleaner, more robust, and solves the issue on my 32bit xp sp3 and win7
systems. I'm double checking on other systems of interest, but don't expect any
regressions.
Jon
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Jun 22, 2013

Comment 10 by jon.forums:

What's still left to do before the patch will be applied to the official repo?
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jun 23, 2013

Comment 11:

I am waiting for test / tests to be added to https://golang.org/cl/9945044 to
pin down new behaviour. I am also unclear about what the change does. I hope tests will
help here. Also, perhaps documentation change is in order and better CL description.
Alex
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Jun 23, 2013

Comment 12 by jon.forums:

Are we in agreement that Go's current behavior (as Peter summarized in #6) is a bug on
Windows? Specifically, do we agree that Go should not be turning a request to set an env
var to an empty string into a request to delete an env var?
My key issue continues to be that this corner case Go code
   os.Setenv("AN_ENV_VAR", "")
behaves differently on WinXP and Win7.
Peter: I will swing back and dig into this again, but in the interest of forward
progress for pinning down "correct" behavior, please post the tests you mentioned in
  https://golang.org/cl/9945044#msg8
Jon
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jun 24, 2013

Comment 13:

Jon,
> Are we in agreement that Go's current behavior (as Peter summarized in #6) is a bug on
Windows? Specifically, do we agree that Go should not be turning a request to set an env
var to an empty string into a request to delete an env var?
I am yet to be convinced. For example, someone needs to confirm that what you are trying
to do (set environment variable to an *empty* string, whatever *empty* means) is
possible. Also how would I remove environment variable, if that ability is taken away?
What do other OSes do in these situations?
> My key issue continues to be that this corner case Go code os.Setenv("AN_ENV_VAR", "")
behaves differently on WinXP and Win7.
Lets decide on our main problem first, then we could consider this in light of our new
proposal.
Alex
@gopherbot

This comment has been minimized.

Copy link
Author

@gopherbot gopherbot commented Jun 24, 2013

Comment 14 by jon.forums:

Alex,
Let's see if I persuade you that:
1) Go's incorrect impl of os.Setenv and WinXP/Win7 differences in MSFT's
SetEnvironmentVariableW impl are conspiring to create this bug.
2) Deleting single env var from Go is a separate issue.
3) MSFT's poor API for deleting single env vars should not be directly surfaced to Go.
Testing a few scenarios via this Go code
  http://play.golang.org/p/-i3IPGq9VB
gives the following multi-platform "expected" results
  # OS X Snow Leopard, Arch Linux 3.9.7, and Windows 7 (32bit)
  # Arch Linux 3.9.7 and Windows 7 (64bit)
  ---> read nonexistent `GO_TEST_ENVAR` value: ``
  ---> write `GO_TEST_ENVAR` to `bogus envar value`
  ---> read `GO_TEST_ENVAR` value: `bogus envar value`
  ---> write `GO_TEST_ENVAR` to empty string
  ---> read `GO_TEST_ENVAR` value: ``
  ---> read `GO_NONEXIST_TEST_ENVAR` value: ``
  ---> write `GO_NONEXIST_TEST_ENVAR` to empty string
  ---> read `GO_NONEXIST_TEST_ENVAR` value: ``
and this "fail" result on WinXP 32bit:
  # Windows XP SP3 (32bit)
  ---> read nonexistent `GO_TEST_ENVAR` value: ``
  ---> write `GO_TEST_ENVAR` to `bogus envar value`
  ---> read `GO_TEST_ENVAR` value: `bogus envar value`
  ---> write `GO_TEST_ENVAR` to empty string
  ---> read `GO_TEST_ENVAR` value: ``
  ---> read `GO_NONEXIST_TEST_ENVAR` value: ``
  setenv: The system could not find the environment option that was entered.
The difference is due to Go incorrectly converting an empty string into a nil *and*
MSFT's SetEnvironmentVariableW different XP/7 behavior for nonexistent env vars.
Although this failure occurs when deleting a nonexistent env var, deleting a single env
var is a separate issue since the original Go code's intent is to *set* an env var to an
empty string. Go shouldn't be changing programmer intention.
MSFT's dual-purpose SetEnvironmentVariableW API for setting *and* deleting an env var is
a poor idea that does not clearly communicate intention, and should not be directly
supported in Go. Go should abstract deleting an env var as a separate cross-platform
API. For example, in C I might use MSFT's API to set an env var via
  SetEnvironmentVariableW(L"NONEXIST_ENV_VAR", L"blah")
but the following don't clearly communicate intention:
  SetEnvironmentVariableW(L"NONEXIST_ENV_VAR", L"")
  SetEnvironmentVariableW(L"NONEXIST_ENV_VAR", NULL)
Are they both meant to "delete" the env var, or something else? While convenient, MSFT's
dual purpose API is a bad idea and I don't want to see this code style in Go.
Currently, I don't believe there is a way from Go to delete a single env var. Instead of
hijacking os.Setenv, I'd rather see Go fall into the clear-and-explicit camp via API like
  func Clearenv()
  func Getenv(key string) error
  func Setenv(key, value string) error
  func Delenv(key string) error
where Delenv is responsible for only deleting a single env var and Setenv is responsible
for only setting a single envar.
It seems Go could provide these abstractions in a cross-platform way, but I've not
looked into it. I plan to carve out some time and play with the C API for Linux, OS X,
and Windows to convince myself Go could abstract deleting a single env var without
hijacking os.Setenv.
Since it appears there is currently no Go API to delete a single env var, we're not
talking about removing existing functionality. I believe os.Setenv can be fixed and it
can stay focused on setting env vars rather than catering to MSFT's set/delete semantics.
Jon
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jun 26, 2013

Comment 15:

Jon,
I am convinced. I decided not to wait for go.peter.90 to finish his CL. Here is mine:
https://golang.org/cl/10594043/.
Alex
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jun 27, 2013

Comment 16:

This issue was closed by revision 04b405c.

Status changed to Fixed.

@golang golang locked and limited conversation to collaborators Jun 24, 2016
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.