Commit `3b2889781b`

3b2889781b80ee8c080d98999b99303d50d8ea91

parent: 0ed36f2a40

Verified · cmc

cmc <hello@cleberg.net> · 2026-09-26 18:57 UTC

Harden exiftool lifecycle and startup checks

- CloseExiftool is final: later requests fail instead of starting a
  process that nothing stops. Fatal exits close it first.
- Each exiftool request times out after 30s and kills the process.
- Pass absolute paths so exiftool's argfile parsing cannot alter them.
- checkDirs rejects cache dirs such as photos/..cache.
- Publish the empty library before the watcher starts.

Ref #3
Ref #4

Layout: unified · split

cmd/gallery/main.go +15 −6

 	if err != nil {
 		return err
 	}
-	if rel, err := filepath.Rel(p, c); err == nil && (rel == "." || !strings.HasPrefix(rel, "..")) {
+	rel, err := filepath.Rel(p, c)
+	if err != nil {
+		return err
+	}
+	if rel != ".." && !strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
 		return fmt.Errorf("cache %s must not be inside photos %s", cache, photos)
 	}
 	return nil
 			log.Printf("rescan: %v", err)
 		}
 	}
+	// Serve an empty library while the first scan runs.
+	store.Set(&library.Library{})
+	// fatal stops exiftool first: it ignores EOF and would outlive the process.
+	fatal := func(err error) {
+		format.CloseExiftool()
+		log.Fatal(err)
+	}
 	// Watch before the first scan so files added during it are not missed.
 	if err := library.Watch(ctx, *photos, 500*time.Millisecond, onChange); err != nil {
 		log.Fatal(err)
 	}
-	// Serve an empty library while the first scan runs.
-	store.Set(&library.Library{})
 	go func() {
 		if err := rescan(); err != nil {
-			log.Fatal(err)
+			fatal(err)
 		}
 	}()
 	}
 	h, err := web.New(store, rend, opt)
 	if err != nil {
-		log.Fatal(err)
+		fatal(err)
 	}
 	srv := &http.Server{Addr: *addr, Handler: h, ReadHeaderTimeout: 10 * time.Second}
 	}()
 	log.Printf("listening on %s", *addr)
 	if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
-		log.Fatal(err)
+		fatal(err)
 	}
 	<-done
 	format.CloseExiftool()

