cmd/go: go test -o NUL fails on Windows

[rvflash]

go test command manages os.DevNull on Windows as relative path not as null device.

What version of Go are you using (go version)?

go version go1.11.1 windows/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

set GOARCH=amd64
set GOBIN=
set GOCACHE=C:\Users\Jupiter\AppData\Local\go-build
set GOEXE=.exe
set GOFLAGS=
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=C:\Users\Jupiter\go
set GOPROXY=
set GORACE=
set GOROOT=C:\Go
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set GOMOD=
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=-m64 -mthreads -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=C:\Users\Jupiter\AppData\Local\Temp\go-build946346806=/tmp/go-build -gno-record-gcc-switches

What did you do?

go test -c -work -o NUL

What did you expect to see?

WORK=C:\Users\Jupiter\AppData\Local\Temp\go-build814365262

What did you see instead?

WORK=C:\Users\Jupiter\AppData\Local\Temp\go-build814365262
go test /C/Users/Jupiter/Documents/echo.test: build output "C:\Users\Jupiter\Documents\echo\NUL" already exists and is not an object file

---

Hi,

Since the go1.11, still true with the latest version go1.11.1, the os.DevNull value for Windows is not well managed by the go test command (see file flag). NUL is managed as relative path, not as null device.
I suggest to add something like that in the file src/cmd/go/internal/test/testflag.go to deal with the -o flag:

// Special case -o /dev/null by not writing at all.
testO = value
if testO == os.DevNull {
	testO = ""
}
testNeedBinary = true

Sample code to reproduce :

// file: echo/echo.go
package echo

func Echo(s string) string {
	return s
}

// file: echo/echo_test.go
package echo

import "testing"

const testdata = "hi"

func TestClientMustProcess(t *testing.T) {
	if s := Echo(testdata); s != testdata {
		t.Fatalf("unexpected result: got=%q exp=%q", s, testdata)
	}
}

Then, inside the echo package directory, run the go test command with file flag: - o.

Hope it helps.

[alexbrainman]

Thank you for the report. I am not sure that your proposed fix is the correct change here.

Bisected to

e83601b4356f92f2f4d05f302d5654754ff05a6d is the first bad commit
commit e83601b4356f92f2f4d05f302d5654754ff05a6d
Author: Alex Brainman <[email protected]>
Date:   Sun Jan 7 12:12:25 2018 +1100

    os: use WIN32_FIND_DATA.Reserved0 to identify symlinks

    os.Stat implementation uses instructions described at
    https://blogs.msdn.microsoft.com/oldnewthing/20100212-00/?p=14963/
    to distinguish symlinks. In particular, it calls
    GetFileAttributesEx or FindFirstFile and checks
    either WIN32_FILE_ATTRIBUTE_DATA.dwFileAttributes
    or WIN32_FIND_DATA.dwFileAttributes to see if
    FILE_ATTRIBUTES_REPARSE_POINT flag is set.
    And that seems to worked fine so far.

    But now we discovered that OneDrive root folder
    is determined as directory:

    c:\>dir C:\Users\Alex | grep OneDrive
    30/11/2017  07:25 PM    <DIR>          OneDrive
    c:\>

    while Go identified it as symlink.

    But we did not follow Microsoft's advice to the letter - we never
    checked WIN32_FIND_DATA.Reserved0. And adding that extra check
    makes Go treat OneDrive as symlink. So use FindFirstFile and
    WIN32_FIND_DATA.Reserved0 to determine symlinks.

    Fixes #22579

    Change-Id: I0cb88929eb8b47b1d24efaf1907ad5a0e20de83f
    Reviewed-on: https://go-review.googlesource.com/86556
    Reviewed-by: Brad Fitzpatrick <[email protected]>
    Run-TryBot: Brad Fitzpatrick <[email protected]>
    TryBot-Result: Gobot Gobot <[email protected]>

