From bcf17cef1d59b0fe93d76e3beb6a8c7e750a3e6f Mon Sep 17 00:00:00 2001 From: David Vorick Date: Sun, 6 Sep 2015 09:25:12 -0400 Subject: [PATCH 1/8] catch the '::1' address --- modules/hostdb/hostentry.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/modules/hostdb/hostentry.go b/modules/hostdb/hostentry.go index e902eebfd8..f7604b7d87 100644 --- a/modules/hostdb/hostentry.go +++ b/modules/hostdb/hostentry.go @@ -23,6 +23,10 @@ func (hdb *HostDB) insertHost(host modules.HostSettings) { return } else if ip := net.ParseIP(string(host.IPAddress)); ip != nil && ip.IsLoopback() { return + } else if host.IPAddress.Host() == "::1" { + // `if ip := net.ParseIP(string(host.IPAddress)); ip != nil && + // ip.IsLoopback()` will not catch "::1". + return } // Add the host to allHosts. From 05589d3a1385b456df0a6b4210e8dde585eb5adb Mon Sep 17 00:00:00 2001 From: David Vorick Date: Sun, 6 Sep 2015 10:18:27 -0400 Subject: [PATCH 2/8] add RemovePort function to NetAddress --- Makefile | 9 ++++----- modules/gateway.go | 13 +++++++++++++ modules/gateway_test.go | 30 ++++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 5 deletions(-) create mode 100644 modules/gateway_test.go diff --git a/Makefile b/Makefile index c85116ef10..1bb63cf4ba 100644 --- a/Makefile +++ b/Makefile @@ -62,11 +62,10 @@ clean: # command. 'test' runs short tests that should last no more than a few seconds, # 'test-long' runs more thorough tests which should not last more than a few # minutes. -pkgs = ./api ./compatibility ./crypto ./encoding ./modules/consensus \ - ./modules/gateway ./modules/host ./modules/hostdb \ - ./modules/miner ./modules/renter ./modules/transactionpool \ - ./modules/wallet ./modules/explorer ./persist \ - ./siae ./types +pkgs = ./api ./compatibility ./crypto ./encoding ./modules ./modules/consensus \ + ./modules/explorer ./modules/gateway ./modules/host ./modules/hostdb \ + ./modules/miner ./modules/renter ./modules/transactionpool \ + ./modules/wallet ./persist ./siae ./types test: clean fmt REBUILD go test -short -tags='debug testing' -timeout=10s $(pkgs) test-v: clean fmt REBUILD diff --git a/modules/gateway.go b/modules/gateway.go index eec069aec8..ba5e02c173 100644 --- a/modules/gateway.go +++ b/modules/gateway.go @@ -47,6 +47,19 @@ func (na NetAddress) Port() string { return port } +// RemovePort removes the port from a NetAddress. Due to error checking, the +// behavior is different from 'Host()' for IPAddresses that don't have a port. +// This function should probably be merged with Host(), but I wasn't sure if +// portions of the gateway relied on getting 'nil' if no port was present in +// the NetAddress. +func (na NetAddress) RemovePort() string { + host, _, err := net.SplitHostPort(string(na)) + if err != nil { + return string(na) + } + return host +} + // A Gateway facilitates the interactions between the local node and remote // nodes (peers). It relays incoming blocks and transactions to local modules, // and broadcasts outgoing blocks and transactions to peers. In a broad sense, diff --git a/modules/gateway_test.go b/modules/gateway_test.go new file mode 100644 index 0000000000..536b4bd016 --- /dev/null +++ b/modules/gateway_test.go @@ -0,0 +1,30 @@ +package modules + +import ( + "testing" +) + +// TestRemovePort tests the RemovePort method of the NetAddress. +func TestRemovePort(t *testing.T) { + testSet := []struct { + query NetAddress + desiredResponse string + }{ + {"localhost", "localhost"}, + {"localhost:1234", "localhost"}, + {"127.0.0.1", "127.0.0.1"}, + {"127.0.0.1:6723", "127.0.0.1"}, + {"bbc.com", "bbc.com"}, + {"bbc.com:6434", "bbc.com"}, + {"bitcoin.ninja", "bitcoin.ninja"}, + {"bitcoin.ninja:6752", "bitcoin.ninja"}, + {"garbage:64:77", "garbage:64:77"}, + {"::1:5856", "::1:5856"}, + {"[::1]:5856", "::1"}, + } + for _, test := range testSet { + if test.query.RemovePort() != test.desiredResponse { + t.Error("test failed:", test, test.query.RemovePort()) + } + } +} From 9d202e5c9c72a83554657ef3b704ce639efb2731 Mon Sep 17 00:00:00 2001 From: David Vorick Date: Sun, 6 Sep 2015 12:05:43 -0400 Subject: [PATCH 3/8] add checks to hostdb for local and garbage hostnames --- modules/gateway.go | 28 ---------- modules/gateway_test.go | 30 ---------- modules/hostdb/hostentry.go | 16 +++--- modules/netaddress.go | 81 +++++++++++++++++++++++++++ modules/netaddress_test.go | 107 ++++++++++++++++++++++++++++++++++++ 5 files changed, 195 insertions(+), 67 deletions(-) delete mode 100644 modules/gateway_test.go create mode 100644 modules/netaddress.go create mode 100644 modules/netaddress_test.go diff --git a/modules/gateway.go b/modules/gateway.go index ba5e02c173..07f4c756de 100644 --- a/modules/gateway.go +++ b/modules/gateway.go @@ -32,34 +32,6 @@ type PeerConn interface { // keeping the connection open after all necessary I/O has been performed. type RPCFunc func(PeerConn) error -// A NetAddress contains the information needed to contact a peer. -type NetAddress string - -// Host returns the NetAddress' IP. -func (na NetAddress) Host() string { - host, _, _ := net.SplitHostPort(string(na)) - return host -} - -// Port returns the NetAddress' port number. -func (na NetAddress) Port() string { - _, port, _ := net.SplitHostPort(string(na)) - return port -} - -// RemovePort removes the port from a NetAddress. Due to error checking, the -// behavior is different from 'Host()' for IPAddresses that don't have a port. -// This function should probably be merged with Host(), but I wasn't sure if -// portions of the gateway relied on getting 'nil' if no port was present in -// the NetAddress. -func (na NetAddress) RemovePort() string { - host, _, err := net.SplitHostPort(string(na)) - if err != nil { - return string(na) - } - return host -} - // A Gateway facilitates the interactions between the local node and remote // nodes (peers). It relays incoming blocks and transactions to local modules, // and broadcasts outgoing blocks and transactions to peers. In a broad sense, diff --git a/modules/gateway_test.go b/modules/gateway_test.go deleted file mode 100644 index 536b4bd016..0000000000 --- a/modules/gateway_test.go +++ /dev/null @@ -1,30 +0,0 @@ -package modules - -import ( - "testing" -) - -// TestRemovePort tests the RemovePort method of the NetAddress. -func TestRemovePort(t *testing.T) { - testSet := []struct { - query NetAddress - desiredResponse string - }{ - {"localhost", "localhost"}, - {"localhost:1234", "localhost"}, - {"127.0.0.1", "127.0.0.1"}, - {"127.0.0.1:6723", "127.0.0.1"}, - {"bbc.com", "bbc.com"}, - {"bbc.com:6434", "bbc.com"}, - {"bitcoin.ninja", "bitcoin.ninja"}, - {"bitcoin.ninja:6752", "bitcoin.ninja"}, - {"garbage:64:77", "garbage:64:77"}, - {"::1:5856", "::1:5856"}, - {"[::1]:5856", "::1"}, - } - for _, test := range testSet { - if test.query.RemovePort() != test.desiredResponse { - t.Error("test failed:", test, test.query.RemovePort()) - } - } -} diff --git a/modules/hostdb/hostentry.go b/modules/hostdb/hostentry.go index f7604b7d87..63999107c5 100644 --- a/modules/hostdb/hostentry.go +++ b/modules/hostdb/hostentry.go @@ -1,8 +1,7 @@ package hostdb import ( - "net" - + "github.com/NebulousLabs/Sia/build" "github.com/NebulousLabs/Sia/modules" "github.com/NebulousLabs/Sia/types" ) @@ -17,15 +16,14 @@ type hostEntry struct { // insert adds a host entry to the state. The host will be inserted into the // set of all hosts, and if it is online and responding to requests it will be // put into the list of active hosts. +// +// TODO: Function should return an error. func (hdb *HostDB) insertHost(host modules.HostSettings) { - // Blacklist localhost. - if host.IPAddress.Host() == "localhost" { - return - } else if ip := net.ParseIP(string(host.IPAddress)); ip != nil && ip.IsLoopback() { + // Remove garbage hosts and local hosts. + if !host.IPAddress.IsValid() { return - } else if host.IPAddress.Host() == "::1" { - // `if ip := net.ParseIP(string(host.IPAddress)); ip != nil && - // ip.IsLoopback()` will not catch "::1". + } + if host.IPAddress.IsLocal() && build.Release != "testing" { return } diff --git a/modules/netaddress.go b/modules/netaddress.go new file mode 100644 index 0000000000..61a5149769 --- /dev/null +++ b/modules/netaddress.go @@ -0,0 +1,81 @@ +package modules + +import ( + "net" + "regexp" + + "github.com/NebulousLabs/Sia/build" +) + +// A NetAddress contains the information needed to contact a peer. +type NetAddress string + +// Host returns the NetAddress' IP. +// +// TODO: Unchecked error resulted in bugs in production code! +func (na NetAddress) Host() string { + host, _, _ := net.SplitHostPort(string(na)) + return host +} + +// Port returns the NetAddress' port number. +// +// TODO: Unchecked error. +func (na NetAddress) Port() string { + _, port, _ := net.SplitHostPort(string(na)) + return port +} + +// RemovePort removes the port from a NetAddress. Due to error checking, the +// behavior is different from 'Host()' for IPAddresses that don't have a port. +// This function should probably be merged with Host(), but I wasn't sure if +// portions of the gateway relied on getting 'nil' if no port was present in +// the NetAddress. +func (na NetAddress) RemovePort() string { + host, _, err := net.SplitHostPort(string(na)) + if err != nil { + return string(na) + } + return host +} + +// IsLocal returns true for ip addresses that are on the same machine. +func (na NetAddress) IsLocal() bool { + if !na.IsValid() { + return false + } + + host := na.RemovePort() + if ip := net.ParseIP(host); ip != nil && ip.IsLoopback() { + return true + } + if host == "localhost" { + return true + } + return false +} + +// IsValid uses a regex to check whether the net address is a valid ip address +// or hostname. +func (na NetAddress) IsValid() bool { + host := na.RemovePort() + // Check if the host is a valid ip address. + if net.ParseIP(host) != nil { + return true + } + + // A regex pulled from + // http://stackoverflow.com/questions/106179/regular-expression-to-match-dns-hostname-or-ip-address + // to check for a valid hostname. + regHostname, err := regexp.Compile("^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]{0,61}[a-zA-Z0-9])(\\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]{0,61}[a-zA-Z0-9]))*$") + if build.DEBUG && err != nil { + panic(err) + } + if err != nil { + return false + } + if regHostname.MatchString(host) { + return true + } + return false +} diff --git a/modules/netaddress_test.go b/modules/netaddress_test.go new file mode 100644 index 0000000000..c7fa663c87 --- /dev/null +++ b/modules/netaddress_test.go @@ -0,0 +1,107 @@ +package modules + +import ( + "testing" +) + +// TestRemovePort tests the RemovePort method of the NetAddress type. +func TestRemovePort(t *testing.T) { + testSet := []struct { + query NetAddress + desiredResponse string + }{ + {"localhost", "localhost"}, + {"localhost:1234", "localhost"}, + {"127.0.0.1", "127.0.0.1"}, + {"127.0.0.1:6723", "127.0.0.1"}, + {"bbc.com", "bbc.com"}, + {"bbc.com:6434", "bbc.com"}, + {"bitcoin.ninja", "bitcoin.ninja"}, + {"bitcoin.ninja:6752", "bitcoin.ninja"}, + {"garbage:64:77", "garbage:64:77"}, + {"::1:5856", "::1:5856"}, + {"[::1]:5856", "::1"}, + {"[::1]", "[::1]"}, + {"::1", "::1"}, + } + for _, test := range testSet { + if test.query.RemovePort() != test.desiredResponse { + t.Error("test failed:", test, test.query.RemovePort()) + } + } +} + +// TestIsLocal tests the IsLocal method of the NetAddress type. +func TestIsLocal(t *testing.T) { + testSet := []struct { + query NetAddress + desiredResponse bool + }{ + // Networks such as 10.0.0.x have been omitted from testing - behavior + // for these networks is currently undefined. + + // Localhost tests. + {"localhost", true}, + {"localhost:1234", true}, + {"127.0.0.1", true}, + {"127.0.0.1:6723", true}, + {"::1", true}, + {"[::1]:7124", true}, + + // Public name tests. + {"hn.com", false}, + {"hn.com:8811", false}, + {"12.34.45.64", false}, + {"12.34.45.64:7777", false}, + {"::1:4646", false}, // TODO: I'm not sure if this is actually localhost. The trailing values are part of the IPAddress, not part of the port. + + // Garbage name tests. + {"garbage", false}, + {"garbage:6432", false}, + {"garbage:6146:616", false}, + {"[::1]", false}, + } + for _, test := range testSet { + if test.query.IsLocal() != test.desiredResponse { + t.Error("test failed:", test, test.query.IsLocal()) + } + } +} + +// TestIsValid checks where a netaddress matches the regex for what counts as a +// valid hostname or ip address. +func TestIsValid(t *testing.T) { + testSet := []struct { + query NetAddress + desiredResponse bool + }{ + // Networks such as 10.0.0.x have been omitted from testing - behavior + // for these networks is currently undefined. + + // Localhost tests. + {"localhost", true}, + {"localhost:1234", true}, + {"127.0.0.1", true}, + {"127.0.0.1:6723", true}, + {"::1", true}, + {"[::1]:7124", true}, + + // Public name tests. + {"hn.com", true}, + {"hn.com:8811", true}, + {"12.34.45.64", true}, + {"12.34.45.64:7777", true}, + {"::1:4646", true}, + {"plain", true}, + {"plain:6432", true}, + + // Garbage name tests. + {"garbage:6146:616", false}, + {"[::1]", false}, + } + for _, test := range testSet { + if test.query.IsValid() != test.desiredResponse { + t.Error("test failed:", test, test.query.IsValid()) + } + } +} From f57ff4c44163d0d413e793d25703ff77ec73b363 Mon Sep 17 00:00:00 2001 From: David Vorick Date: Sun, 6 Sep 2015 14:16:56 -0400 Subject: [PATCH 4/8] add logger to renter also reorganize the persistance code a bit --- modules/renter/files.go | 2 +- modules/renter/persist.go | 36 +++++++++++++++++++++++++++++----- modules/renter/persist_test.go | 2 +- modules/renter/renter.go | 21 ++++++++------------ modules/renter/renter_test.go | 12 ++++++------ modules/renter/upload.go | 6 +++--- 6 files changed, 50 insertions(+), 29 deletions(-) diff --git a/modules/renter/files.go b/modules/renter/files.go index 499835f37c..17ef404093 100644 --- a/modules/renter/files.go +++ b/modules/renter/files.go @@ -135,7 +135,7 @@ func (r *Renter) DeleteFile(nickname string) error { } delete(r.files, nickname) - os.Remove(filepath.Join(r.saveDir, f.name+ShareExtension)) + os.Remove(filepath.Join(r.persistDir, f.name+ShareExtension)) r.save() return nil diff --git a/modules/renter/persist.go b/modules/renter/persist.go index bfc8d59a5e..f3bb01f529 100644 --- a/modules/renter/persist.go +++ b/modules/renter/persist.go @@ -6,6 +6,7 @@ import ( "encoding/base64" "errors" "io" + "log" "os" "path/filepath" "strconv" @@ -124,7 +125,7 @@ func (f *file) load(r io.Reader) error { // saveFile saves a file to the renter directory. func (r *Renter) saveFile(f *file) error { - handle, err := persist.NewSafeFile(filepath.Join(r.saveDir, f.name+ShareExtension)) + handle, err := persist.NewSafeFile(filepath.Join(r.persistDir, f.name+ShareExtension)) if err != nil { return err } @@ -162,13 +163,13 @@ func (r *Renter) save() error { b, _ := id.MarshalJSON() data.Contracts[string(b)] = fc } - return persist.SaveFile(saveMetadata, data, filepath.Join(r.saveDir, PersistFilename)) + return persist.SaveFile(saveMetadata, data, filepath.Join(r.persistDir, PersistFilename)) } // load fetches the saved renter data from disk. func (r *Renter) load() error { // Load all files found in renter directory. - dir, err := os.Open(r.saveDir) // TODO: store in a subdir? + dir, err := os.Open(r.persistDir) // TODO: store in a subdir? if err != nil { return err } @@ -181,7 +182,7 @@ func (r *Renter) load() error { if filepath.Ext(path) != ShareExtension { continue } - file, err := os.Open(filepath.Join(r.saveDir, path)) + file, err := os.Open(filepath.Join(r.persistDir, path)) if err != nil { // maybe just skip? return err @@ -198,7 +199,7 @@ func (r *Renter) load() error { Contracts map[string]types.FileContract Entropy [32]byte }{} - err = persist.LoadFile(saveMetadata, &data, filepath.Join(r.saveDir, PersistFilename)) + err = persist.LoadFile(saveMetadata, &data, filepath.Join(r.persistDir, PersistFilename)) if err != nil { return err } @@ -346,6 +347,31 @@ func (r *Renter) loadSharedFiles(reader io.Reader) ([]string, error) { return names, nil } +// initPersist handles all of the persistence initialization, such as creating +// the persistance directory and starting the logger. +func (r *Renter) initPersist() error { + // Create the perist directory if it does not yet exist. + err := os.MkdirAll(r.persistDir, 0700) + if err != nil { + return err + } + + // Initialize the logger. + logFile, err := os.OpenFile(filepath.Join(r.persistDir, "renter.log"), os.O_WRONLY|os.O_CREATE|os.O_APPEND, 0660) + if err != nil { + return err + } + r.log = log.New(logFile, "", log.Ldate|log.Ltime|log.Lmicroseconds|log.Lshortfile) + r.log.Println("STARTUP: Renter has started logging") + + // Load the prior persistance structures. + err = r.load() + if err != nil && !os.IsNotExist(err) { + return err + } + return nil +} + // LoadSharedFiles loads a .sia file into the renter. It returns the nicknames // of the loaded files. func (r *Renter) LoadSharedFiles(filename string) ([]string, error) { diff --git a/modules/renter/persist_test.go b/modules/renter/persist_test.go index e1329b9950..893c893001 100644 --- a/modules/renter/persist_test.go +++ b/modules/renter/persist_test.go @@ -158,7 +158,7 @@ func TestRenterSaveLoad(t *testing.T) { } // Corrupt a renter file and try to reload it. - err = ioutil.WriteFile(filepath.Join(rt.renter.saveDir, "corrupt"+ShareExtension), []byte{1, 2, 3}, 0660) + err = ioutil.WriteFile(filepath.Join(rt.renter.persistDir, "corrupt"+ShareExtension), []byte{1, 2, 3}, 0660) if err != nil { t.Fatal(err) } diff --git a/modules/renter/renter.go b/modules/renter/renter.go index 16224dc743..5a17bd45cd 100644 --- a/modules/renter/renter.go +++ b/modules/renter/renter.go @@ -3,7 +3,7 @@ package renter import ( "crypto/rand" "errors" - "os" + "log" "github.com/NebulousLabs/Sia/modules" "github.com/NebulousLabs/Sia/sync" @@ -30,13 +30,14 @@ type Renter struct { contracts map[types.FileContractID]types.FileContract entropy [32]byte // used to generate signing keys downloadQueue []*download - saveDir string - mu *sync.RWMutex + persistDir string + log *log.Logger + mu *sync.RWMutex } // New returns an empty renter. -func New(cs modules.ConsensusSet, hdb modules.HostDB, wallet modules.Wallet, tpool modules.TransactionPool, saveDir string) (*Renter, error) { +func New(cs modules.ConsensusSet, hdb modules.HostDB, wallet modules.Wallet, tpool modules.TransactionPool, persistDir string) (*Renter, error) { if cs == nil { return nil, ErrNilCS } @@ -58,25 +59,19 @@ func New(cs modules.ConsensusSet, hdb modules.HostDB, wallet modules.Wallet, tpo files: make(map[string]*file), contracts: make(map[types.FileContractID]types.FileContract), - saveDir: saveDir, - mu: sync.New(modules.SafeMutexDelay, 1), + persistDir: persistDir, + mu: sync.New(modules.SafeMutexDelay, 1), } _, err := rand.Read(r.entropy[:]) if err != nil { return nil, err } - - err = os.MkdirAll(saveDir, 0700) + err = r.initPersist() if err != nil { return nil, err } - err = r.load() - if err != nil && !os.IsNotExist(err) { - return nil, err - } - r.cs.ConsensusSetSubscribe(r) return r, nil diff --git a/modules/renter/renter_test.go b/modules/renter/renter_test.go index 2a224a341f..8d7b042f93 100644 --- a/modules/renter/renter_test.go +++ b/modules/renter/renter_test.go @@ -128,27 +128,27 @@ func TestNilInputs(t *testing.T) { if err != nil { t.Fatal(err) } - _, err = New(rt.cs, rt.hostdb, rt.wallet, rt.tpool, rt.renter.saveDir+"1") + _, err = New(rt.cs, rt.hostdb, rt.wallet, rt.tpool, rt.renter.persistDir+"1") if err != nil { t.Error(err) } - _, err = New(nil, nil, nil, nil, rt.renter.saveDir+"2") + _, err = New(nil, nil, nil, nil, rt.renter.persistDir+"2") if err == nil { t.Error("no error returned for nil inputs") } - _, err = New(nil, rt.hostdb, rt.wallet, rt.tpool, rt.renter.saveDir+"3") + _, err = New(nil, rt.hostdb, rt.wallet, rt.tpool, rt.renter.persistDir+"3") if err != ErrNilCS { t.Error(err) } - _, err = New(rt.cs, nil, rt.wallet, rt.tpool, rt.renter.saveDir+"5") + _, err = New(rt.cs, nil, rt.wallet, rt.tpool, rt.renter.persistDir+"5") if err != ErrNilHostDB { t.Error(err) } - _, err = New(rt.cs, rt.hostdb, nil, rt.tpool, rt.renter.saveDir+"6") + _, err = New(rt.cs, rt.hostdb, nil, rt.tpool, rt.renter.persistDir+"6") if err != ErrNilWallet { t.Error(err) } - _, err = New(rt.cs, rt.hostdb, rt.wallet, nil, rt.renter.saveDir+"6") + _, err = New(rt.cs, rt.hostdb, rt.wallet, nil, rt.renter.persistDir+"6") if err != ErrNilTpool { t.Error(err) } diff --git a/modules/renter/upload.go b/modules/renter/upload.go index 37b193eb9b..45d1b1f364 100644 --- a/modules/renter/upload.go +++ b/modules/renter/upload.go @@ -183,12 +183,12 @@ func (r *Renter) Upload(up modules.FileUploadParams) error { totalsize := up.PieceSize * uint64(up.ErasureCode.NumPieces()) * f.numChunks() var hosts []uploader for _, host := range r.hostDB.RandomHosts(up.ErasureCode.NumPieces() * 3 / 2) { - host, err := r.newHostUploader(host, totalsize, up.Duration, f.masterKey) + hostUploader, err := r.newHostUploader(host, totalsize, up.Duration, f.masterKey) if err != nil { continue } - defer host.Close() - hosts = append(hosts, host) + defer hostUploader.Close() + hosts = append(hosts, hostUploader) } if len(hosts) < up.ErasureCode.MinPieces() { return errors.New("not enough hosts to support upload") From bb2bb0f72063923adeb2ce82598ad23a5c4cb879 Mon Sep 17 00:00:00 2001 From: David Vorick Date: Sun, 6 Sep 2015 19:00:52 -0400 Subject: [PATCH 5/8] merge Host and RemovePort --- modules/netaddress.go | 22 +++++----------------- modules/netaddress_test.go | 8 ++++---- 2 files changed, 9 insertions(+), 21 deletions(-) diff --git a/modules/netaddress.go b/modules/netaddress.go index 61a5149769..79f587ae61 100644 --- a/modules/netaddress.go +++ b/modules/netaddress.go @@ -10,11 +10,12 @@ import ( // A NetAddress contains the information needed to contact a peer. type NetAddress string -// Host returns the NetAddress' IP. -// -// TODO: Unchecked error resulted in bugs in production code! +// Host removes the port from a NetAddress, returning just the host. func (na NetAddress) Host() string { - host, _, _ := net.SplitHostPort(string(na)) + host, _, err := net.SplitHostPort(string(na)) + if err != nil { + return string(na) + } return host } @@ -26,19 +27,6 @@ func (na NetAddress) Port() string { return port } -// RemovePort removes the port from a NetAddress. Due to error checking, the -// behavior is different from 'Host()' for IPAddresses that don't have a port. -// This function should probably be merged with Host(), but I wasn't sure if -// portions of the gateway relied on getting 'nil' if no port was present in -// the NetAddress. -func (na NetAddress) RemovePort() string { - host, _, err := net.SplitHostPort(string(na)) - if err != nil { - return string(na) - } - return host -} - // IsLocal returns true for ip addresses that are on the same machine. func (na NetAddress) IsLocal() bool { if !na.IsValid() { diff --git a/modules/netaddress_test.go b/modules/netaddress_test.go index c7fa663c87..508df3590b 100644 --- a/modules/netaddress_test.go +++ b/modules/netaddress_test.go @@ -4,8 +4,8 @@ import ( "testing" ) -// TestRemovePort tests the RemovePort method of the NetAddress type. -func TestRemovePort(t *testing.T) { +// TestHost tests the Host method of the NetAddress type. +func TestHost(t *testing.T) { testSet := []struct { query NetAddress desiredResponse string @@ -25,8 +25,8 @@ func TestRemovePort(t *testing.T) { {"::1", "::1"}, } for _, test := range testSet { - if test.query.RemovePort() != test.desiredResponse { - t.Error("test failed:", test, test.query.RemovePort()) + if test.query.Host() != test.desiredResponse { + t.Error("test failed:", test, test.query.Host()) } } } From 6765d6afc9323c3fbfb82de0b88d76878ac1eae0 Mon Sep 17 00:00:00 2001 From: David Vorick Date: Sun, 6 Sep 2015 22:52:00 -0400 Subject: [PATCH 6/8] altered behavior of net address methods --- modules/netaddress.go | 21 ++++++++++++-------- modules/netaddress_test.go | 40 ++++++++++++++++++++------------------ 2 files changed, 34 insertions(+), 27 deletions(-) diff --git a/modules/netaddress.go b/modules/netaddress.go index 79f587ae61..85d95d76fc 100644 --- a/modules/netaddress.go +++ b/modules/netaddress.go @@ -10,20 +10,25 @@ import ( // A NetAddress contains the information needed to contact a peer. type NetAddress string -// Host removes the port from a NetAddress, returning just the host. +// Host removes the port from a NetAddress, returning just the host. If the +// address is invalid, the empty string is returned. func (na NetAddress) Host() string { host, _, err := net.SplitHostPort(string(na)) + // 'host' is not always the empty string if an error is returned. if err != nil { - return string(na) + return "" } return host } -// Port returns the NetAddress' port number. -// -// TODO: Unchecked error. +// Port returns the NetAddress object's port number. The empty string is +// returned if the NetAddress is invalid. func (na NetAddress) Port() string { - _, port, _ := net.SplitHostPort(string(na)) + _, port, err := net.SplitHostPort(string(na)) + // 'port' will not always be the empty string if an error is returned. + if err != nil { + return "" + } return port } @@ -33,7 +38,7 @@ func (na NetAddress) IsLocal() bool { return false } - host := na.RemovePort() + host := na.Host() if ip := net.ParseIP(host); ip != nil && ip.IsLoopback() { return true } @@ -46,7 +51,7 @@ func (na NetAddress) IsLocal() bool { // IsValid uses a regex to check whether the net address is a valid ip address // or hostname. func (na NetAddress) IsValid() bool { - host := na.RemovePort() + host := na.Host() // Check if the host is a valid ip address. if net.ParseIP(host) != nil { return true diff --git a/modules/netaddress_test.go b/modules/netaddress_test.go index 508df3590b..428daf4c6b 100644 --- a/modules/netaddress_test.go +++ b/modules/netaddress_test.go @@ -10,19 +10,19 @@ func TestHost(t *testing.T) { query NetAddress desiredResponse string }{ - {"localhost", "localhost"}, + {"localhost", ""}, {"localhost:1234", "localhost"}, - {"127.0.0.1", "127.0.0.1"}, + {"127.0.0.1", ""}, {"127.0.0.1:6723", "127.0.0.1"}, - {"bbc.com", "bbc.com"}, + {"bbc.com", ""}, {"bbc.com:6434", "bbc.com"}, - {"bitcoin.ninja", "bitcoin.ninja"}, + {"bitcoin.ninja", ""}, {"bitcoin.ninja:6752", "bitcoin.ninja"}, - {"garbage:64:77", "garbage:64:77"}, - {"::1:5856", "::1:5856"}, + {"garbage:64:77", ""}, + {"::1:5856", ""}, {"[::1]:5856", "::1"}, - {"[::1]", "[::1]"}, - {"::1", "::1"}, + {"[::1]", ""}, + {"::1", ""}, } for _, test := range testSet { if test.query.Host() != test.desiredResponse { @@ -41,11 +41,11 @@ func TestIsLocal(t *testing.T) { // for these networks is currently undefined. // Localhost tests. - {"localhost", true}, + {"localhost", false}, {"localhost:1234", true}, - {"127.0.0.1", true}, + {"127.0.0.1", false}, {"127.0.0.1:6723", true}, - {"::1", true}, + {"::1", false}, {"[::1]:7124", true}, // Public name tests. @@ -53,9 +53,10 @@ func TestIsLocal(t *testing.T) { {"hn.com:8811", false}, {"12.34.45.64", false}, {"12.34.45.64:7777", false}, - {"::1:4646", false}, // TODO: I'm not sure if this is actually localhost. The trailing values are part of the IPAddress, not part of the port. + {"::1:4646", false}, // Garbage name tests. + {"", false}, {"garbage", false}, {"garbage:6432", false}, {"garbage:6146:616", false}, @@ -79,23 +80,24 @@ func TestIsValid(t *testing.T) { // for these networks is currently undefined. // Localhost tests. - {"localhost", true}, + {"localhost", false}, {"localhost:1234", true}, - {"127.0.0.1", true}, + {"127.0.0.1", false}, {"127.0.0.1:6723", true}, - {"::1", true}, + {"::1", false}, {"[::1]:7124", true}, // Public name tests. - {"hn.com", true}, + {"hn.com", false}, {"hn.com:8811", true}, - {"12.34.45.64", true}, + {"12.34.45.64", false}, {"12.34.45.64:7777", true}, - {"::1:4646", true}, - {"plain", true}, + {"::1:4646", false}, + {"plain", false}, {"plain:6432", true}, // Garbage name tests. + {"", false}, {"garbage:6146:616", false}, {"[::1]", false}, } From b040ad87b944a1c852cd63b87faee8483d3903f2 Mon Sep 17 00:00:00 2001 From: David Vorick Date: Sun, 6 Sep 2015 23:36:03 -0400 Subject: [PATCH 7/8] better comment for NetAddress.IsValid --- modules/netaddress.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/netaddress.go b/modules/netaddress.go index 85d95d76fc..68940a904b 100644 --- a/modules/netaddress.go +++ b/modules/netaddress.go @@ -52,7 +52,9 @@ func (na NetAddress) IsLocal() bool { // or hostname. func (na NetAddress) IsValid() bool { host := na.Host() - // Check if the host is a valid ip address. + // Check if the host is a valid ip address. Host will have been returned as + // the empty string (which is not a valid ip address) if there is anything + // structurally incorrect with the NetAddress. if net.ParseIP(host) != nil { return true } From 5a37098d1579787be8089a0e0dc4f7a65afde580 Mon Sep 17 00:00:00 2001 From: David Vorick Date: Mon, 7 Sep 2015 00:54:39 -0400 Subject: [PATCH 8/8] give up on hostname validation --- modules/netaddress.go | 30 +++--------------------------- modules/netaddress_test.go | 1 + 2 files changed, 4 insertions(+), 27 deletions(-) diff --git a/modules/netaddress.go b/modules/netaddress.go index 68940a904b..2c7cb34c4a 100644 --- a/modules/netaddress.go +++ b/modules/netaddress.go @@ -2,9 +2,6 @@ package modules import ( "net" - "regexp" - - "github.com/NebulousLabs/Sia/build" ) // A NetAddress contains the information needed to contact a peer. @@ -48,29 +45,8 @@ func (na NetAddress) IsLocal() bool { return false } -// IsValid uses a regex to check whether the net address is a valid ip address -// or hostname. +// IsValid does nothing. Please ignore it. func (na NetAddress) IsValid() bool { - host := na.Host() - // Check if the host is a valid ip address. Host will have been returned as - // the empty string (which is not a valid ip address) if there is anything - // structurally incorrect with the NetAddress. - if net.ParseIP(host) != nil { - return true - } - - // A regex pulled from - // http://stackoverflow.com/questions/106179/regular-expression-to-match-dns-hostname-or-ip-address - // to check for a valid hostname. - regHostname, err := regexp.Compile("^([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]{0,61}[a-zA-Z0-9])(\\.([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]{0,61}[a-zA-Z0-9]))*$") - if build.DEBUG && err != nil { - panic(err) - } - if err != nil { - return false - } - if regHostname.MatchString(host) { - return true - } - return false + _, _, err := net.SplitHostPort(string(na)) + return err == nil } diff --git a/modules/netaddress_test.go b/modules/netaddress_test.go index 428daf4c6b..e4abb02409 100644 --- a/modules/netaddress_test.go +++ b/modules/netaddress_test.go @@ -100,6 +100,7 @@ func TestIsValid(t *testing.T) { {"", false}, {"garbage:6146:616", false}, {"[::1]", false}, + // {"google.com:notAPort", false}, TODO: Failed test case. } for _, test := range testSet { if test.query.IsValid() != test.desiredResponse {