all: race condition in getting environment variable on windows

[jaykrell]

https://github.com/golang/sys/blob/master/windows/env_windows.go

This code exhibits a common bug.
Other threads can change the value, or unset it, while it is being retrieved.
You should loop until either
not found
or variable fits in buffer

[alexbrainman]

Can you demonstrate the bug with a bit of code? I really don't see what the problem is. Thank you.

Alex

[jaykrell]

Consider another thread calling SetEnvironmentVariable concurrently with this code.

• Jay

On Feb 2, 2015, at 10:43 PM, alexbrainman notifications@github.com wrote:

Can you demonstrate the bug with a bit of code? I really don't see what the problem is. Thank you.

Alex

—
Reply to this email directly or view it on GitHub.

[jaykrell]

Same bug here:
https://github.com/golang/sys/blob/master/windows/exec_windows.go

Consider another thread calling SetCurrentDirectory.
Again the correct code requires a loop.

• Jay

On Feb 3, 2015, at 12:07 AM, Jay jay.krell@cornell.edu wrote:

Consider another thread calling SetEnvironmentVariable concurrently with this code.

• Jay

On Feb 2, 2015, at 10:43 PM, alexbrainman notifications@github.com wrote:

Can you demonstrate the bug with a bit of code? I really don't see what the problem is. Thank you.

Alex

—
Reply to this email directly or view it on GitHub.

[mattn]

package main

import (
    "os"
    "runtime"
    "time"
)

func main() {
    runtime.GOMAXPROCS(runtime.NumCPU())
    go func() {
        for {
            os.Setenv("FOO", "GO")
            os.Setenv("FOO", "GOLAAAAAAAAAAAAAAAAAAAAAANG")
        }
    }()

    time.Sleep(1e9)

    for {
        e := os.Getenv("FOO")
        if e != "GO" && e != "GOLAAAAAAAAAAAAAAAAAAAAAANG" {
            panic("BUG!: " + e)
        }
    }
}

Hmm, I run this code but couldn't repro

[mattn]

package main

import (
    "fmt"
    "os"
    "runtime"
    "strings"
    "time"
)

func main() {
    runtime.GOMAXPROCS(runtime.NumCPU())
    n := 1
    go func() {
        for {
            os.Setenv("FOO", "GOL"+strings.Repeat("O", n)+"NG")
            if n++; n > 5000 {
                n = 1
            }
        }
    }()

    time.Sleep(1e9)

    for {
        s := os.Getenv("FOO")
        if !strings.HasSuffix(s, "ONG") {
            panic(fmt.Sprintf("BUG(%d): %s", n, s))
        }
    }
}

http://play.golang.org/p/srvhu31O1r

I could make this crash on windows easily... 👎

panic: BUG(228):

goroutine 1 [running]:
main.main()
        C:/dev/envrace.go:28 +0x274

goroutine 4 [runnable]:
syscall.UTF16FromString(0x118d9d10, 0xf0, 0x0, 0x0, 0x0, 0x0, 0x0)
        c:/go/src/syscall/syscall_windows.go:42 +0xf7
syscall.UTF16PtrFromString(0x118d9d10, 0xf0, 0xf0, 0x0, 0x0)
        c:/go/src/syscall/syscall_windows.go:66 +0x40
syscall.Setenv(0x4cebe0, 0x3, 0x118d9d10, 0xf0, 0x0, 0x0)
        c:/go/src/syscall/env_windows.go:35 +0x40
os.Setenv(0x4cebe0, 0x3, 0x118d9d10, 0xf0, 0x0, 0x0)
        c:/go/src/os/env.go:87 +0x50
main.funcﾂｷ001()
        C:/dev/envrace.go:16 +0x96
created by main.main
        C:/dev/envrace.go:21 +0x90
exit status 2

GetEnvironmentVariable is not thread-safe!?

[alexbrainman]

I see. I agree we should fix this. Thank you.

Alex

[jaykrell]

GetEnvironmentVariable is not thread-safe!?

It is thread-safe. But there is no public lock to be had around multiple calls to it. This a common aspect of various "Get" functions. GetFullPathame, GetModuleFileName, etc.

• Jay

On Feb 3, 2015, at 12:33 AM, mattn notifications@github.com wrote:

GetEnvironmentVariable is not thread-safe!?

[alexbrainman]

I looked in main repo. These are to be fixed:

net/getAdapterList
path/filepath/toShort
path/filepath/toSLong
syscall/FullPath
syscall/Getenv
syscall/security_windows.go everything that returns ERROR_INSUFFICIENT_BUFFER error
syscall/TempDir

(@jaykrell can you see others?)

None of them look likely to happens in real program (IMO), but we might as well fix them. Once fixed we should copy syscall changes into golang.org/x/sys/windows.

@jaykrell interested to make changes? If not I will do it myself.

Alex

[alexbrainman]

Here https://go-review.googlesource.com/4940 is the fix.

Alex

[gopherbot]

CL https://golang.org/cl/11088 mentions this issue.