Skip to content

Commit 1d59b5b

Browse files
chrischallclaude
chrischall
and
claude
committed
fix(gmail): run PDF text extraction in a killable subprocess
Address the remaining review P1: a goroutine timeout cannot terminate parser work, so repeated hostile PDFs could leave CPU/memory work behind in a long-lived MCP server. - gmail attachment --text now re-execs the gog binary as a hidden `gmail pdf-extract` child (PDF bytes on stdin, text on stdout) under exec.CommandContext, so the 10s timeout SIGKILLs the parser outright. - Child re-checks the input size cap; parent caps extracted-text output at 4 MiB (compressed streams can inflate past the input cap) and surfaces child stderr as the failure reason. - Tests spawn the real subprocess boundary (test binary dispatches via a TestMain hook): extraction through the child, a hung child killed on timeout, corrupt-PDF error propagation, direct child-command IO, and oversized-input rejection. Co-Authored-By: Claude Fable 5 <[email protected]>
1 parent 5e4cd61 commit 1d59b5b

6 files changed

Lines changed: 168 additions & 41 deletions

File tree

‎internal/cmd/gmail.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ type GmailCmd struct {
2727

2828
Settings GmailSettingsCmd `cmd:"" name:"settings" group:"Admin" help:"Settings and admin"`
2929

30+
PDFExtract GmailPDFExtractCmd `cmd:"" name:"pdf-extract" hidden:"" help:"Extract text from PDF bytes on stdin (internal: killable child for attachment --text)"`
31+
3032
Watch GmailWatchCmd `cmd:"" name:"watch" hidden:"" help:"Manage Gmail watch"`
3133
AutoForward GmailAutoForwardCmd `cmd:"" name:"autoforward" hidden:"" help:"Auto-forwarding settings"`
3234
Delegates GmailDelegatesCmd `cmd:"" name:"delegates" hidden:"" help:"Delegate operations"`

‎internal/cmd/gmail_attachment.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -135,7 +135,7 @@ func (c *GmailAttachmentCmd) Run(ctx context.Context, flags *RootFlags) error {
135135
if info != nil {
136136
filename, mimeType = info.Filename, info.MimeType
137137
}
138-
addTextContent(payload, path, bytes, filename, mimeType)
138+
addTextContent(ctx, payload, path, bytes, filename, mimeType)
139139
}
140140
return printAttachmentDownloadResult(ctx, u, payload)
141141
}

‎internal/cmd/gmail_attachment_pdfextract.go‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
package cmd
2+
3+
import (
4+
"context"
5+
"io"
6+
)
7+
8+
// GmailPDFExtractCmd is the hidden re-exec target behind `gmail attachment
9+
// --text`: it reads PDF bytes on stdin and writes extracted text to stdout,
10+
// so the parent can enforce a hard timeout by killing this process.
11+
type GmailPDFExtractCmd struct{}
12+
13+
func (c *GmailPDFExtractCmd) Run(ctx context.Context, flags *RootFlags) error {
14+
text, err := pdfExtractChild(stdinReader(ctx))
15+
if err != nil {
16+
return err
17+
}
18+
_, err = io.WriteString(stdoutWriter(ctx), text)
19+
return err
20+
}

‎internal/cmd/gmail_attachment_text.go‎

Lines changed: 74 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -2,10 +2,13 @@ package cmd
22