:040000 040000 6e4ea25b8e56d55d3119d258431eb49b66ac19fc a6eac27eee057861c2fa8254dc627fa181fd6ae5 M  src

I will investigate properly when I have time.

Alex

[rvflash]

Hi Alex,

I knew it, I had looked very quickly at the differences between the go build and go test commands on this specific input, without even testing.

Thanks, Hervé

[alexbrainman]

NUL is managed as relative path, not as null device.

Yes, I agree. path/filepath.IsAbs("NUL") returns false on Windows. I think it should return true.

I suggest to add something like that in the file src/cmd/go/internal/test/testflag.go to deal with the -o flag:

// Special case -o /dev/null by not writing at all.
testO = value
if testO == os.DevNull {
	testO = ""
}
testNeedBinary = true

This will not work if user runs

go test -c -work -o nul

command. But this is another problem.

I knew it, I had looked very quickly at the differences between the go build and go test commands on this specific input, without even testing.

I do not understand what you are saying. Please try again.

Alex

[rvflash]

I agree Alex.

Just by reading the code, I was looking for differences explaining why this command was in error with go test -o and I saw this check with go build -o. I thought it could be that, without looking any further, sorry my bad.

Next time, I will push my analysis further before submitting a hypothesis ;-)

[as]

What does IsAbs(.\nul) return? What about ..\nul. Will I be able to trick IsAbs by giving it NUL anywhere in the path?

[alexbrainman]

Next time, I will push my analysis further before submitting a hypothesis ;-)

I still do not know why commit e83601b creates this issue - I did not have time to investigate properly. So, anything is possible. Feel free to investigate and push hypothesis - correct or wrong. I make mistakes all the time. It is OK to make mistakes.

What does IsAbs(.\nul) return? What about ..\nul. Will I be able to trick IsAbs by giving it NUL anywhere in the path?

I do not know. Hopefully, if we make path/filepath.IsAbs("nul") return true, we won't get paths like .\nul and ..\nul in our code.

Alex

[alexbrainman]

Finally I had time to understand why e83601b introduces this issue.

The issue happens because path/filepath.IsAbs("NUL") returns false. See this code

go/src/cmd/go/internal/test/test.go

Lines 886 to 888 in b4150f7

	if !filepath.IsAbs(target) {
	target = filepath.Join(base.Cwd, target)
	}

target is set to "NUL" on line 886, and it becomes "C:\Users\Jupiter\Documents\echo\NUL" on line 887.

The difference of what code does before and after e83601b can be seen here (dst is set to "C:\Users\Jupiter\Documents\echo\NUL")

go/src/cmd/go/internal/work/exec.go

Lines 1560 to 1567 in b4150f7

	if fi, err := os.Stat(dst); err == nil {
	if fi.IsDir() {
	return fmt.Errorf("build output %q already exists and is a directory", dst)
	}
	if !force && fi.Mode().IsRegular() && !isObject(dst) {
	return fmt.Errorf("build output %q already exists and is not an object file", dst)
	}
	}

Before e83601b os.Stat on line 1560 fails, so none of 1561-L1567 lines run.

