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

rvflash · 2018-10-05T15:09:01Z

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.

The text was updated successfully, but these errors were encountered:

alexbrainman · 2018-10-07T01:35:20Z

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 <alex.brainman@gmail.com>
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 <bradfitz@golang.org>
    Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
    TryBot-Result: Gobot Gobot <gobot@golang.org>

:040000 040000 6e4ea25b8e56d55d3119d258431eb49b66ac19fc a6eac27eee057861c2fa8254dc627fa181fd6ae5 M  src

I will investigate properly when I have time.

Alex

rvflash · 2018-10-07T09:19:21Z

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 · 2018-10-07T09:38:18Z

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 · 2018-10-07T12:32:20Z

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 · 2018-10-08T02:48:10Z

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

alexbrainman · 2018-10-08T08:46:30Z

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 · 2018-10-14T06:00:27Z

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 · 2018-10-17T00:17:33Z

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 · 2018-10-17T09:01:14Z

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 · 2018-10-17T13:30:01Z

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 · 2018-10-18T01:04:46Z

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 · 2018-10-18T09:04:08Z

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 · 2018-10-20T03:17:41Z

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

gopherbot · 2018-10-28T02:38:56Z

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

rvflash · 2018-11-02T13:27:07Z

Thank you @alexbrainman.

ianlancetaylor · 2022-11-09T22:22:40Z

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

neild · 2022-11-09T23:56:38Z

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

ianlancetaylor added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels Oct 5, 2018

ianlancetaylor added this to the Go1.12 milestone Oct 5, 2018

ianlancetaylor changed the title ~~build error: go test -o NUL (aka os.DevNull on Windows)~~ cmd/go: go test -o NUL fails on Windows Oct 5, 2018

agnivade added the OS-Windows label Oct 6, 2018

Pykselle mentioned this issue Oct 15, 2018

GO1.11 : go test -o NUL fails on Windows cucumber/godog#142

Closed

alexbrainman mentioned this issue Oct 16, 2018

open NUL: The system cannot find the file spec #27895

Closed

gopherbot closed this as completed in d154ef6 Nov 2, 2018

golang locked and limited conversation to collaborators Nov 2, 2019

gopherbot added the FrozenDueToAge label Nov 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rvflash commented Oct 5, 2018

alexbrainman commented Oct 7, 2018

rvflash commented Oct 7, 2018

alexbrainman commented Oct 7, 2018

rvflash commented Oct 7, 2018

as commented Oct 8, 2018

alexbrainman commented Oct 8, 2018

alexbrainman commented Oct 14, 2018

ianlancetaylor commented Oct 17, 2018

alexbrainman commented Oct 17, 2018

ianlancetaylor commented Oct 17, 2018

as commented Oct 18, 2018

alexbrainman commented Oct 18, 2018

as commented Oct 20, 2018

gopherbot commented Oct 28, 2018

rvflash commented Nov 2, 2018

ianlancetaylor commented Nov 9, 2022

neild commented Nov 9, 2022

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

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

Comments

rvflash commented Oct 5, 2018

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

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

Does this issue reproduce with the latest release?

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

What did you do?

What did you expect to see?

What did you see instead?

alexbrainman commented Oct 7, 2018

rvflash commented Oct 7, 2018

alexbrainman commented Oct 7, 2018

rvflash commented Oct 7, 2018

as commented Oct 8, 2018

alexbrainman commented Oct 8, 2018

alexbrainman commented Oct 14, 2018

ianlancetaylor commented Oct 17, 2018

alexbrainman commented Oct 17, 2018

ianlancetaylor commented Oct 17, 2018

as commented Oct 18, 2018

alexbrainman commented Oct 18, 2018

as commented Oct 20, 2018

gopherbot commented Oct 28, 2018

rvflash commented Nov 2, 2018

ianlancetaylor commented Nov 9, 2022

neild commented Nov 9, 2022

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

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