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

net: TestLookupNullByte is failing on macOS (and maybe other non-Windows ports) #37031

Closed
dmitshur opened this issue Feb 5, 2020 · 4 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone

Comments

@dmitshur
Copy link
Contributor

dmitshur commented Feb 5, 2020

CL 173362 was a part of Go 1.14 changes and fixed a crash in net.LookupHost on Windows. It added the test TestLookupNullByte. A part of that test is not compatible with non-Windows ports such as darwin/amd64, causing the test to fail:

$ go test net -run=TestLookupNullByte
--- FAIL: TestLookupNullByte (0.00s)
    lookup_test.go:1188: unexpected success
FAIL
FAIL	net	0.234s

Marking as release-blocker because it's a regression, and the test failure is very reproducible. The fix will be small.


The test's primary goal is was to ensure net.LookupHost("foo\x00bar") doesn't crash on Windows. Additionally, it happens to check that err != nil on Windows:

// Issue 31586: don't crash on null byte in name
func TestLookupNullByte(t *testing.T) {
	testenv.MustHaveExternalNetwork(t)
	testenv.SkipFlakyNet(t)
	_, err := LookupHost("foo\x00bar") // used to crash on Windows
	if err == nil {
		t.Errorf("unexpected success")
	}
}

On darwin/amd64, net.LookupHost("foo\x00bar") on my machine/network returns a nil error even with Go 1.13.7:

package main

import (
	"fmt"
	"net"
	"runtime"
)

func main() {
	fmt.Println(runtime.Version())
	_, err := net.LookupHost("foo\x00bar")
	fmt.Println(err)
}

// Output:
// go1.13.7
// <nil>

/cc @bradfitz @golang/osp-team I plan to update the test so it either runs only on Windows, or relax the err == nil check. Let me know if there are preferences between those two fixes.

/cc @bradfitz Is it normal that net.LookupHost("foo\x00bar") can succeed on macOS? If it's not expected, I believe a separate issue should be filed for that.

@dmitshur dmitshur added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsFix The path to resolution is known, but the work has not been done. release-blocker labels Feb 5, 2020
@dmitshur dmitshur added this to the Go1.14 milestone Feb 5, 2020
@dmitshur dmitshur self-assigned this Feb 5, 2020
@ianlancetaylor
Copy link
Contributor

What is the net.Lookup call returning for the non-error result?

@dmitshur
Copy link
Contributor Author

dmitshur commented Feb 5, 2020

It's returning the same IP that I get if I do ping foo on my machine.

This was using the Cgo DNS resolver, which was picked by default:

$ echo "1.1.1.1 foo" >> /etc/hosts
$ GODEBUG=netdns=1 go run try.go
go1.13.7
go package net: using cgo DNS resolver
[... 1.1.1.1] <nil>

@dmitshur
Copy link
Contributor Author

dmitshur commented Feb 5, 2020

This might be related to the network I'm on more than to the choice of operating system. If that's the case, a better fix is to remove the "unexpected success" check rather than make the test Windows-only.

Edit: That said, this test has passed for two other people on a similar network who were using Linux. /cc @cagedmantis @toothrot

@gopherbot
Copy link

Change https://golang.org/cl/217729 mentions this issue: net: don't check LookupHost error in TestLookupNullByte

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Development

No branches or pull requests

3 participants