Fix file redownloads, Ubuntu 20/Debian 11 compatibility (#359)

* fix new file contents getting appended to end of file

* add test-init helper to makefile

* fix hash stream functions

* use temp file for redownload

.gitignore

@@ -22,7 +22,7 @@ tmp/
 *.core
 *.gdb
 vgcore.*
-__debug_bin
+__debug_bin*
 
 # do not include binaries, but do include sources
 onedriver

Makefile

@@ -1,4 +1,4 @@
-.PHONY: all, test, srpm, rpm, dsc, changes, deb, clean, install, uninstall
+.PHONY: all, test, test-init, srpm, rpm, dsc, changes, deb, clean, install, uninstall
 
 # autocalculate software/package versions
 VERSION := $(shell grep Version onedriver.spec | sed 's/Version: *//g')
@@ -119,6 +119,13 @@ dmel.fa:
 	curl ftp://ftp.ensemblgenomes.org/pub/metazoa/release-42/fasta/drosophila_melanogaster/dna/Drosophila_melanogaster.BDGP6.22.dna.chromosome.X.fa.gz | zcat > $@
 
 
+# setup tests for the first time on a new computer
+test-init: onedriver
+	go install github.com/rakyll/gotest@latest
+	mkdir -p mount/
+	$< -a mount/	
+
+
 # For offline tests, the test binary is built online, then network access is
 # disabled and tests are run. sudo is required - otherwise we don't have
 # permission to deny network access to onedriver during the test.

fs/content_cache.go

@@ -2,6 +2,7 @@ package fs
 
 import (
 	"io"
+	"io/ioutil"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -27,15 +28,15 @@ func (l *LoopbackCache) contentPath(id string) string {
 	return filepath.Join(l.directory, id)
 }
 
-// GetContent reads a file's content from disk.
+// Get reads a file's content from disk.
 func (l *LoopbackCache) Get(id string) []byte {
-	content, _ := os.ReadFile(l.contentPath(id))
+	content, _ := ioutil.ReadFile(l.contentPath(id))
 	return content
 }
 
 // InsertContent writes file content to disk in a single bulk insert.
 func (l *LoopbackCache) Insert(id string, content []byte) error {
-	return os.WriteFile(l.contentPath(id), content, 0600)
+	return ioutil.WriteFile(l.contentPath(id), content, 0600)
 }
 
 // InsertStream inserts a stream of data
@@ -47,12 +48,13 @@ func (l *LoopbackCache) InsertStream(id string, reader io.Reader) (int64, error)
 	return io.Copy(fd, reader)
 }
 
-// DeleteContent deletes content from disk.
+// Delete closes the fd AND deletes content from disk.
 func (l *LoopbackCache) Delete(id string) error {
+	l.Close(id)
 	return os.Remove(l.contentPath(id))
 }
 
-// MoveContent moves content from one ID to another
+// Move moves content from one ID to another
 func (l *LoopbackCache) Move(oldID string, newID string) error {
 	return os.Rename(l.contentPath(oldID), l.contentPath(newID))
 }
@@ -75,7 +77,7 @@ func (l *LoopbackCache) HasContent(id string) bool {
 	return err == nil
 }
 
-// OpenContent returns a filehandle for subsequent access
+// Open returns a filehandle for subsequent access
 func (l *LoopbackCache) Open(id string) (*os.File, error) {
 	if fd, ok := l.fds.Load(id); ok {
 		// already opened, return existing fd
@@ -96,6 +98,7 @@ func (l *LoopbackCache) Open(id string) (*os.File, error) {
 	return fd, nil
 }
 
+// Close closes the currently open fd
 func (l *LoopbackCache) Close(id string) {
 	if fd, ok := l.fds.Load(id); ok {
 		file := fd.(*os.File)

fs/fs.go

@@ -1,6 +1,7 @@
 package fs
 
 import (
+	"io"
 	"math"
 	"os"
 	"path/filepath"
@@ -513,11 +514,25 @@ func (f *Filesystem) Open(cancel <-chan struct{}, in *fuse.OpenIn, out *fuse.Ope
 	ctx.Info().Msg(
 		"Not using cached item due to file hash mismatch, fetching content from API.",
 	)
-	size, err := graph.GetItemContentStream(id, f.auth, fd)
+
+	// write to tempfile first to ensure our download is good
+	tempID := "temp-" + id
+	temp, err := f.content.Open(tempID)
 	if err != nil {
+		ctx.Error().Err(err).Msg("Failed to create tempfile for download.")
+		return fuse.EIO
+	}
+	defer f.content.Delete(tempID)
+
+	// replace content only on a match
+	size, err := graph.GetItemContentStream(id, f.auth, temp)
+	if err != nil || !inode.VerifyChecksum(graph.QuickXORHashStream(temp)) {
 		ctx.Error().Err(err).Msg("Failed to fetch remote content.")
 		return fuse.EREMOTEIO
 	}
+	fd.Seek(0, 0)
+	fd.Truncate(0)
+	io.Copy(fd, temp)
 	inode.DriveItem.Size = size
 	return fuse.OK
 }
@@ -761,6 +776,7 @@ func (f *Filesystem) SetAttr(cancel <-chan struct{}, in *fuse.SetAttrIn, out *fu
 			Uint64("newSize", size).
 			Msg("")
 		fd, _ := f.content.Open(i.DriveItem.ID)
+		// the unix syscall does not update the seek position, so neither should we
 		fd.Truncate(int64(size))
 		i.DriveItem.Size = size
 		i.hasChanges = true

fs/graph/drive_item.go

@@ -122,8 +122,10 @@ func GetItemContent(id string, auth *Auth) ([]byte, uint64, error) {
 	return buf.Bytes(), uint64(n), err
 }
 
-// GetItemContentStream is the same as GetItemContent, but writes data to an output
-// reader
+// GetItemContentStream is the same as GetItemContent, but writes data to an
+// output reader. This function assumes a brand-new io.Writer is used, so
+// "output" must be truncated if there is content already in the io.Writer
+// prior to use.
 func GetItemContentStream(id string, auth *Auth, output io.Writer) (uint64, error) {
 	// determine the size of the item
 	item, err := GetItem(id, auth)

fs/graph/hashes.go

@@ -15,9 +15,11 @@ func SHA256Hash(data *[]byte) string {
 	return strings.ToUpper(fmt.Sprintf("%x", sha256.Sum256(*data)))
 }
 
-func SHA256HashStream(reader io.Reader) string {
+func SHA256HashStream(reader io.ReadSeeker) string {
+	reader.Seek(0, 0)
 	hash := sha256.New()
 	io.Copy(hash, reader)
+	reader.Seek(0, 0)
 	return strings.ToUpper(fmt.Sprintf("%x", hash.Sum(nil)))
 }
 
@@ -28,9 +30,11 @@ func SHA1Hash(data *[]byte) string {
 }
 
 // SHA1HashStream hashes the contents of a stream.
-func SHA1HashStream(reader io.Reader) string {
+func SHA1HashStream(reader io.ReadSeeker) string {
+	reader.Seek(0, 0)
 	hash := sha1.New()
 	io.Copy(hash, reader)
+	reader.Seek(0, 0)
 	return strings.ToUpper(fmt.Sprintf("%x", hash.Sum(nil)))
 }
 
@@ -43,9 +47,11 @@ func QuickXORHash(data *[]byte) string {
 }
 
 // QuickXORHashStream hashes a stream.
-func QuickXORHashStream(reader io.Reader) string {
+func QuickXORHashStream(reader io.ReadSeeker) string {
+	reader.Seek(0, 0)
 	hash := quickxorhash.New()
 	io.Copy(hash, reader)
+	reader.Seek(0, 0)
 	return base64.StdEncoding.EncodeToString(hash.Sum(nil))
 }
 
@@ -56,8 +62,6 @@ func (d *DriveItem) VerifyChecksum(checksum string) bool {
 	if len(checksum) == 0 || d.File == nil {
 		return false
 	}
-	// all checksums are converted to upper to avoid casing issues from whatever
-	// the API decides to return at this point in time.
 	return strings.EqualFold(d.File.Hashes.QuickXorHash, checksum)
 }
 

fs/graph/hashes_test.go

@@ -2,6 +2,7 @@ package graph
 
 import (
 	"bytes"
+	"io"
 	"os"
 	"testing"
 
@@ -67,3 +68,16 @@ func TestQuickXORHashReader(t *testing.T) {
 	actual := QuickXORHashStream(reader)
 	assert.Equal(t, expected, actual)
 }
+
+func TestHashSeekPosition(t *testing.T) {
+	tmp, err := os.CreateTemp("", "onedriverHashTest")
+	if err != nil {
+		t.Error(err)
+	}
+	content := []byte("some test content")
+	io.Copy(tmp, bytes.NewBuffer(content))
+
+	assert.Equal(t, QuickXORHash(&content), QuickXORHashStream(tmp))
+	assert.Equal(t, SHA1Hash(&content), SHA1HashStream(tmp))
+	assert.Equal(t, SHA256Hash(&content), SHA256HashStream(tmp))
+}