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: LookupPort does not perform lookup if service starts with number #14322

Closed
kennylevinsen opened this issue Feb 13, 2016 · 7 comments

Comments

Projects
None yet
4 participants
@kennylevinsen
Copy link

commented Feb 13, 2016

Calling net.LookupPort with a service-name starting with a number, regardless of whether or not it is in /etc/services, incorrectly returns the numeric prefix of the service-name as port. In my case, I was trying to perform a lookup on "9pfs".

LookupPort (net/lookup.go) calls dtoi (net/parse.go). lookupPort is only called if dtoi returns that it was not successful, but dtoi is successful as long as the string starts with a digit, which makes it incapable of detecting if a string is a service or a port.

Test code

package main

import "fmt"
import "net"

func main() {
    x, err := net.LookupPort("tcp", "9pfs")
    fmt.Printf("Port: %d, err: %v\n", x, err)
    x, err = net.LookupPort("tcp", "1234port")
    fmt.Printf("Port: %d, err: %v\n", x, err)
    x, err = net.LookupPort("tcp", "port1234")
    fmt.Printf("Port: %d, err: %v\n", x, err)
    x, err = net.LookupPort("tcp", "http")
    fmt.Printf("Port: %d, err: %v\n", x, err)
}

Expected result

Port: 564, err: <nil>
Port: 0, err: lookup tcp/1234port: nodename nor servname provided, or not known
Port: 0, err: lookup tcp/port1234: nodename nor servname provided, or not known
Port: 80, err: <nil>

Actual result

Port: 9, err: <nil>
Port: 1234, err: <nil>
Port: 0, err: lookup tcp/port1234: nodename nor servname provided, or not known
Port: 80, err: <nil>
@gopherbot

This comment has been minimized.

Copy link

commented Feb 19, 2016

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

@msiebuhr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2016

The above CL fails on quite a few platforms, as 9pfs apparently isn't included in /etc/services (I don't have a linux box at hand to check on; my FreeBSD and OS X boxes both have it).

I hope to dig a bit more into it once the kids have been put to bed. And any ideas on how to handle this? Leave the test out entirely, or selectively include that particular test only on known-good platforms?

@kennylevinsen

This comment has been minimized.

Copy link
Author

commented Feb 21, 2016

Example 2 (the 1234port one) can test without an entry in service:

x, err := net.LookupPort("tcp", "1234superfake")
if err == nil {
    t.Errorf("fake service was accepted as %d", x)
}

However, a few quick sample Linux boxes had 9pfs in their service file, so that should not be an issue.

EDIT: Ubuntu lacks it, Arch Linux got it. The fake service test should cover the issue, though.

@msiebuhr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2016

@joushou - I don't quite follow your idea for testing 1234port without service. AFAIK, the test-code already checks if the lookup is expected to fail or not.

What really puzzles me is that this change causes some of the other networking tests to fail on windows/386...

@kennylevinsen

This comment has been minimized.

Copy link
Author

commented Feb 21, 2016

In the previous code, 1234port would always succeed, returning 1234. The correct behaviour is to return the port if it is in /etc/services, otherwise fail. If you use an intentionally fake service starting with a digit, you can verify that the fix works as intended without having any requirements to /etc/services. This might be necessary, as only some Linux distros as well as BSD and OSX have 9pfs in /etc/services.

Does that make sense?

Not sure about the other tests failing, haven't looked into them.

@msiebuhr

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2016

Agreed, it would be best with a positive test too. But I fail to see how the code you gave above adds a fake service to do the lookup against. If I revert the fix in net/lookup.go and re-run the tests, they do indeed fail to report an error when looking up 123badservice.

Digging through what's being called, I should be able to dump something into the services-map in https://golang.org/src/net/port_unix.go, but that will only fix it on unixes w/o. cgo. As far as I've skimmed the others, they fairly begin calling system libraries...

@kennylevinsen

This comment has been minimized.

Copy link
Author

commented Feb 21, 2016

Ah, sorry, you already had a test for 123badservice. I'm just spouting nonsense, then.

As for a positive test, a quick read of my ubuntu box's /etc/services show that a positive test on all platforms won't be possible. It contains no services starting with a digit (although OS X got 43 of such entries). I suspect injection of entries on some platforms might make the test a bit too nasty. We could run the positive test on BSD and OS X, but it's still heavily dependent on user configuration, which is not very nice.

As for the test errors. the issue is that you're now accepting arbitrarily large ports, as you fall to lookupPort if the port wasn't ok ((!ok && port != big && port != -big)) or if it didn't consume everything (consumed != len(service)). That means that, if the port was invalid because the service was longer than a valid port, it'll call lookupPort instead of throwing and error with "invalid port".

I guess the correct flow is either:

  • Look up service. If found, return
  • Convert to port number. If successful, return
  • Return error.

Or:

  • Convert to port number. If successful and fully consumed, return
  • Look up service. If found, return
  • Return error.

Edit: Removed super broken code snippet I had written at far too late. Sorry about that.

@gopherbot gopherbot closed this in 002c69e Apr 15, 2016

@golang golang locked and limited conversation to collaborators Apr 15, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.