crypt: fix crypt writer getting stuck in a loop #902

This happened when the underlying reader returned io.ErrUnexpectedEOF.
The error handling for the call to io.ReadFull failed to take this
into account.

io.ErrUnexpectedEOF is reasonably common when SSL connections go wrong
when communicating with ACD, so it manifested itself as transfers from
non-encrypted ACD to encrypted ACD getting stuck.

crypt/cipher.go

@@ -449,14 +449,13 @@ func (fh *encrypter) Read(p []byte) (n int, err error) {
 		// FIXME should overlap the reads with a go-routine and 2 buffers?
 		readBuf := fh.readBuf[:blockDataSize]
 		n, err = io.ReadFull(fh.in, readBuf)
-		if err == io.EOF {
+		if n == 0 {
-			// ReadFull only returns n=0 and EOF
+			// err can't be nil since:
-			return fh.finish(io.EOF)
+			// n == len(buf) if and only if err == nil.
-		} else if err == io.ErrUnexpectedEOF {
-			// Next read will return EOF
-		} else if err != nil {
 			return fh.finish(err)
 		}
+		// possibly err != nil here, but we will process the
+		// data and the next call to ReadFull will return 0, err
 		// Write nonce to start of block
 		copy(fh.buf, fh.nonce[:])
 		// Encrypt the block using the nonce
@@ -563,26 +562,27 @@ func (fh *decrypter) fillBuffer() (err error) {
 	// FIXME should overlap the reads with a go-routine and 2 buffers?
 	readBuf := fh.readBuf
 	n, err := io.ReadFull(fh.rc, readBuf)
-	if err == io.EOF {
+	if n == 0 {
-		// ReadFull only returns n=0 and EOF
+		// err can't be nil since:
-		return io.EOF
+		// n == len(buf) if and only if err == nil.
-	} else if err == io.ErrUnexpectedEOF {
-		// Next read will return EOF
-	} else if err != nil {
 		return err
 	}
+	// possibly err != nil here, but we will process the data and
+	// the next call to ReadFull will return 0, err
+
 	// Check header + 1 byte exists
 	if n <= blockHeaderSize {
+		if err != nil {
+			return err // return pending error as it is likely more accurate
+		}
 		return ErrorEncryptedFileBadHeader
 	}
 	// Decrypt the block using the nonce
 	block := fh.buf
 	_, ok := secretbox.Open(block[:0], readBuf[:n], fh.nonce.pointer(), &fh.c.dataKey)
 	if !ok {
-		// if block wouldn't decode and got unexpected EOF
-		// then return that as it is probably a better error
 		if err != nil {
-			return err
+			return err // return pending error as it is likely more accurate
 		}
 		return ErrorEncryptedBadBlock
 	}

crypt/cipher_test.go

@@ -788,6 +788,21 @@ func TestNewEncrypter(t *testing.T) {
 
 }
 
+// Test the stream returning 0, io.ErrUnexpectedEOF - this used to
+// cause a fatal loop
+func TestNewEncrypterErrUnexpectedEOF(t *testing.T) {
+	c, err := newCipher(NameEncryptionStandard, "", "")
+	assert.NoError(t, err)
+
+	in := &errorReader{io.ErrUnexpectedEOF}
+	fh, err := c.newEncrypter(in)
+	assert.NoError(t, err)
+
+	n, err := io.CopyN(ioutil.Discard, fh, 1E6)
+	assert.Equal(t, io.ErrUnexpectedEOF, err)
+	assert.Equal(t, int64(32), n)
+}
+
 type errorReader struct {
 	err error
 }
@@ -854,6 +869,23 @@ func TestNewDecrypter(t *testing.T) {
 	}
 }
 
+// Test the stream returning 0, io.ErrUnexpectedEOF
+func TestNewDecrypterErrUnexpectedEOF(t *testing.T) {
+	c, err := newCipher(NameEncryptionStandard, "", "")
+	assert.NoError(t, err)
+
+	in2 := &errorReader{io.ErrUnexpectedEOF}
+	in1 := bytes.NewBuffer(file16)
+	in := ioutil.NopCloser(io.MultiReader(in1, in2))
+
+	fh, err := c.newDecrypter(in)
+	assert.NoError(t, err)
+
+	n, err := io.CopyN(ioutil.Discard, fh, 1E6)
+	assert.Equal(t, io.ErrUnexpectedEOF, err)
+	assert.Equal(t, int64(16), n)
+}
+
 func TestNewDecrypterSeek(t *testing.T) {
 	c, err := newCipher(NameEncryptionStandard, "", "")
 	assert.NoError(t, err)
@@ -936,8 +968,6 @@ func TestDecrypterRead(t *testing.T) {
 		case i == fileHeaderSize:
 			// This would normally produce an error *except* on the first block
 			expectedErr = nil
-		case i <= fileHeaderSize+blockHeaderSize:
-			expectedErr = ErrorEncryptedFileBadHeader
 		default:
 			expectedErr = io.ErrUnexpectedEOF
 		}