Merge PR #71: Dealing with file access errors

Fixes issues #66, #69, #70

server/handle_files.go

@@ -10,69 +10,89 @@ import (
 )
 
 func (c *clientHandler) handleSTOR() {
-	c.handleStoreAndAppend(false)
+	c.transferFile(true, false)
 }
 
 func (c *clientHandler) handleAPPE() {
-	c.handleStoreAndAppend(true)
+	c.transferFile(true, true)
 }
 
-// Handles both the "STOR" and "APPE" commands
-func (c *clientHandler) handleStoreAndAppend(append bool) {
-	file, err := c.openFile(c.absPath(c.param), append)
+func (c *clientHandler) handleRETR() {
+	c.transferFile(false, false)
+}
 
-	if err != nil {
-		c.writeMessage(550, "Could not open file: "+err.Error())
-		return
-	}
+// File transfer, read or write, seek or not, is basically the same.
+// To make sure we don't miss any step, we execute everything in order
+func (c *clientHandler) transferFile(write bool, append bool) {
 
-	if tr, err := c.TransferOpen(); err == nil {
-		defer c.TransferClose()
-		if _, err := c.storeOrAppend(tr, file); err != nil && err != io.EOF {
-			c.writeMessage(550, err.Error())
+	var file FileStream
+	var err error
+
+	// We try to open the file
+	{
+		var fileFlag int
+		if write {
+			fileFlag = os.O_WRONLY
+			if append {
+				fileFlag |= os.O_APPEND
+			}
+		} else {
+			fileFlag = os.O_RDONLY
 		}
-	} else {
-		c.writeMessage(550, "Could not open transfer: "+err.Error())
-	}
-}
 
-func (c *clientHandler) openFile(path string, append bool) (FileStream, error) {
-	flag := os.O_WRONLY
-	if append {
-		flag |= os.O_APPEND
+		// If this fail, can stop right here
+		if file, err = c.driver.OpenFile(c, c.absPath(c.param), fileFlag); err != nil {
+			c.writeMessage(550, "Could not access file: "+err.Error())
+			return
+		}
 	}
 
-	return c.driver.OpenFile(c, path, flag)
-}
+	// Try to seek on it
+	if c.ctxRest != 0 {
+		if err == nil {
+			if _, errSeek := file.Seek(c.ctxRest, 0); errSeek != nil {
+				err = errSeek
+			}
+		}
 
-func (c *clientHandler) handleRETR() {
+		// Whatever happens we should reset the seek position
+		c.ctxRest = 0
+	}
 
-	path := c.absPath(c.param)
+	// Start the transfer
+	if err == nil {
+		var tr net.Conn
+		if tr, err = c.TransferOpen(); err == nil {
+			defer c.TransferClose()
+
+			// Copy the data
+			var in io.Reader
+			var out io.Writer
+
+			if write { // ... from the connection to the file
+				in = tr
+				out = file
+			} else { // ... from the file to the connection
+				in = file
+				out = tr
+			}
 
-	if tr, err := c.TransferOpen(); err == nil {
-		defer c.TransferClose()
-		if _, err := c.download(tr, path); err != nil && err != io.EOF {
-			c.writeMessage(550, err.Error())
+			if _, errCopy := io.Copy(out, in); errCopy != nil && errCopy != io.EOF {
+				err = errCopy
+			}
 		}
-	} else {
-		c.writeMessage(550, err.Error())
 	}
-}
-
-func (c *clientHandler) download(conn net.Conn, name string) (int64, error) {
-	file, err := c.driver.OpenFile(c, name, os.O_RDONLY)
 
-	if err != nil {
-		return 0, err
+	// *ALWAYS* close the file but only save the error if there wasn't one before
+	// Note: We could discard the error in read mode
+	if errClose := file.Close(); errClose != nil && err == nil {
+		err = errClose
 	}
 
-	if c.ctxRest != 0 {
-		file.Seek(c.ctxRest, 0)
-		c.ctxRest = 0
+	if err != nil {
+		c.writeMessage(550, "Could not transfer file: "+err.Error())
+		return
 	}
-
-	defer file.Close()
-	return io.Copy(conn, file)
 }
 
 func (c *clientHandler) handleCHMOD(params string) {
@@ -94,16 +114,6 @@ func (c *clientHandler) handleCHMOD(params string) {
 	c.writeMessage(200, "SITE CHMOD command successful")
 }
 
-func (c *clientHandler) storeOrAppend(conn net.Conn, file FileStream) (int64, error) {
-	if c.ctxRest != 0 {
-		file.Seek(c.ctxRest, 0)
-		c.ctxRest = 0
-	}
-
-	defer file.Close()
-	return io.Copy(file, conn)
-}
-
 func (c *clientHandler) handleDELE() {
 	path := c.absPath(c.param)
 	if err := c.driver.DeleteFile(c, path); err == nil {

tests/test_driver.go

@@ -49,11 +49,13 @@ type ServerDriver struct {
 	TLS   bool
 
 	Settings *server.Settings // Settings
+	server.FileStream
 }
 
 // ClientDriver defines a minimal serverftp client driver
 type ClientDriver struct {
 	baseDir string
+	server.FileStream
 }
 
 // NewClientDriver creates a client driver
@@ -73,7 +75,11 @@ func (driver *ServerDriver) WelcomeUser(cc server.ClientContext) (string, error)
 // AuthUser with authenticate users
 func (driver *ServerDriver) AuthUser(cc server.ClientContext, user, pass string) (server.ClientHandlingDriver, error) {
 	if user == "test" && pass == "test" {
-		return NewClientDriver(), nil
+		clientdriver := NewClientDriver()
+		if driver.FileStream != nil {
+			clientdriver.FileStream = driver.FileStream
+		}
+		return clientdriver, nil
 	}
 	return nil, errors.New("bad username or password")
 }
@@ -130,6 +136,10 @@ func (driver *ClientDriver) OpenFile(cc server.ClientContext, path string, flag
 		}
 	}
 
+	if driver.FileStream != nil {
+		return driver.FileStream, nil
+	}
+
 	return os.OpenFile(path, flag, 0666)
 }
 

tests/transfer_test.go

@@ -3,6 +3,7 @@ package tests
 import (
 	"crypto/sha256"
 	"encoding/hex"
+	"errors"
 	"io"
 	"io/ioutil"
 	"math/rand"
@@ -166,3 +167,44 @@ func TestFailedTransfer(t *testing.T) {
 		t.Fatal("This upload should have succeeded", err)
 	}
 }
+
+func TestFailedFileClose(t *testing.T) {
+	driver := &ServerDriver{
+		Debug:      true,
+		FileStream: &failingCloser{},
+	}
+
+	s := NewTestServerWithDriver(driver)
+	defer s.Stop()
+
+	conf := goftp.Config{
+		User:     "test",
+		Password: "test",
+	}
+
+	var err error
+	var c *goftp.Client
+
+	if c, err = goftp.DialConfig(conf, s.Addr()); err != nil {
+		t.Fatal("Couldn't connect", err)
+	}
+	defer c.Close()
+
+	file := createTemporaryFile(t, 1*1024)
+	err = c.Store("file.bin", file)
+	if err == nil {
+		t.Fatal("this upload should not succeed", err)
+	}
+	if !strings.Contains(err.Error(), errFailingCloser.Error()) {
+		t.Errorf("got %s as the error message, want it to contain %s", err, errFailingCloser)
+	}
+}
+
+type failingCloser struct {
+	os.File
+	fail bool
+}
+
+var errFailingCloser = errors.New("the hard disk crashed")
+
+func (f *failingCloser) Close() error { return errFailingCloser }