Author SHA1 Message Date
clawbot 722675f153 Escape the database path in the SQLite connection string (closes #55)
check / check (push) Waiting to run
openDB put the path into the connection string unescaped, so a ? or #
in it ended the file name and a % started an escape: scan could
silently fill a database under a shortened name. The path now goes
through net/url as a file: URI. An absolute path gets an empty host and
a relative path none, because SQLite reads what follows file:// up to
the next slash as a host name. The path is not cleaned, so it stays
exactly what the operator gave.

A test runs scan, report and trees against such a file name given as an
absolute path, as one starting with //, and as a relative path, and
checks that only that file and its lock file exist afterwards.

Model: opus-5-5
2026-10-04 15:13:19 +02:00
5 changed files with 109 additions and 56 deletions
+3 -1
View File
@@ -283,7 +283,9 @@ All three subcommands operate on a single SQLite database file:
- Location: the value of the `SFDUPES_DATABASE` environment variable
when set and non-empty, otherwise `/var/lib/sfdupes/db.sqlite`.
There is no command-line flag.
There is no command-line flag. The path names the file exactly,
whatever characters it holds (`?`, `#` and `%` included); a
relative path is relative to the working directory.
- `scan` creates the database (and its parent directory) on first
use. `report` and `trees` require an existing database; a missing
database file is a fatal error (exit 1) telling the user to run
+2 -2
View File
@@ -29,8 +29,8 @@
# Completed Steps
- a test fails when either `hashWorker` cancellation check in `scan.go`
is removed (2026-10-04, https://git.eeqj.de/sneak/sfdupes/issues/83)
- a database path holding `?`, `#` or `%` opens exactly the file it names
(2026-10-04, https://git.eeqj.de/sneak/sfdupes/issues/55)
- `scan` rejects `--workers` below 1 as a usage error instead of
running single-threaded (2026-10-04,
+22 -52
View File
@@ -729,77 +729,47 @@ func TestFeedHashJobsClosesJobsWhenCancelled(t *testing.T) {
// TestHashWorkerDropsQueuedRuns checks that a cancelled hash worker
// keeps reading jobs and drops the runs rather than reading files
// nobody wants the hashes of — while still letting the range run out
// so the pool tears down. The hash function records that it was
// called, so a worker that hashed the queued run anyway is caught
// every time.
// so the pool tears down. The queued run names a file that does not
// exist, so a worker that hashed it anyway would produce a result.
//
// hashWorker's other cancellation exit, abandoning the send of a
// result, is reachable from the scan: stop cancels the pool before it
// drains results, so a worker waiting on that send can leave through
// it. The tests that stop a scan mid-hash, among them
// TestScanHashWriteFailureUnwindsPool, reach it in some runs only,
// depending on timing, and no test fails without it, since stop's
// drain frees a waiting worker anyway. This test, for its part, catches
// a removed drop check in some runs only: a worker that hashes the run
// anyway then picks at random between sending the result and leaving.
func TestHashWorkerDropsQueuedRuns(t *testing.T) {
t.Parallel()
done := make(chan struct{})
jobs := make(chan []fileRec, 1)
results := make(chan hashResult)
results := make(chan hashResult, 1)
jobs <- []fileRec{{path: filepath.Join(t.TempDir(), "missing"), size: 1}}
run := []fileRec{{path: filepath.Join(t.TempDir(), "missing"), size: 1}}
jobs <- run
close(jobs)
var hashed atomic.Bool
hash := func(path string, size int64) (string, string, string, error) {
hashed.Store(true)
return hashSignature(path, size)
}
go func() {
defer close(done)
hashWorker(cancelledContext(t), jobs, results, hash)
hashWorker(cancelledContext(t), jobs, results, hashSignature)
}()
awaitReturn(t, done, "hashWorker")
if hashed.Load() {
t.Error("cancelled hash worker hashed the queued run, want it dropped")
select {
case r := <-results:
t.Errorf("cancelled hash worker produced %+v, want the run dropped",
r)
default:
}
}
// TestHashWorkerAbandonsBlockedSend checks that a hash worker with a
// result to deliver and nobody to deliver it to leaves once the scan
// is cancelled, instead of holding the pool open. The scan tests do
// not catch this: stop drains results, which frees a parked worker
// anyway.
func TestHashWorkerAbandonsBlockedSend(t *testing.T) {
t.Parallel()
ctx, cancel := context.WithCancel(t.Context())
defer cancel()
done := make(chan struct{})
jobs := make(chan []fileRec, 1)
// Unbuffered and unread, with jobs left open: the worker's only way
// out is the cancellation case beside its send.
results := make(chan hashResult)
jobs <- []fileRec{{path: filepath.Join(t.TempDir(), "missing"), size: 1}}
// The scan is cancelled while the worker hashes, so the worker has
// already passed the check that drops queued runs.
hash := func(path string, size int64) (string, string, string, error) {
cancel()
return hashSignature(path, size)
}
go func() {
defer close(done)
hashWorker(ctx, jobs, results, hash)
}()
awaitReturn(t, done, "hashWorker")
}
// TestHashPhaseCancelledReturnsContextError checks the result loop's
// own exit: with the pool cancelled, no result will ever arrive, and
// the loop must leave through the cancellation rather than wait for a
+14 -1
View File
@@ -6,6 +6,7 @@ import (
"errors"
"fmt"
"io/fs"
"net/url"
"os"
"path/filepath"
"slices"
@@ -107,7 +108,19 @@ const reportParams = "mode=ro" +
// openDB opens the SQLite database at path with the connection
// parameters params. It does not create or verify the schema.
func openDB(path, params string) (*sql.DB, error) {
db, err := sql.Open("sqlite", "file:"+path+"?"+params)
// The path is escaped into a file: URI, so ?, # and % in it stay
// part of the file name. SQLite reads what follows file:// up to
// the next / as a host name, so an absolute path goes after an
// empty host (file:///abs) and a relative path goes without one
// (file:rel).
uri := url.URL{
Scheme: "file",
OmitHost: !filepath.IsAbs(path),
Path: path,
RawQuery: params,
}
db, err := sql.Open("sqlite", uri.String())
if err != nil {
return nil, fmt.Errorf("open database %s: %w", path, err)
}
+68
View File
@@ -8,6 +8,7 @@ import (
"io/fs"
"os"
"path/filepath"
"slices"
"strconv"
"strings"
"testing"
@@ -639,6 +640,73 @@ func TestRunReportsNeedOnlyReadAccess(t *testing.T) {
}
}
// assertRunsUseDatabase runs scan, then report and trees, against the
// database that SFDUPES_DATABASE names, the file name in dir. It fails
// unless the reports find the duplicate pair the scan recorded and dir
// then holds only that file and its lock file: nothing was created
// under a shortened name.
func assertRunsUseDatabase(t *testing.T, dir, name string) {
t.Helper()
dupes := scanFixture(t)
want := "first\tdupe\tsize\n" + dupes[0] + "\t" + dupes[1] + "\t300\n"
if got := runStdout(t, cmdReport); got != want {
t.Errorf("report stdout = %q, want %q", got, want)
}
want = "first\tdupe\tfiles\tsize\n" +
filepath.Dir(dupes[0]) + "\t" + filepath.Dir(dupes[1]) + "\t1\t300\n"
if got := runStdout(t, cmdTrees); got != want {
t.Errorf("trees stdout = %q, want %q", got, want)
}
entries, err := os.ReadDir(dir)
if err != nil {
t.Fatal(err)
}
got := make([]string, 0, len(entries))
for _, e := range entries {
got = append(got, e.Name())
}
if wantFiles := []string{name, name + ".lock"}; !slices.Equal(got, wantFiles) {
t.Errorf("%s holds %q, want %q", dir, got, wantFiles)
}
}
func TestRunDatabasePathUsedAsGiven(t *testing.T) {
// README §Database: the path names the database file exactly. In
// SQLite's connection string an unescaped ? or # would end the file
// name and % would start an escape, and a path starting with //
// could be read as a host name. %25 is a valid escape, so unescaped
// this name opens a file named a without any error.
const name = "a?b#c%25d e.sqlite"
t.Run("absolute", func(t *testing.T) {
dir := t.TempDir()
t.Setenv(databaseEnv, filepath.Join(dir, name))
assertRunsUseDatabase(t, dir, name)
})
t.Run("leading double slash", func(t *testing.T) {
dir := t.TempDir()
t.Setenv(databaseEnv, "/"+filepath.Join(dir, name))
assertRunsUseDatabase(t, dir, name)
})
t.Run("relative", func(t *testing.T) {
dir := t.TempDir()
t.Chdir(dir)
t.Setenv(databaseEnv, name)
assertRunsUseDatabase(t, dir, name)
})
}
// holdScanLock takes the lock on the database at path, as a running
// scan does, and holds it until the test ends. It fails the test when
// the lock is already held.