tests run on OpenBSD; a tool's error message survives one large write; lock pids beyond 32 bits name no process

5 files changed, 36 insertions, 12 deletions

diff --git a/cmd/krino/hostile_test.go b/cmd/krino/hostile_test.go
index 16a5b19..ad10ba8 100644
--- a/cmd/krino/hostile_test.go
+++ b/cmd/krino/hostile_test.go
@@ -50,7 +50,9 @@ func TestHostileNamesNeverReachTheTerminal(t *testing.T) {
 	if err := os.Mkdir(bin, 0o755); err != nil {
 		t.Fatal(err)
 	}
-	tool := "#!/bin/sh\nprintf 'bad \\033[2Jpdf\\n' >&2\nexit 1\n"
+	// printf by its own PATH: PATH below holds only bin, and printf is not a
+	// shell builtin everywhere (OpenBSD).
+	tool := "#!/bin/sh\nPATH=/usr/bin:/bin\nprintf 'bad \\033[2Jpdf\\n' >&2\nexit 1\n"
 	if err := os.WriteFile(filepath.Join(bin, "pdftotext"), []byte(tool), 0o755); err != nil {
 		t.Fatal(err)
 	}
diff --git a/internal/extract/tools.go b/internal/extract/tools.go
index ca6a9b0..f815123 100644
--- a/internal/extract/tools.go
+++ b/internal/extract/tools.go
@@ -65,17 +65,19 @@ type boundedWriter struct {
 }
 
 func (w *boundedWriter) Write(p []byte) (int, error) {
+	if room := w.limit - w.total; room > 0 {
+		// The part of p that still fits is kept even when p overflows, so a
+		// tool that writes its whole error message in one large write still
+		// has that message's start to show.
+		w.buf.Write(p[:min(int64(len(p)), room)])
+	}
 	w.total += int64(len(p))
-	if w.total > w.limit {
-		if !w.overflowed {
-			w.overflowed = true
-			if w.onOverflow != nil {
-				w.onOverflow()
-			}
+	if w.total > w.limit && !w.overflowed {
+		w.overflowed = true
+		if w.onOverflow != nil {
+			w.onOverflow()
 		}
-		return len(p), nil
 	}
-	w.buf.Write(p)
 	return len(p), nil
 }
 
diff --git a/internal/extract/tools_test.go b/internal/extract/tools_test.go
index 756903d..5af80fb 100644
--- a/internal/extract/tools_test.go
+++ b/internal/extract/tools_test.go
@@ -212,13 +212,17 @@ func TestCallerCancelReturnsPromptly(t *testing.T) {
 // it at all must not cost unbounded memory.
 func TestStderrFloodBounded(t *testing.T) {
 	bin := t.TempDir()
-	fakeTool(t, bin, "pdftotext", `head -c 10000000 /dev/zero | tr '\0' x >&2; exit 1`)
+	// dd, not head -c: OpenBSD's head has no -c.
+	fakeTool(t, bin, "pdftotext", `dd if=/dev/zero bs=10000 count=1000 2>/dev/null | tr '\0' x >&2; exit 1`)
 	e := newWithPath(bin)
 
 	_, err := e.run(context.Background(), maxToolOutput, "pdftotext")
 	if err == nil {
 		t.Fatal("stderr flood: want an error")
 	}
+	if !strings.Contains(err.Error(), strings.Repeat("x", 100)) {
+		t.Fatalf("stderr flood: the message %q does not come from the flood", err)
+	}
 	if max := len("pdftotext failed: ") + 200; len(err.Error()) > max {
 		t.Errorf("stderr flood: message is %d bytes, want <=%d", len(err.Error()), max)
 	}
@@ -231,7 +235,7 @@ func TestStderrFloodBounded(t *testing.T) {
 // result.
 func TestMaxReadCapsToolOutput(t *testing.T) {
 	bin := t.TempDir()
-	fakeTool(t, bin, "pdftotext", `yes aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa | head -c 200000`)
+	fakeTool(t, bin, "pdftotext", `dd if=/dev/zero bs=1000 count=200 2>/dev/null | tr '\0' a`)
 	e := newWithPath(bin)
 	if _, err := text(t, e, file(t, "small.pdf", []byte("%PDF-1.4")), 1024); !errors.Is(err, ErrTooLarge) {
 		t.Fatalf("got %v, want ErrTooLarge (max-read 1024 should have capped a 200000-byte tool output)", err)
diff --git a/internal/lock/lock.go b/internal/lock/lock.go
index 462b26e..2b98227 100644
--- a/internal/lock/lock.go
+++ b/internal/lock/lock.go
@@ -10,6 +10,7 @@ import (
 	"errors"
 	"fmt"
 	"io/fs"
+	"math"
 	"os"
 	"path/filepath"
 	"strconv"
@@ -156,8 +157,10 @@ func parsePid(s string) (int, bool) {
 }
 
 // running reports whether pid names a process that is currently running.
+// A pid outside the kernel's 32-bit range names no process: kill(2) would
+// cut it to its low bits and ask about another one.
 func running(pid int) bool {
-	if pid <= 0 {
+	if pid <= 0 || pid > math.MaxInt32 {
 		return false
 	}
 	proc, err := os.FindProcess(pid)
diff --git a/internal/lock/lock_test.go b/internal/lock/lock_test.go
index fca4d76..53ce5ca 100644
--- a/internal/lock/lock_test.go
+++ b/internal/lock/lock_test.go
@@ -122,3 +122,16 @@ func TestStaleLockIsTakenOver(t *testing.T) {
 		t.Error("the takeover was not reported to the caller")
 	}
 }
+
+// TestRunningRejectsPidsBeyondTheKernelsRange: a pid wider than 32 bits in a
+// damaged lock file is cut to its low bits by kill(2), so without a range
+// check it names some other process - here this very one - and a stale lock
+// would never be taken over.
+func TestRunningRejectsPidsBeyondTheKernelsRange(t *testing.T) {
+	if running(os.Getpid() + 1<<32) {
+		t.Error("running(pid + 2^32) is true: the pid was truncated to this process")
+	}
+	if !running(os.Getpid()) {
+		t.Error("running(own pid) is false")
+	}
+}