33
import (
44
"bytes"
5+
"context"
6+
"errors"
57
"fmt"
68
"io"
79
"net/http"
810
"os"
11+
"os/exec"
912
"path/filepath"
1013
"strings"
1114
"time"
@@ -17,7 +20,7 @@ import (
1720

1821
// addTextContent embeds extracted text for supported attachment types (PDF,
1922
// HTML, plain text), or a reason when extraction is not possible.
20-
func addTextContent(payload map[string]any, path string, size int64, filename, mimeType string) {
23+
func addTextContent(ctx context.Context, payload map[string]any, path string, size int64, filename, mimeType string) {
2124
if size > maxInlineAttachmentBytes {
2225
payload["reason"] = fmt.Sprintf("attachment size %d bytes exceeds inline size limit (%d bytes); content written to path only", size, maxInlineAttachmentBytes)
2326
return
@@ -37,9 +40,7 @@ func addTextContent(payload map[string]any, path string, size int64, filename, m
3740

3841
switch {
3942
case isPDFAttachment(data, filename, mimeType):
40-
text, err := runPDFExtractionWithTimeout(func() (string, error) {
41-
return extractPDFText(data)
42-
}, pdfExtractTimeout)
43+
text, err := extractPDFTextIsolated(ctx, data, pdfExtractTimeout)
4344
if err != nil {
4445
payload["reason"] = fmt.Sprintf("pdf text extraction failed: %v; use --inline or --out for the raw bytes", err)
4546
return
@@ -92,25 +93,77 @@ func isTextAttachment(mimeType string) bool {
9293
// run; the input size is already capped by maxInlineAttachmentBytes.
9394
const pdfExtractTimeout = 10 * time.Second
9495

95-
// runPDFExtractionWithTimeout runs fn and gives up after the timeout, so a
96-
// pathological PDF cannot hang the command (or a long-lived MCP server)
97-
// indefinitely. On timeout the extraction goroutine is abandoned.
98-
func runPDFExtractionWithTimeout(fn func() (string, error), timeout time.Duration) (string, error) {
99-
type result struct {
100-
text string
101-
err error
102-
}
103-
ch := make(chan result, 1)
104-
go func() {
105-
text, err := fn()
106-
ch <- result{text: text, err: err}
107-
}()
108-
select {
109-
case r := <-ch:
110-
return r.text, r.err
111-
case <-time.After(timeout):
96+
// maxExtractedTextBytes caps the text a PDF may expand to (compressed content
97+
// streams can inflate well past the input size cap).
98+
const maxExtractedTextBytes = 4 << 20
99+
100+
// newPDFExtractCmd builds the self-exec command for the extraction child
101+
// (`gog gmail pdf-extract`). GOG_PDF_EXTRACT_CHILD lets the test binary
102+
// dispatch to the child logic instead of re-running the suite.
103+
var newPDFExtractCmd = func(ctx context.Context) (*exec.Cmd, error) {
104+
exe, err := os.Executable()
105+
if err != nil {
106+
return nil, err
107+
}
108+
cmd := exec.CommandContext(ctx, exe, "gmail", "pdf-extract") // #nosec G204 -- re-exec of our own binary with fixed args.
109+
cmd.Env = append(os.Environ(), "GOG_PDF_EXTRACT_CHILD=1")
110+
return cmd, nil
111+
}
112+
113+
// extractPDFTextIsolated parses the PDF in a separate killable process so a
114+
// pathological attachment cannot hang or bloat the command (or a long-lived
115+
// MCP server): on timeout the child is killed outright.
116+
func extractPDFTextIsolated(ctx context.Context, data []byte, timeout time.Duration) (string, error) {
117+
cctx, cancel := context.WithTimeout(ctx, timeout)
118+
defer cancel()
119+
120+
cmd, err := newPDFExtractCmd(cctx)
121+
if err != nil {
122+
return "", err
123+
}
124+
cmd.Stdin = bytes.NewReader(data)
125+
var out, errBuf bytes.Buffer
126+
cmd.Stdout = &cappedWriter{w: &out, remaining: maxExtractedTextBytes}
127+
cmd.Stderr = &cappedWriter{w: &errBuf, remaining: 4096}
128+
129+
runErr := cmd.Run()
130+
if cctx.Err() != nil {
112131
return "", fmt.Errorf("timed out after %s", timeout)
113132
}
133+
if runErr != nil {
134+
if msg := strings.TrimSpace(errBuf.String()); msg != "" {
135+
return "", errors.New(msg)
136+
}
137+
return "", runErr
138+
}
139+
return out.String(), nil
140+
}
141+
142+
// cappedWriter fails the write (and thereby the child) once the cap is hit.
143+
type cappedWriter struct {
144+
w io.Writer
145+
remaining int64
146+
}
147+
148+
func (c *cappedWriter) Write(p []byte) (int, error) {
149+
if int64(len(p)) > c.remaining {
150+
return 0, errors.New("output exceeds size limit")
151+
}
152+
c.remaining -= int64(len(p))
153+
return c.w.Write(p)
154+
}
155+
156+
// pdfExtractChild is the child-process side: read the PDF from stdin (size
157+
// re-checked) and return the extracted text.
158+
func pdfExtractChild(in io.Reader) (string, error) {
159+
data, err := io.ReadAll(io.LimitReader(in, maxInlineAttachmentBytes+1))
160+
if err != nil {
161+
return "", err
162+
}
163+
if int64(len(data)) > maxInlineAttachmentBytes {
164+
return "", fmt.Errorf("pdf input exceeds %d bytes", maxInlineAttachmentBytes)
165+
}
166+
return extractPDFText(data)
114167
}
115168

116169
// extractPDFText pulls the plain text out of a PDF. The pdf library panics on

‎internal/cmd/gmail_attachment_text_unit_test.go‎

Lines changed: 53 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,37 +1,71 @@
11
package cmd
22

33
import (
4-
"errors"
4+
"bytes"
5+
"context"
6+
"os"
7+
"os/exec"
58
"strings"
69
"testing"
710
"time"
811
)
912

10-
func TestRunPDFExtractionWithTimeout_TimesOut(t *testing.T) {
11-
_, err := runPDFExtractionWithTimeout(func() (string, error) {
12-
time.Sleep(500 * time.Millisecond)
13-
return "too late", nil
14-
}, 10*time.Millisecond)
13+
func TestExtractPDFTextIsolated_ExtractsViaSubprocess(t *testing.T) {
14+
pdf := buildMinimalPDF(t, "BT /F1 24 Tf 72 720 Td (Isolated hello) Tj ET")
15+
text, err := extractPDFTextIsolated(context.Background(), pdf, 30*time.Second)
16+
if err != nil {
17+
t.Fatalf("extract: %v", err)
18+
}
19+
if !strings.Contains(text, "Isolated hello") {
20+
t.Fatalf("text=%q", text)
21+
}
22+
}
23+
24+
func TestExtractPDFTextIsolated_KillsChildOnTimeout(t *testing.T) {
25+
old := newPDFExtractCmd
26+
t.Cleanup(func() { newPDFExtractCmd = old })
27+
newPDFExtractCmd = func(ctx context.Context) (*exec.Cmd, error) {
28+
cmd := exec.CommandContext(ctx, os.Args[0]) // #nosec G204 -- test binary re-exec.
29+
cmd.Env = append(os.Environ(), "GOG_PDF_EXTRACT_CHILD=1", "GOG_TEST_PDF_EXTRACT_SLEEP=1")
30+
return cmd, nil
31+
}
32+
33+
start := time.Now()
34+
_, err := extractPDFTextIsolated(context.Background(), []byte("%PDF-1.4"), 100*time.Millisecond)
1535
if err == nil || !strings.Contains(err.Error(), "timed out") {
1636
t.Fatalf("err=%v", err)
1737
}
38+
if elapsed := time.Since(start); elapsed > 10*time.Second {
39+
t.Fatalf("hung child was not killed promptly (took %s)", elapsed)
40+
}
1841
}
1942

20-
func TestRunPDFExtractionWithTimeout_PassesThroughResult(t *testing.T) {
21-
text, err := runPDFExtractionWithTimeout(func() (string, error) {
22-
return "ok", nil
23-
}, time.Second)
24-
if err != nil || text != "ok" {
25-
t.Fatalf("text=%q err=%v", text, err)
43+
func TestExtractPDFTextIsolated_SurfacesChildError(t *testing.T) {
44+
_, err := extractPDFTextIsolated(context.Background(), []byte("%PDF-1.4\nnot a real pdf"), 30*time.Second)
45+
if err == nil {
46+
t.Fatalf("expected error for corrupt pdf")
2647
}
2748
}
2849

29-
func TestRunPDFExtractionWithTimeout_PassesThroughError(t *testing.T) {
30-
wantErr := errors.New("boom")
31-
_, err := runPDFExtractionWithTimeout(func() (string, error) {
32-
return "", wantErr
33-
}, time.Second)
34-
if !errors.Is(err, wantErr) {
35-
t.Fatalf("err=%v", err)
50+
func TestGmailPDFExtractCmd_WritesTextToStdout(t *testing.T) {
51+
pdf := buildMinimalPDF(t, "BT /F1 24 Tf 72 720 Td (Child direct) Tj ET")
52+
var out, errBuf bytes.Buffer
53+
ctx := newCmdRuntimeIOContext(t, bytes.NewReader(pdf), &out, &errBuf)
54+
55+
if err := (&GmailPDFExtractCmd{}).Run(ctx, &RootFlags{}); err != nil {
56+
t.Fatalf("Run: %v", err)
57+
}
58+
if !strings.Contains(out.String(), "Child direct") {
59+
t.Fatalf("stdout=%q", out.String())
60+
}
61+
}
62+
63+
func TestGmailPDFExtractCmd_ErrorsOnOversizedInput(t *testing.T) {
64+
big := bytes.Repeat([]byte("x"), maxInlineAttachmentBytes+1)
65+
var out, errBuf bytes.Buffer
66+
ctx := newCmdRuntimeIOContext(t, bytes.NewReader(big), &out, &errBuf)
67+
68+
if err := (&GmailPDFExtractCmd{}).Run(ctx, &RootFlags{}); err == nil {
69+
t.Fatalf("expected size error")
3670
}
3771
}

‎internal/cmd/testmain_test.go‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,32 @@
11
package cmd
22

33
import (
4+
"fmt"
45
"os"
56
"path/filepath"
67
"testing"
8+
"time"
79

810
_ "github.com/steipete/gogcli/internal/tzembed" // Embed IANA timezone database for Windows test support
911
)
1012

1113
func TestMain(m *testing.M) {
14+
// Isolated PDF extraction re-execs the current binary; under `go test`
15+
// that is this test binary, so dispatch to the child logic instead of
16+
// recursively running the suite.
17+
if os.Getenv("GOG_PDF_EXTRACT_CHILD") == "1" {
18+
if os.Getenv("GOG_TEST_PDF_EXTRACT_SLEEP") == "1" {
19+
time.Sleep(time.Hour)
20+
}
21+
text, err := pdfExtractChild(os.Stdin)
22+
if err != nil {
23+
fmt.Fprintln(os.Stderr, err)
24+
os.Exit(1)
25+
}
26+
fmt.Print(text)
27+
os.Exit(0)
28+
}
29+
1230
contactsSearchWarmupDelay = 0
1331

1432
root, err := os.MkdirTemp("", "gogcli-tests-*")

0 commit comments

Comments
 (0)