fix(sdk,perfbench): licensing open race, httpx retry buffering, perfbench -count (D-0621, D-1112, D-0606)

sdk/go/httpx/client.go

@@ -77,7 +77,7 @@ func (c *Client) Do(ctx context.Context, req *http.Request) (*http.Response, err
 		req.Header.Set("User-Agent", c.opts.UserAgent)
 	}
 
-	getBody, buffered, err := bodySource(req, c.opts.MaxRetries > 0)
+	getBody, buffered, inferred, err := bodySource(req, c.opts.MaxRetries > 0)
 	if err != nil {
 		return nil, err
 	}
@@ -112,6 +112,9 @@ func (c *Client) Do(ctx context.Context, req *http.Request) (*http.Response, err
 				}
 				reqClone.Body = body
 			}
+			if buffered && inferred >= 0 {
+				reqClone.ContentLength = inferred
+			}
 		}
 		resp, lastErr = c.HTTP.Do(reqClone)
 		if lastErr != nil {
@@ -136,31 +139,41 @@ func (c *Client) Do(ctx context.Context, req *http.Request) (*http.Response, err
 // single attempt (retries false), or a body without GetBody larger than
 // maxBufferedBody (req.Body is then rebuilt so the one attempt still sends it
 // whole). buffered reports that req.Body was read into memory and closed, so
-// even the first attempt must use the copy; req.ContentLength is set to the
-// buffered length.
-func bodySource(req *http.Request, retries bool) (getBody func() (io.ReadCloser, error), buffered bool, err error) {
+// even the first attempt must use the copy. inferred is the buffered length
+// for Do to set on each attempt's clone, or -1 when the length must not be
+// inferred: a request with Trailer set stays chunked so its trailers are
+// sent. The caller's req.ContentLength is never changed; an explicit
+// ContentLength > 0 that differs from the buffered length is an error rather
+// than a silently truncated or padded body.
+func bodySource(req *http.Request, retries bool) (getBody func() (io.ReadCloser, error), buffered bool, inferred int64, err error) {
 	if !retries || req.Body == nil || req.Body == http.NoBody {
-		return nil, false, nil
+		return nil, false, -1, nil
 	}
 	if req.GetBody != nil {
-		return req.GetBody, false, nil
+		return req.GetBody, false, -1, nil
 	}
 	orig := req.Body
 	data, err := io.ReadAll(io.LimitReader(orig, maxBufferedBody+1))
 	if err != nil {
 		_ = orig.Close()
-		return nil, false, fmt.Errorf("httpx: read request body: %w", err)
+		return nil, false, -1, fmt.Errorf("httpx: read request body: %w", err)
 	}
 	if int64(len(data)) > maxBufferedBody {
 		req.Body = struct {
 			io.Reader
 			io.Closer
 		}{io.MultiReader(bytes.NewReader(data), orig), orig}
-		return nil, false, nil
+		return nil, false, -1, nil
 	}
 	_ = orig.Close()
-	req.ContentLength = int64(len(data))
+	if req.ContentLength > 0 && req.ContentLength != int64(len(data)) {
+		return nil, false, -1, fmt.Errorf("httpx: request ContentLength %d does not match body length %d", req.ContentLength, len(data))
+	}
+	inferred = int64(len(data))
+	if req.Trailer != nil {
+		inferred = -1
+	}
 	return func() (io.ReadCloser, error) {
 		return io.NopCloser(bytes.NewReader(data)), nil
-	}, true, nil
+	}, true, inferred, nil
 }

sdk/go/httpx/client_retry_test.go

@@ -8,6 +8,7 @@ import (
 	"net/http/httptest"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 )
@@ -238,3 +239,90 @@ func TestDo_RedirectReplaysBufferedBody(t *testing.T) {
 		t.Errorf("body at redirect target = %q, want %q", final, payload)
 	}
 }
+
+// TestDo_ContentLengthMismatchErrors verifies that an explicit ContentLength
+// that differs from the buffered body is rejected, not sent truncated.
+func TestDo_ContentLengthMismatchErrors(t *testing.T) {
+	var hits int32
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		atomic.AddInt32(&hits, 1)
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer srv.Close()
+
+	c := New(ClientOptions{MaxRetries: 1, RetryDelay: time.Millisecond, Timeout: 5 * time.Second})
+	req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, srv.URL,
+		onlyReader{strings.NewReader(strings.Repeat("x", 100))})
+	if err != nil {
+		t.Fatalf("NewRequest: %v", err)
+	}
+	req.ContentLength = 10
+	resp, err := c.Do(context.Background(), req)
+	if err == nil {
+		resp.Body.Close()
+		t.Fatal("Do accepted ContentLength 10 for a 100 byte body")
+	}
+	if atomic.LoadInt32(&hits) != 0 {
+		t.Errorf("server was contacted %d times, want 0", hits)
+	}
+}
+
+// TestDo_DoesNotMutateCallerContentLength verifies the inferred length lands
+// on the per-attempt clone only.
+func TestDo_DoesNotMutateCallerContentLength(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		_, _ = io.Copy(io.Discard, r.Body)
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer srv.Close()
+
+	c := New(ClientOptions{MaxRetries: 1, RetryDelay: time.Millisecond, Timeout: 5 * time.Second})
+	req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, srv.URL,
+		onlyReader{strings.NewReader("12345")})
+	if err != nil {
+		t.Fatalf("NewRequest: %v", err)
+	}
+	resp, err := c.Do(context.Background(), req)
+	if err != nil {
+		t.Fatalf("Do: %v", err)
+	}
+	resp.Body.Close()
+	if req.ContentLength != 0 {
+		t.Errorf("caller ContentLength = %d, want it untouched (0)", req.ContentLength)
+	}
+}
+
+// TestDo_TrailerKeepsChunkedEncoding verifies that a request with Trailer set
+// is not given a Content-Length, so its trailers still go out.
+func TestDo_TrailerKeepsChunkedEncoding(t *testing.T) {
+	var mu sync.Mutex
+	var gotLen int64
+	var gotTrailer string
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		_, _ = io.Copy(io.Discard, r.Body)
+		mu.Lock()
+		gotLen = r.ContentLength
+		gotTrailer = r.Trailer.Get("X-Sum")
+		mu.Unlock()
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer srv.Close()
+
+	c := New(ClientOptions{MaxRetries: 1, RetryDelay: time.Millisecond, Timeout: 5 * time.Second})
+	req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, srv.URL,
+		onlyReader{strings.NewReader("12345")})
+	if err != nil {
+		t.Fatalf("NewRequest: %v", err)
+	}
+	req.Trailer = http.Header{"X-Sum": {"abc"}}
+	resp, err := c.Do(context.Background(), req)
+	if err != nil {
+		t.Fatalf("Do: %v", err)
+	}
+	resp.Body.Close()
+	mu.Lock()
+	defer mu.Unlock()
+	if gotLen != -1 || gotTrailer != "abc" {
+		t.Errorf("server saw ContentLength %d trailer %q, want -1 and %q", gotLen, gotTrailer, "abc")
+	}
+}