And after e83601b os.Stat succeeds. I think the fact that os.Stat(C:\Users\Jupiter\Documents\echo\NUL) succeeds is fine - any file with the name of NUL is treated as NUL on Windows (I tried this https://play.golang.org/p/c1-Zh0p3D2W program, and it succeeds on my computer). But then the fi.Mode().IsRegular() on line 1564 returns true, and that is wrong. If you look at current os.Stat implementation, we determine NUL device by comparing os.Stat parameter with "NUL" string, so that obviously fails for C:\Users\Jupiter\Documents\echo\NUL.

Anyways, we could change os.Stat(C:\Users\Jupiter\Documents\echo\NUL) to return Mode().IsRegular() false. But the main problem here is that path/filepath.IsAbs("NUL") returns false.

So, should we make path/filepath.IsAbs("NUL") return false? ( and others, like CON, PRN, AUX, NUL, COM1, COM2, COM3, COM4, COM5, COM6, COM7, COM8, COM9, LPT1, LPT2, LPT3, LPT4, LPT5, LPT6, LPT7, LPT8, and LPT9 - search https://docs.microsoft.com/en-us/windows/desktop/fileio/naming-a-file for NUL). Is that the right thing to do? Should we similarly handle other functions in path/filepath ?

Alternatively, we could special case NUL in cmd/go, like Russ did in a3faa80 - Russ adjusted go build code, we need to adjust do test code. Interestingly, we have cmd/go.TestGoBuildDashODevNull, but cmd/go.TestGoTestDashODevNull is missing.

Also a3faa80 determines os.DevNull by comparing cmd/go parameter with os.DevNull string. That will not work on Windows because user can pass both NUL and nul. So we would need to deal with that too. Luckily a3faa80 is a NOP on Windows, so it is not important there.

Sorry for long comment.

I need decision here (I can create a proposal, if needed).

@ianlancetaylor @bradfitz @mattn and everyone else who has anything to add here

Thank you.

Alex

[ianlancetaylor]

I suppose I think that on Windows the special names like NUL and CON should be treated as absolute: filepath.IsAbs should return true for them.

If we make that change is that enough to fix this problem?

[alexbrainman]

I suppose I think that on Windows the special names like NUL and CON should be treated as absolute: filepath.IsAbs should return true for them.

SGTM. My only concern is, that we should audit all our path/filepath functions for files like NUL. Maybe will start with path/filepath.IsAbs, and see how it goes.

If we make that change is that enough to fix this problem?

Yes, fixing path/filepath.IsAbs will fix this issue. I will add a cmd/go test to make sure it stays fixed.

Should we also do something about string comparison if testO == os.DevNull { I mentioned in #28035 (comment) ? The result of this expression is not used on Windows at this moment, but that code can grow. And other people might look at the code, and assume that it is correct.

Alex

[ianlancetaylor]

The comparison I see is in cmd/go/internal/work/build.go: cfg.BuildO == os.DevNull. It went in for #16811. That problem (deleting /dev/null) doesn't arise on Windows, so I don't know if there is a problem here that needs fixing. That is, I assume that go build -o NUL works as on Unix, but does go build -o nul actually do the wrong thing, after we fix this issue?

In any case that seems like a separate issue to consider after this one is fixed.

[as]

Take caution with -o NUL. Creating a file named NUL on Windows can actually be done inadvertently and may lead to strange behavior where the file and containing directory is difficult to delete.

[alexbrainman]

The comparison I see is in cmd/go/internal/work/build.go: cfg.BuildO == os.DevNull. It went in for #16811. That problem (deleting /dev/null) doesn't arise on Windows,

Correct. That comparison has no effect on Windows.

so I don't know if there is a problem here that needs fixing.

There is no problem related to cfg.BuildO == os.DevNull code.

That is, I assume that go build -o NUL works as on Unix, but does go build -o nul actually do the wrong thing, after we fix this issue?

Both go build -o NUL and go build -o nul work on Windows.

In any case that seems like a separate issue to consider after this one is fixed.

It is a separate issue from this issue. I will leave that code as is, since nothing is broken yet.

Creating a file named NUL on Windows can actually be done inadvertently

os package uses Windows CreateFile API. How do you create file named "NUL" with this API?

Alex

[as]

@alexbrainman disregard my statement about creating NUL. I can niether reproduce it with a trivial example nor have the bandwidth to investigate further.

[gopherbot]

Change https://golang.org/cl/145220 mentions this issue: path/filepath: change IsAbs("NUL") to return true

[rvflash]

Thank you @alexbrainman.

[ianlancetaylor]

@neild Does this need to be reopened now that https://golang.org/cl/145220 has been reverted?

[neild]

No, https://go.dev/cl/449117 should have retained a fix for go test -o NUL.