cmd/gallery/main_test.go +1

	@@ -17,6 +17,7 @@ func TestCheckDirs(t *testing.T) {
17	{photos, false},	17	{photos, false},
18	{filepath.Join(photos, "cache"), false},	18	{filepath.Join(photos, "cache"), false},
19	{filepath.Join(photos, ".cache"), false},	19	{filepath.Join(photos, ".cache"), false},
		20	{filepath.Join(photos, "..cache"), false},
20	{filepath.Join(photos, "a", "..", "cache"), false},	21	{filepath.Join(photos, "a", "..", "cache"), false},
21	} {	22	} {
22	err := checkDirs(photos, tc.cache)	23	err := checkDirs(photos, tc.cache)

internal/format/exiftool.go +38 −9

 import (
 	"bufio"
 	"bytes"
+	"errors"
 	"fmt"
 	"io"
 	"os/exec"
 	"strings"
 	"sync"
 	"sync/atomic"
+	"time"
 )
+const exiftoolTimeout = 30 * time.Second
 // exiftool keeps one `exiftool -stay_open` process and sends it one request at a time.
 type exiftool struct {
-	mu     sync.Mutex
+	mu      sync.Mutex
-	cmd    *exec.Cmd
+	cmd     *exec.Cmd
-	stdin  io.WriteCloser
+	stdin   io.WriteCloser
-	stdout *bufio.Reader
+	stdout  *bufio.Reader
+	closed  bool
+	timeout time.Duration // per request; exiftoolTimeout when zero
 }
 var (
 	e.cmd = nil
 }
+// close stops the process for good; later requests fail instead of restarting it.
 func (e *exiftool) close() {
 	e.mu.Lock()
 	defer e.mu.Unlock()
+	e.closed = true
 	e.stop(true)
 }
-// CloseExiftool stops the shared exiftool process, if one is running.
+// CloseExiftool stops the shared exiftool process. Call it before exiting.
 func CloseExiftool() {
 	sharedExiftool.close()
 }
 // run sends one request, one argument per line, and returns its stdout.
-// The process is restarted on the next call if the pipe breaks.
+// A broken pipe or a request exceeding the timeout kills the process; the
+// next request starts a new one.
 func (e *exiftool) run(args ...string) ([]byte, error) {
 	for _, a := range args {
 		if strings.ContainsAny(a, "\r\n") {
 	}
 	e.mu.Lock()
 	defer e.mu.Unlock()
+	if e.closed {
+		return nil, errors.New("exiftool closed")
+	}
 	if e.cmd == nil {
 		if err := e.start(); err != nil {
 			return nil, err
 		}
 	}
-	if _, err := io.WriteString(e.stdin, strings.Join(args, "\n")+"\n-execute\n"); err != nil {
+	timeout := e.timeout
+	if timeout == 0 {
+		timeout = exiftoolTimeout
+	}
+	proc := e.cmd.Process
+	var timedOut atomic.Bool
+	timer := time.AfterFunc(timeout, func() {
+		timedOut.Store(true)
+		proc.Kill()
+	})
+	defer timer.Stop()
+	fail := func(err error) ([]byte, error) {
 		e.stop(false)
+		if timedOut.Load() {
+			return nil, fmt.Errorf("timed out after %s", timeout)
+		}
 		return nil, err
 	}
+	if _, err := io.WriteString(e.stdin, strings.Join(args, "\n")+"\n-execute\n"); err != nil {
+		return fail(err)
+	}
 	var out []byte
 	for {
 		line, err := e.stdout.ReadBytes('\n')
 		if err != nil {
-			e.stop(false)
+			return fail(err)
-			return nil, err
 		}
 		if string(bytes.TrimRight(line, "\r\n")) == "{ready}" {
 			return out, nil

internal/format/exiftool_test.go +26 −2

	@@ -5,6 +5,7 @@ import (
5	"os"	5	"os"
6	"syscall"	6	"syscall"
7	"testing"	7	"testing"
		8	"time"
8	)	9	)
9		10
10	func TestMain(m *testing.M) {	11	func TestMain(m *testing.M) {
	@@ -27,8 +28,31 @@ func TestExiftoolCloseEndsProcess(t *testing.T) {
27	if err := syscall.Kill(pid, 0); !errors.Is(err, syscall.ESRCH) {	28	if err := syscall.Kill(pid, 0); !errors.Is(err, syscall.ESRCH) {
28	t.Fatalf("exiftool pid %d still running after close (kill 0: %v)", pid, err)	29	t.Fatalf("exiftool pid %d still running after close (kill 0: %v)", pid, err)
29	}	30	}
30	if _, err := e.run("-ver"); err != nil {	31	}
31	t.Fatalf("run after close should restart: %v", err)	32
		33	func TestExiftoolRunAfterCloseFails(t *testing.T) {
		34	requireTools(t)
		35	var e exiftool
		36	e.close()
		37	before := exiftoolStarts.Load()
		38	if _, err := e.run("-ver"); err == nil {
		39	t.Fatal("run after close: want error")
		40	}
		41	if exiftoolStarts.Load() != before {
		42	t.Fatal("run after close started a process")
		43	}
		44	}
		45
		46	func TestExiftoolTimeoutKillsAndRecovers(t *testing.T) {
		47	requireTools(t)
		48	e := exiftool{timeout: time.Nanosecond}
		49	if _, err := e.run("-ver"); err == nil {
		50	t.Fatal("want timeout error")
		51	}
		52	e.timeout = 0
		53	out, err := e.run("-ver")
		54	if err != nil \|\| len(out) == 0 {
		55	t.Fatalf("run after timeout: %q, %v", out, err)
32	}	56	}
33	e.close()	57	e.close()
34	}	58	}

internal/format/magick.go +6 −2

 	"fmt"
 	"io"
 	"os/exec"
+	"path/filepath"
 	"strconv"
 	"strings"
 )
 func (m *Magick) Kind() Kind             { return KindImage }
 func (m *Magick) Metadata(path string) (Meta, error) {
-	if strings.HasPrefix(path, "-") {
+	// exiftool's argfile strips leading spaces and skips "#" lines, and a
-		path = "./" + path
+	// leading "-" would be read as an option; an absolute path avoids all three.
+	path, err := filepath.Abs(path)
+	if err != nil {
+		return Meta{}, err
 	}
 	out, err := sharedExiftool.run("-json", "-n",
 		"-ImageWidth", "-ImageHeight", "-Orientation",

internal/format/magick_test.go +14

	@@ -159,3 +159,17 @@ func TestResizeArgsLimitResources(t *testing.T) {
159	t.Fatalf("got %s\nwant %s", got, want)	159	t.Fatalf("got %s\nwant %s", got, want)
160	}	160	}
161	}	161	}
		162
		163	func TestMagickMetadataRelativePathArgfileSafe(t *testing.T) {
		164	requireTools(t)
		165	dir := t.TempDir()
		166	run(t, "magick", "-size", "30x20", "xc:gray", filepath.Join(dir, "#a.jpg"))
		167	t.Chdir(dir)
		168	m, err := NewMagick("jpeg", ".jpg").Metadata("#a.jpg")
		169	if err != nil {
		170	t.Fatal(err)
		171	}
		172	if m.Width != 30 \|\| m.Height != 20 {
		173	t.Fatalf("got %dx%d", m.Width, m.Height)
		174	}
		175	}

internal/render/render.go +3 −1

	@@ -86,7 +86,9 @@ func (r Renderer) generate(it library.Item, w int, dst string) error {
86	}	86	}
87		87
88	// pruneGrace keeps stale directories modified this recently, since a request	88	// pruneGrace keeps stale directories modified this recently, since a request
89	// holding an older library snapshot may still be writing into them.	89	// holding an older library snapshot may still be writing into them. This is
		90	// best-effort: a directory's mtime changes when entries are created or
		91	// renamed, not while a file is written, so a narrow window remains.
90	const pruneGrace = 5 * time.Minute	92	const pruneGrace = 5 * time.Minute
91		93
92	// Prune removes derivative directories for items no longer in lib or whose source changed.	94	// Prune removes derivative directories for items no longer in lib or whose source changed.

Commit 3b2889781b

Commit `3b2889781b`