sdk/go/licensing/entitlement.go

@@ -49,18 +49,25 @@ func verifyFile(path, slug string, now time.Time) error {
 	return verifyToken(strings.TrimSpace(string(data)), slug, now)
 }
 
+// beforeTokenOpen is a test seam run just before the token file is opened.
+var beforeTokenOpen func()
+
 // Purpose: bound reads of mounted files. Inputs: path. Outputs: bytes or rejection.
 // Constraints: oversized files never allocate unbounded memory. SPORT: plugin.entitlement-token.
 func readTokenFile(path string) ([]byte, error) {
-	info, err := os.Lstat(path)
-	if err != nil || !info.Mode().IsRegular() {
-		return nil, ErrEntitlementExpired
+	if beforeTokenOpen != nil {
+		beforeTokenOpen()
 	}
-	f, err := os.Open(path)
+	f, err := openTokenFile(path)
 	if err != nil {
 		return nil, ErrEntitlementExpired
 	}
 	defer func() { _ = f.Close() }()
+	// Check the opened descriptor, not the path: a path check followed by an
+	// open leaves a window to swap in a FIFO or symlink.
+	if info, err := f.Stat(); err != nil || !info.Mode().IsRegular() {
+		return nil, ErrEntitlementExpired
+	}
 	data, err := io.ReadAll(io.LimitReader(f, maxTokenBytes+1))
 	if len(data) > maxTokenBytes {
 		return nil, errTokenTooLarge

sdk/go/licensing/entitlement_fifo_test.go

@@ -5,6 +5,7 @@ package licensing
 import (
 	"context"
 	"errors"
+	"os"
 	"path/filepath"
 	"syscall"
 	"testing"
@@ -26,3 +27,39 @@ func TestEntitlementFIFORejectedWithoutBlocking(t *testing.T) {
 		t.Fatal("FIFO authorized")
 	}
 }
+
+// A FIFO or symlink swapped in right before the open must be rejected without
+// blocking the reader.
+func TestEntitlementSwapRaceRejected(t *testing.T) {
+	for _, kind := range []string{"fifo", "symlink"} {
+		t.Run(kind, func(t *testing.T) {
+			dir := t.TempDir()
+			path := filepath.Join(dir, "entitlement.jwt")
+			target := filepath.Join(dir, "target.jwt")
+			for _, p := range []string{path, target} {
+				if err := os.WriteFile(p, []byte("x"), 0600); err != nil {
+					t.Fatal(err)
+				}
+			}
+			beforeTokenOpen = func() {
+				_ = os.Remove(path)
+				if kind == "fifo" {
+					_ = syscall.Mkfifo(path, 0600)
+				} else {
+					_ = os.Symlink(target, path)
+				}
+			}
+			defer func() { beforeTokenOpen = nil }()
+			done := make(chan error, 1)
+			go func() { _, err := readTokenFile(path); done <- err }()
+			select {
+			case err := <-done:
+				if !errors.Is(err, ErrEntitlementExpired) {
+					t.Fatalf("swapped %s returned %v", kind, err)
+				}
+			case <-time.After(5 * time.Second):
+				t.Fatalf("swapped %s blocked the reader", kind)
+			}
+		})
+	}
+}

sdk/go/licensing/entitlement_open_other.go

@@ -0,0 +1,17 @@
+//go:build !unix
+
+package licensing
+
+import "os"
+
+// Purpose: open a mounted token file; platforms without O_NOFOLLOW/O_NONBLOCK
+// reject a symlink up front. Inputs: path. Outputs: open file or error.
+// Constraints: best effort only, the caller checks the opened descriptor.
+// SPORT: plugin.entitlement-token.
+func openTokenFile(path string) (*os.File, error) {
+	info, err := os.Lstat(path)
+	if err != nil || !info.Mode().IsRegular() {
+		return nil, ErrEntitlementExpired
+	}
+	return os.Open(path)
+}

sdk/go/licensing/entitlement_open_unix.go

@@ -0,0 +1,16 @@
+//go:build unix
+
+package licensing
+
+import (
+	"os"
+	"syscall"
+)
+
+// Purpose: open a mounted token file without following a final symlink or
+// blocking on a FIFO. Inputs: path. Outputs: open file or error.
+// Constraints: O_NOFOLLOW and O_NONBLOCK make the swap race harmless; the
+// caller still checks the opened descriptor is a regular file. SPORT: plugin.entitlement-token.
+func openTokenFile(path string) (*os.File, error) {
+	return os.OpenFile(path, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0)
+}

tools/perfbench/ab_guard_test.go

@@ -108,8 +108,11 @@ func (emptyProbes) Run(context.Context, scenarios.Config) ([]scenarios.Sample, e
 }
 func (emptyProbes) Probes() []scenarios.Probe { return nil }
 
+// Registered in init, not in the test: the registry is global and Register
+// panics on a duplicate, so registering per test run broke go test -count=N.
+func init() { scenarios.Register(emptyProbes{}) }
+
 func TestABRefusesEmptyMetrics(t *testing.T) {
-	scenarios.Register(emptyProbes{})
 	bin := script(t, "nself", okScript)
 	for _, extra := range [][]string{{}, {"-json"}} {
 		var out, errb bytes.Buffer

tools/perfbench/run_test.go

@@ -66,8 +66,11 @@ func TestUnknownScenarioAndSubcommand(t *testing.T) {
 	}
 }
 
+// Registered in init, not in the test, so go test -count=N does not hit the
+// registry's duplicate-name panic.
+func init() { scenarios.Register(mixedScenario{}) }
+
 func TestMixedUnitsFailTheRun(t *testing.T) {
-	scenarios.Register(mixedScenario{})
 	var out, errb bytes.Buffer
 	code := dispatch([]string{"run", "-scenario", "test-mixed", "-bin", "x", "-runs", "2"}, &out, &errb)
 	if code != 1 || !strings.Contains(errb.String(), "mixed units") {