From 983f5a717a35a604847ae4a4964c19000dc44016 Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Wed, 13 May 2015 16:04:06 +0200 Subject: p2p/nat: fix concurrent access to autodisc Interface Concurrent calls to Interface methods on autodisc could return a "not discovered" error if the discovery did not finish before the call. autodisc.wait expected the done channel to carry the found Interface but it was closed instead. The fix is to use sync.Once for now, which is easier to get right. And there is a test. Finally. This will have to change again when we introduce re-discovery. --- p2p/nat/nat.go | 32 +++++++++++++++----------------- p2p/nat/nat_test.go | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+), 17 deletions(-) create mode 100644 p2p/nat/nat_test.go (limited to 'p2p/nat') diff --git a/p2p/nat/nat.go b/p2p/nat/nat.go index fe00bdab0..58d50976d 100644 --- a/p2p/nat/nat.go +++ b/p2p/nat/nat.go @@ -172,8 +172,9 @@ func PMP(gateway net.IP) Interface { // This type is useful because discovery can take a while but we // want return an Interface value from UPnP, PMP and Auto immediately. type autodisc struct { - what string - done <-chan Interface + what string // type of interface being autodiscovered + once sync.Once + doit func() Interface mu sync.Mutex found Interface @@ -181,9 +182,10 @@ type autodisc struct { func startautodisc(what string, doit func() Interface) Interface { // TODO: monitor network configuration and rerun doit when it changes. - done := make(chan Interface) - ad := &autodisc{what: what, done: done} - go func() { done <- doit(); close(done) }() + ad := &autodisc{what: what, doit: doit} + // Start the auto discovery as early as possible so it is already + // in progress when the rest of the stack calls the methods. + go ad.wait() return ad } @@ -218,19 +220,15 @@ func (n *autodisc) String() string { } } +// wait blocks until auto-discovery has been performed. func (n *autodisc) wait() error { - n.mu.Lock() - found := n.found - n.mu.Unlock() - if found != nil { - // already discovered - return nil - } - if found = <-n.done; found == nil { - return errors.New("no UPnP or NAT-PMP router discovered") + n.once.Do(func() { + n.mu.Lock() + n.found = n.doit() + n.mu.Unlock() + }) + if n.found == nil { + return fmt.Errorf("no %s router discovered", n.what) } - n.mu.Lock() - n.found = found - n.mu.Unlock() return nil } diff --git a/p2p/nat/nat_test.go b/p2p/nat/nat_test.go new file mode 100644 index 000000000..95c50522e --- /dev/null +++ b/p2p/nat/nat_test.go @@ -0,0 +1,48 @@ +package nat + +import ( + "bytes" + "net" + "testing" + "time" +) + +// This test checks that autodisc doesn't hang and returns +// consistent results when multiple goroutines call its methods +// concurrently. +func TestAutoDiscRace(t *testing.T) { + ad := startautodisc("thing", func() Interface { + time.Sleep(500 * time.Millisecond) + return extIP{33, 44, 55, 66} + }) + + // Spawn a few concurrent calls to ad.ExternalIP. + type rval struct { + ip net.IP + err error + } + results := make(chan rval, 50) + for i := 0; i < cap(results); i++ { + go func() { + ip, err := ad.ExternalIP() + results <- rval{ip, err} + }() + } + + // Check that they all return the correct result within the deadline. + deadline := time.After(550 * time.Millisecond) + for i := 0; i < cap(results); i++ { + select { + case <-deadline: + t.Fatal("deadline exceeded") + case rval := <-results: + if rval.err != nil { + t.Errorf("result %d: unexpected error: %v", i, rval.err) + } + wantIP := net.IP{33, 44, 55, 66} + if !bytes.Equal(rval.ip, wantIP) { + t.Errorf("result %d: got IP %v, want %v", i, rval.ip, wantIP) + } + } + } +} -- cgit v1.2.3 From 663d4e0aff4cad98a9c5b17de18b2385a5874f6c Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Wed, 13 May 2015 16:12:46 +0200 Subject: p2p/nat: add test for UPnP auto discovery via SSDP The test listens for multicast UDP packets on the default interface because I couldn't get it to work reliably on loopback without massive changes to goupnp. This means that the test might fail when there is a UPnP-enabled router attached on that interface. I checked that locally by looping the test and it passes reliably because the local SSDP server always responds faster. --- p2p/nat/natupnp_test.go | 223 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 223 insertions(+) create mode 100644 p2p/nat/natupnp_test.go (limited to 'p2p/nat') diff --git a/p2p/nat/natupnp_test.go b/p2p/nat/natupnp_test.go new file mode 100644 index 000000000..074e97c81 --- /dev/null +++ b/p2p/nat/natupnp_test.go @@ -0,0 +1,223 @@ +package nat + +import ( + "fmt" + "io" + "net" + "net/http" + "strings" + "testing" + + "github.com/huin/goupnp/httpu" +) + +func TestUPNP_DDWRT(t *testing.T) { + dev := &fakeIGD{ + t: t, + ssdpResp: "HTTP/1.1 200 OK\r\n" + + "Cache-Control: max-age=300\r\n" + + "Date: Sun, 10 May 2015 10:05:33 GMT\r\n" + + "Ext: \r\n" + + "Location: http://{{listenAddr}}/InternetGatewayDevice.xml\r\n" + + "Server: POSIX UPnP/1.0 DD-WRT Linux/V24\r\n" + + "ST: urn:schemas-upnp-org:device:WANConnectionDevice:1\r\n" + + "USN: uuid:CB2471CC-CF2E-9795-8D9C-E87B34C16800::urn:schemas-upnp-org:device:WANConnectionDevice:1\r\n" + + "\r\n", + httpResps: map[string]string{ + "GET /InternetGatewayDevice.xml": ` + + + + 1 + 0 + + + urn:schemas-upnp-org:device:InternetGatewayDevice:1 + DD-WRT + http://www.dd-wrt.com + Gateway + Asus RT-N16:DD-WRT + Asus RT-N16 + V24 + 0000001 + http://www.dd-wrt.com + uuid:A13AB4C3-3A14-E386-DE6A-EFEA923A06FE + + + urn:schemas-upnp-org:service:Layer3Forwarding:1 + urn:upnp-org:serviceId:L3Forwarding1 + /x_layer3forwarding.xml + /control?Layer3Forwarding + /event?Layer3Forwarding + + + + + urn:schemas-upnp-org:device:WANDevice:1 + WANDevice + DD-WRT + http://www.dd-wrt.com + Gateway + router + http://www.dd-wrt.com + uuid:48FD569B-F9A9-96AE-4EE6-EB403D3DB91A + + + urn:schemas-upnp-org:service:WANCommonInterfaceConfig:1 + urn:upnp-org:serviceId:WANCommonIFC1 + /x_wancommoninterfaceconfig.xml + /control?WANCommonInterfaceConfig + /event?WANCommonInterfaceConfig + + + + + urn:schemas-upnp-org:device:WANConnectionDevice:1 + WAN Connection Device + DD-WRT + http://www.dd-wrt.com + Gateway + router + http://www.dd-wrt.com + uuid:CB2471CC-CF2E-9795-8D9C-E87B34C16800 + + + urn:schemas-upnp-org:service:WANIPConnection:1 + urn:upnp-org:serviceId:WANIPConn1 + /x_wanipconnection.xml + /control?WANIPConnection + /event?WANIPConnection + + + + + + + urn:schemas-upnp-org:device:LANDevice:1 + LANDevice + DD-WRT + http://www.dd-wrt.com + Gateway + router + http://www.dd-wrt.com + uuid:04021998-3B35-2BDB-7B3C-99DA4435DA09 + + + urn:schemas-upnp-org:service:LANHostConfigManagement:1 + urn:upnp-org:serviceId:LANHostCfg1 + /x_lanhostconfigmanagement.xml + /control?LANHostConfigManagement + /event?LANHostConfigManagement + + + + + http://{{listenAddr}} + + + `, + // The response to our GetNATRSIPStatus call. This + // particular implementation has a bug where the elements + // inside u:GetNATRSIPStatusResponse are not properly + // namespaced. + "POST /control?WANIPConnection": ` + + + + 0 + 1 + + + + `, + }, + } + if err := dev.listen(); err != nil { + t.Skipf("cannot listen: %v", err) + } + dev.serve() + defer dev.close() + + // Attempt to discover the fake device. + discovered := discoverUPnP() + if discovered == nil { + t.Fatalf("not discovered") + } + upnp, _ := discovered.(*upnp) + if upnp.service != "IGDv1-IP1" { + t.Errorf("upnp.service mismatch: got %q, want %q", upnp.service, "IGDv1-IP1") + } + wantURL := "http://" + dev.listener.Addr().String() + "/InternetGatewayDevice.xml" + if upnp.dev.URLBaseStr != wantURL { + t.Errorf("upnp.dev.URLBaseStr mismatch: got %q, want %q", upnp.dev.URLBaseStr, wantURL) + } +} + +// fakeIGD presents itself as a discoverable UPnP device which sends +// canned responses to HTTPU and HTTP requests. +type fakeIGD struct { + t *testing.T // for logging + + listener net.Listener + mcastListener *net.UDPConn + + // This should be a complete HTTP response (including headers). + // It is sent as the response to any sspd packet. Any occurrence + // of "{{listenAddr}}" is replaced with the actual TCP listen + // address of the HTTP server. + ssdpResp string + // This one should contain XML payloads for all requests + // performed. The keys contain method and path, e.g. "GET /foo/bar". + // As with ssdpResp, "{{listenAddr}}" is replaced with the TCP + // listen address. + httpResps map[string]string +} + +// httpu.Handler +func (dev *fakeIGD) ServeMessage(r *http.Request) { + dev.t.Logf(`HTTPU request %s %s`, r.Method, r.RequestURI) + conn, err := net.Dial("udp4", r.RemoteAddr) + if err != nil { + fmt.Printf("reply Dial error: %v", err) + return + } + defer conn.Close() + io.WriteString(conn, dev.replaceListenAddr(dev.ssdpResp)) +} + +// http.Handler +func (dev *fakeIGD) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if resp, ok := dev.httpResps[r.Method+" "+r.RequestURI]; ok { + dev.t.Logf(`HTTP request "%s %s" --> %d`, r.Method, r.RequestURI, 200) + io.WriteString(w, dev.replaceListenAddr(resp)) + } else { + dev.t.Logf(`HTTP request "%s %s" --> %d`, r.Method, r.RequestURI, 404) + w.WriteHeader(http.StatusNotFound) + } +} + +func (dev *fakeIGD) replaceListenAddr(resp string) string { + return strings.Replace(resp, "{{listenAddr}}", dev.listener.Addr().String(), -1) +} + +func (dev *fakeIGD) listen() (err error) { + if dev.listener, err = net.Listen("tcp", "127.0.0.1:0"); err != nil { + return err + } + laddr := &net.UDPAddr{IP: net.ParseIP("239.255.255.250"), Port: 1900} + if dev.mcastListener, err = net.ListenMulticastUDP("udp", nil, laddr); err != nil { + dev.listener.Close() + return err + } + return nil +} + +func (dev *fakeIGD) serve() { + go httpu.Serve(dev.mcastListener, dev) + go http.Serve(dev.listener, dev) +} + +func (dev *fakeIGD) close() { + dev.mcastListener.Close() + dev.listener.Close() +} -- cgit v1.2.3 From c14de2e9733685e60f165ee5f2dd1f7c17f3f6cd Mon Sep 17 00:00:00 2001 From: Felix Lange Date: Thu, 14 May 2015 12:54:59 +0200 Subject: p2p/nat: tweak port mapping log messages and levels People stil get confused about the messages. This commit changes the levels so that the only thing printed at the default level (info) is a successful mapping. --- p2p/nat/nat.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) (limited to 'p2p/nat') diff --git a/p2p/nat/nat.go b/p2p/nat/nat.go index 58d50976d..9acb34398 100644 --- a/p2p/nat/nat.go +++ b/p2p/nat/nat.go @@ -86,13 +86,13 @@ func Map(m Interface, c chan struct{}, protocol string, extport, intport int, na refresh := time.NewTimer(mapUpdateInterval) defer func() { refresh.Stop() - glog.V(logger.Debug).Infof("Deleting port mapping: %s %d -> %d (%s) using %s\n", protocol, extport, intport, name, m) + glog.V(logger.Debug).Infof("deleting port mapping: %s %d -> %d (%s) using %s\n", protocol, extport, intport, name, m) m.DeleteMapping(protocol, extport, intport) }() - glog.V(logger.Debug).Infof("add mapping: %s %d -> %d (%s) using %s\n", protocol, extport, intport, name, m) if err := m.AddMapping(protocol, intport, extport, name, mapTimeout); err != nil { - glog.V(logger.Warn).Infof("network port %d could not be mapped: %v\n", intport, err) - glog.V(logger.Debug).Infof("mapping with %v returned %v\n", m, err) + glog.V(logger.Debug).Infof("network port %s:%d could not be mapped: %v\n", protocol, intport, err) + } else { + glog.V(logger.Info).Infof("mapped network port %s:%d -> %d (%s) using %s\n", protocol, extport, intport, name, m) } for { select { @@ -101,10 +101,9 @@ func Map(m Interface, c chan struct{}, protocol string, extport, intport int, na return } case <-refresh.C: - glog.V(logger.Detail).Infof("refresh mapping: %s %d -> %d (%s) using %s\n", protocol, extport, intport, name, m) + glog.V(logger.Detail).Infof("refresh port mapping %s:%d -> %d (%s) using %s\n", protocol, extport, intport, name, m) if err := m.AddMapping(protocol, intport, extport, name, mapTimeout); err != nil { - glog.V(logger.Warn).Infof("network port %d could not be mapped: %v\n", intport, err) - glog.V(logger.Debug).Infof("mapping with %v returned %v\n", m, err) + glog.V(logger.Debug).Infof("network port %s:%d could not be mapped: %v\n", protocol, intport, err) } refresh.Reset(mapUpdateInterval) } -- cgit v1.2.3