fix(ingestion): close the PR #156 review findings on raw/ reads
- Reverting the ingest call sites to extract.Text(path) stayed green; a test now fails if anything outside internal/extract calls extract.Text( (only TextIn, through an os.Root, may be used). - The watcher symlink test passed for a watcher that ingested nothing, or stopped at the first symlink (SkipAll: one synced link halting all ingestion). It now plants a real note after the symlink and asserts exactly one ingest, and the symlinked-raw refusal has its own test. - A FIFO swapped in between Lstat and Open hung open(2) forever. extract.OpenRegular opens non-blocking and checks the type on the handle; readRawNote, TextIn and the watcher's archive copy use it. - The symlinked-root test covers pending, promote, reject, resolve and ReadIngestFile, and is named for exactly those. - The hard-link residual is documented where it is accepted. Out of scope, filed: #157 (symlinks elsewhere in the Syncthing-shared brain dir reach search and other readers), #158 (writes under raw/ follow symlinks). Refs #153 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,43 @@
|
||||
package extract
|
||||
|
||||
import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
// PR #156 review: putting extract.Text(path) back at the ingest call sites
|
||||
// stayed green, because only TextIn itself was tested. Reading a caller-chosen
|
||||
// path by name reopens it after the containment check (hyperguild#153 item 3),
|
||||
// so outside this package only TextIn may be used.
|
||||
func TestNothingOutsideExtractCallsTextByPath(t *testing.T) {
|
||||
root := filepath.Join("..", "..")
|
||||
var offenders []string
|
||||
err := filepath.WalkDir(root, func(p string, d os.DirEntry, err error) error {
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if d.IsDir() && (d.Name() == "vendor" || d.Name() == ".git") {
|
||||
return filepath.SkipDir
|
||||
}
|
||||
if d.IsDir() || !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") ||
|
||||
strings.Contains(filepath.ToSlash(p), "internal/extract/") {
|
||||
return nil
|
||||
}
|
||||
b, err := os.ReadFile(p)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if strings.Contains(string(b), "extract.Text(") {
|
||||
offenders = append(offenders, p)
|
||||
}
|
||||
return nil
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatal(err)
|
||||
}
|
||||
if len(offenders) > 0 {
|
||||
t.Fatalf("extract.Text( called outside internal/extract (use TextIn through an os.Root): %v", offenders)
|
||||
}
|
||||
}
|
||||
@@ -6,6 +6,7 @@ import (
|
||||
"io"
|
||||
"os"
|
||||
"strings"
|
||||
"syscall"
|
||||
)
|
||||
|
||||
// Text reads the file at path and returns its plain-text content.
|
||||
@@ -56,7 +57,7 @@ func TextIn(r *os.Root, rel string) (string, error) {
|
||||
if !fi.Mode().IsRegular() {
|
||||
return "", fmt.Errorf("%s: not a regular file", rel)
|
||||
}
|
||||
f, err := r.Open(rel)
|
||||
f, _, err := OpenRegular(r, rel)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("open %s: %w", rel, err)
|
||||
}
|
||||
@@ -87,3 +88,25 @@ func TextIn(r *os.Root, rel string) (string, error) {
|
||||
return "", fmt.Errorf("unsupported file extension: %s", ext)
|
||||
}
|
||||
}
|
||||
|
||||
// OpenRegular opens rel inside r and returns it only if the OPENED handle is
|
||||
// a regular file. The open is non-blocking, so a FIFO swapped in after an
|
||||
// Lstat (PR #156 review: it hung open(2) forever) is refused promptly rather
|
||||
// than stalling the reader. Callers keep their Lstat check, which refuses
|
||||
// symlinks outright; this closes the race between that check and the open.
|
||||
func OpenRegular(r *os.Root, rel string) (*os.File, os.FileInfo, error) {
|
||||
f, err := r.OpenFile(rel, os.O_RDONLY|syscall.O_NONBLOCK, 0)
|
||||
if err != nil {
|
||||
return nil, nil, err
|
||||
}
|
||||
fi, err := f.Stat()
|
||||
if err != nil {
|
||||
_ = f.Close()
|
||||
return nil, nil, err
|
||||
}
|
||||
if !fi.Mode().IsRegular() {
|
||||
_ = f.Close()
|
||||
return nil, nil, fmt.Errorf("%s: not a regular file", rel)
|
||||
}
|
||||
return f, fi, nil
|
||||
}
|
||||
|
||||
@@ -3,7 +3,9 @@ package extract
|
||||
import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"syscall"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
@@ -34,3 +36,29 @@ func TestTextInReadsInsideTheRootAndRefusesEscapes(t *testing.T) {
|
||||
_, err = TextIn(r, "../"+filepath.Base(outside))
|
||||
assert.Error(t, err)
|
||||
}
|
||||
|
||||
// PR #156 review: a regular file swapped for a FIFO between Lstat and Open
|
||||
// blocked open(2) forever. OpenRegular opens non-blocking and checks the type
|
||||
// on the opened handle, so a FIFO is refused promptly instead of hanging.
|
||||
func TestOpenRegularRefusesAFIFOWithoutBlocking(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
require.NoError(t, syscall.Mkfifo(filepath.Join(dir, "pipe.md"), 0o644))
|
||||
r, err := os.OpenRoot(dir)
|
||||
require.NoError(t, err)
|
||||
defer func() { _ = r.Close() }()
|
||||
|
||||
done := make(chan error, 1)
|
||||
go func() {
|
||||
f, _, err := OpenRegular(r, "pipe.md")
|
||||
if f != nil {
|
||||
_ = f.Close()
|
||||
}
|
||||
done <- err
|
||||
}()
|
||||
select {
|
||||
case err := <-done:
|
||||
assert.Error(t, err)
|
||||
case <-time.After(2 * time.Second):
|
||||
t.Fatal("OpenRegular blocked on a FIFO")
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user