From 2218ecd0492e7ba300a407c5c052e22c570d765a Mon Sep 17 00:00:00 2001
From: Alexander Neumann <alexander@bumpern.de>
Date: Tue, 1 May 2018 23:05:50 +0200
Subject: [PATCH] archiver: Use lstat before open/fstat

The previous code tried to be as efficient as possible and only do a
single open() on an item to save, and then fstat() on the fd to find out
what the item is (file, dir, other). For normal files, it would then
start reading the data without opening the file again, so it could not
be exchanged for e.g. a symlink.

This behavior starts the watchdog on my machine when /dev is saved
with restic, and after a few seconds, the machine reboots.

This commit reverts the behavior to the strategy the old archiver code
used: run lstat(), then decide what to do. For normal files, open the
file and then run fstat() on the fd to verify it's still a normal file,
then start reading the data.

The downside is that for normal files we now do two stat() calls
(lstat+fstat) instead of only one. On the upside, this does not start
the watchdog. :)
---
 internal/archiver/archiver.go | 76 ++++++++++++++++++-----------------
 1 file changed, 40 insertions(+), 36 deletions(-)

diff --git a/internal/archiver/archiver.go b/internal/archiver/archiver.go
index 9970e45b3..03e916e7e 100644
--- a/internal/archiver/archiver.go
+++ b/internal/archiver/archiver.go
@@ -7,7 +7,6 @@ import (
 	"path"
 	"runtime"
 	"sort"
-	"syscall"
 	"time"
 
 	"github.com/restic/restic/internal/debug"
@@ -331,36 +330,19 @@ func (arch *Archiver) Save(ctx context.Context, snPath, target string, previous
 
 	fn.absTarget = abstarget
 
-	var fi os.FileInfo
-	var errFI error
-
-	file, errOpen := arch.FS.OpenFile(target, fs.O_RDONLY|fs.O_NOFOLLOW|fs.O_NONBLOCK, 0)
-	if errOpen == nil {
-		fi, errFI = file.Stat()
-	}
-
+	fi, err := arch.FS.Lstat(target)
 	if !arch.Select(abstarget, fi) {
 		debug.Log("%v is excluded", target)
-		if file != nil {
-			_ = file.Close()
-		}
 		return FutureNode{}, true, nil
 	}
 
-	if errOpen != nil {
-		debug.Log("  open error %#v", errOpen)
-		// test if the open failed because target is a symbolic link or a socket
-		if e, ok := errOpen.(*os.PathError); ok && (e.Err == syscall.ELOOP || e.Err == syscall.ENXIO) {
-			// in this case, redo the stat and carry on
-			fi, errFI = arch.FS.Lstat(target)
-		} else {
-			return FutureNode{}, false, errors.Wrap(errOpen, "OpenFile")
+	if err != nil {
+		debug.Log("lstat() for %v returned error: %v", target, err)
+		err = arch.error(abstarget, fi, err)
+		if err != nil {
+			return FutureNode{}, false, errors.Wrap(err, "Lstat")
 		}
-	}
-
-	if errFI != nil {
-		_ = file.Close()
-		return FutureNode{}, false, errors.Wrap(errFI, "Stat")
+		return FutureNode{}, true, nil
 	}
 
 	switch {
@@ -368,6 +350,39 @@ func (arch *Archiver) Save(ctx context.Context, snPath, target string, previous
 		debug.Log("  %v regular file", target)
 		start := time.Now()
 
+		// reopen file and do an fstat() on the open file to check it is still
+		// a file (and has not been exchanged for e.g. a symlink)
+		file, err := arch.FS.OpenFile(target, fs.O_RDONLY|fs.O_NOFOLLOW|fs.O_NONBLOCK, 0)
+		if err != nil {
+			debug.Log("Openfile() for %v returned error: %v", target, err)
+			err = arch.error(abstarget, fi, err)
+			if err != nil {
+				return FutureNode{}, false, errors.Wrap(err, "Lstat")
+			}
+			return FutureNode{}, true, nil
+		}
+
+		fi, err = file.Stat()
+		if err != nil {
+			debug.Log("stat() on opened file %v returned error: %v", target, err)
+			_ = file.Close()
+			err = arch.error(abstarget, fi, err)
+			if err != nil {
+				return FutureNode{}, false, errors.Wrap(err, "Lstat")
+			}
+			return FutureNode{}, true, nil
+		}
+
+		// make sure it's still a file
+		if !fs.IsRegularFile(fi) {
+			err = errors.Errorf("file %v changed type, refusing to archive")
+			err = arch.error(abstarget, fi, err)
+			if err != nil {
+				return FutureNode{}, false, err
+			}
+			return FutureNode{}, true, nil
+		}
+
 		// use previous node if the file hasn't changed
 		if previous != nil && !fileChanged(fi, previous) {
 			debug.Log("%v hasn't changed, returning old node", target)
@@ -386,8 +401,6 @@ func (arch *Archiver) Save(ctx context.Context, snPath, target string, previous
 			arch.CompleteItem(snPath, previous, node, stats, time.Since(start))
 		})
 
-		file = nil
-
 	case fi.IsDir():
 		debug.Log("  %v dir", target)
 
@@ -400,7 +413,6 @@ func (arch *Archiver) Save(ctx context.Context, snPath, target string, previous
 		if err == nil {
 			arch.CompleteItem(snItem, previous, fn.node, fn.stats, time.Since(start))
 		} else {
-			_ = file.Close()
 			return FutureNode{}, false, err
 		}
 
@@ -413,18 +425,10 @@ func (arch *Archiver) Save(ctx context.Context, snPath, target string, previous
 
 		fn.node, err = arch.nodeFromFileInfo(target, fi)
 		if err != nil {
-			_ = file.Close()
 			return FutureNode{}, false, err
 		}
 	}
 
-	if file != nil {
-		err = file.Close()
-		if err != nil {
-			return fn, false, errors.Wrap(err, "Close")
-		}
-	}
-
 	debug.Log("return after %.3f", time.Since(start).Seconds())
 
 	return fn, false, nil