Merge pull request #132 from fclairamb/feature/pasv-port-allocation-fix

Passive port allocation fix

Author: probot-auto-merge[bot]

.github/workflows/docker-test.yml

@@ -13,7 +13,7 @@ jobs:
       ftpserver:
         image: ftpserver/ftpserver
         ports:
-          - 2121:2121
+          - 2121-2200:2121-2200
 
     steps:
       - name: Run sample
@@ -26,22 +26,21 @@ jobs:
           sudo apt-get install netcat curl -y
 
           # Getting a file
-          curl -o file.bin https://github.com/fclairamb/ftpserver/releases/download/v0.5/ftpserver-linux-amd64
+          curl https://placekitten.com/2048/2048 -o kitty.jpg
 
           # Waiting for server to be ready
           while ! nc -z localhost ${SERVER_PORT} </dev/null; do sleep 1; done
 
           # Upload a file
-          curl -P - -T file.bin ftp://test:test@localhost:2121/remote.bin
+          curl -P - -T kitty.jpg ftp://test:test@localhost:${SERVER_PORT}/remote.jpg
 
           # Download a file
-          curl -P - ftp://test:test@localhost:2121/remote.bin -o remote.bin
+          curl -P - ftp://test:test@localhost:2121/remote.jpg -o remote.jpg
 
           # Checking that the output file exists
-          if [ ! -f remote.bin ]; then
+          if [ ! -f remote.jpg ]; then
             exit 1
           fi
 
           # Comparing file contents
-          diff file.bin remote.bin
-
+          diff kitty.jpg remote.jpg

.gitignore

@@ -2,6 +2,5 @@
 ftpserver
 .idea
 .vscode
-settings.toml
 debug
 __debug_bin

README.md

@@ -68,15 +68,11 @@ mkdir -p data
 docker run --rm -d -p 2121-2200:2121-2200 -v $(pwd)/data:/data fclairamb/ftpserver
 
 # Download some file
-if [ ! -f file.bin ]; then
-    wget -O file.bin.tmp https://github.com/fclairamb/ftpserver/releases/download/v0.5/ftpserver-linux-amd64 && mv file.bin.tmp file.bin
+if [ ! -f kitty.jpg ]; then
+    curl -o kitty.jpg.tmp https://placekitten.com/2048/2048 && mv kitty.jpg.tmp kitty.jpg
 fi
 
-# Connecting to it and uploading a file
-ftp ftp://test:test@localhost:2121
-put file.bin
-quit
-ls -lh data/file.bin
+curl -v -T kitty.bin ftp://test:test@localhost:2121/
 ```
 
 ## The driver

local-docker-test.sh

@@ -0,0 +1,12 @@
+#!/bin/sh -ex
+
+docker rm -f ftpserver ||:
+
+GOOS=linux GOARCH=amd64 go build
+docker build . -t test
+if [ ! -f kitty.jpg ]; then
+  curl -o kitty.jpg.tmp https://placekitten.com/2048/2048 && mv kitty.jpg.tmp kitty.jpg
+fi
+docker run --name ftpserver -p 2121-2200:2121-2200 test &
+while ! nc -z localhost 2121 </dev/null; do sleep 1; done
+curl -v -T kitty.jpg ftp://test:test@localhost:2121/

main.go

@@ -42,7 +42,7 @@ func main() {
 	// The general idea here is that if you start it without any arg, you're probably doing a local quick&dirty run
 	// possibly on a windows machine, so we're better of just using a default file name and create the file.
 	if confFile == "" {
-		confFile = "settings.toml"
+		confFile = "settings_test.toml"
 		autoCreate = true
 	}
 
@@ -102,8 +102,6 @@ func signalHandler() {
 
 func confFileContent() []byte {
 	str := `# ftpserver configuration file
-#
-# These are all the config parameters with their default values. If not present,
 
 # Max number of control connections to accept
 # max_connections = 0
@@ -114,7 +112,7 @@ max_connections = 10
 # listen_addr="0.0.0.0:2121"
 
 # Public host to expose in the passive connection
-# public_host = ""
+public_host = "127.0.0.1"
 
 # Idle timeout time
 # idle_timeout = 900

prepare.sh

@@ -200,7 +200,7 @@ func (driver *MainDriver) getCertificate() (*tls.Certificate, error) {
 // WelcomeUser is called to send the very first welcome message
 func (driver *MainDriver) WelcomeUser(cc server.ClientContext) (string, error) {
 	nbClients := atomic.AddInt32(&driver.nbClients, 1)
-	if nbClients > driver.config.MaxConnections {
+	if driver.config.MaxConnections != 0 && nbClients > driver.config.MaxConnections {
 		return "Cannot accept any additional client", fmt.Errorf(
 			"too many clients: %d > % d",
 			driver.nbClients,
@@ -313,8 +313,11 @@ func (driver *ClientDriver) OpenFile(cc server.ClientContext, path string, flag
 	if (flag & os.O_WRONLY) != 0 {
 		flag |= os.O_CREATE
 		if (flag & os.O_APPEND) == 0 {
-			if err := os.Remove(path); err != nil {
-				fmt.Println("Problem removing file", path, "err:", err)
+			_, err := os.Stat(path)
+			if !os.IsNotExist(err) {
+				if err := os.Remove(path); err != nil {
+					fmt.Println("Problem removing file", path, "err:", err)
+				}
 			}
 		}
 	}
@@ -415,7 +418,7 @@ func (f *virtualFile) Seek(n int64, w int) (int64, error) {
 }
 
 func (f *virtualFile) Write(buffer []byte) (int, error) {
-	return 0, nil
+	return len(buffer), nil
 }
 
 type virtualFileInfo struct {

sample/sample_driver.go

@@ -54,10 +54,8 @@ func (c *clientHandler) transferFile(write bool, append bool) {
 
 	// Try to seek on it
 	if c.ctxRest != 0 {
-		if err == nil {
-			if _, errSeek := file.Seek(c.ctxRest, 0); errSeek != nil {
-				err = errSeek
-			}
+		if _, errSeek := file.Seek(c.ctxRest, 0); errSeek != nil {
+			err = errSeek
 		}
 
 		// Whatever happens we should reset the seek position
@@ -85,8 +83,13 @@ func (c *clientHandler) transferFile(write bool, append bool) {
 				out = tr
 			}
 
-			if _, errCopy := io.Copy(out, in); errCopy != nil && errCopy != io.EOF {
+			if written, errCopy := io.Copy(out, in); errCopy != nil && errCopy != io.EOF {
 				err = errCopy
+			} else {
+				c.logger.Debug(
+					logKeyMsg, "Stream copy finished",
+					"writtenBytes", written,
+				)
 			}
 		}
 	}

server/handle_files.go

@@ -63,19 +63,37 @@ func (c *clientHandler) handlePASV() error {
 	portRange := c.server.settings.PassiveTransferPortRange
 
 	if portRange != nil {
-		for start := portRange.Start; start < portRange.End; start++ {
-			port := portRange.Start + rand.Intn(portRange.End-portRange.Start)
-			laddr, err2 := net.ResolveTCPAddr("tcp", "0.0.0.0:"+fmt.Sprintf("%d", port))
+		nbAttempts := portRange.End - portRange.Start
+
+		// Making sure we trying a reasonable amount of ports before giving up
+		if nbAttempts < 10 {
+			nbAttempts = 10
+		} else if nbAttempts > 1000 {
+			nbAttempts = 1000
+		}
+
+		for i := 0; i < nbAttempts; i++ {
+			port := portRange.Start + rand.Intn(portRange.End-portRange.Start+1)
+			laddr, err2 := net.ResolveTCPAddr("tcp", fmt.Sprintf("0.0.0.0:%d", port))
 
 			if err2 != nil {
-				continue
+				c.logger.Error(logKeyMsg, "Problem resolving local port", "port", port)
+				return err2
 			}
 
 			tcpListener, err = net.ListenTCP("tcp", laddr)
 			if err == nil {
 				break
 			}
 		}
+		if err != nil {
+			c.logger.Warn(
+				logKeyMsg, "Could not find any free port",
+				"nbAttempts", nbAttempts,
+				"portRangeStart", portRange.Start,
+				"portRAngeEnd", portRange.End,
+			)
+		}
 	} else {
 		tcpListener, err = net.ListenTCP("tcp", addr)
 	}