From 77bd66acabff4691a35e6e9926c6138cd5978351 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sat, 24 Oct 2026 08:09:47 +0200 Subject: [PATCH 1/2] Bound Git requests by their body and response progress upload-pack was started with its stdin attached to the request body, after the read deadline had been extended to two hours, and ls-refs requests took no slot. A client that sent part of a request and then nothing kept an upload-pack process waiting, and any number of them could. Stateless requests are complete once sent, so the handler now reads the body in full first, within the server's read timeout, keeping bodies past 1 MiB in a temporary file. Only then are the deadlines extended for the response. info/refs and ls-refs share their own slots, four per CPU, so they still never wait behind clones. Each response write is bounded to two minutes, so a client that stops reading a pack frees its slot instead of holding it until the request timeout. --- internal/gitcache/cache.go | 18 +++- internal/gitcache/helpers_test.go | 11 ++- internal/gitcache/serve.go | 135 +++++++++++++++++++++++------- internal/gitcache/serve_test.go | 101 ++++++++++++++++++++++ internal/handler/git.go | 79 +++++++++++++---- internal/handler/git_test.go | 91 +++++++++++++++++++- 6 files changed, 384 insertions(+), 51 deletions(-) diff --git a/internal/gitcache/cache.go b/internal/gitcache/cache.go index 73d0529e..eb1578f7 100644 --- a/internal/gitcache/cache.go +++ b/internal/gitcache/cache.go @@ -69,14 +69,15 @@ type Options struct { // else in CacheDir is touched, so it may hold other data. Upstreams map[string]string - // refCheckTimeout, lowSpeedTime, maxRequestBody and maxUploadPacks - // override their defaults in tests, and refreshWaitOver, when set, - // replaces the RefreshWait timer. + // refCheckTimeout, lowSpeedTime, maxRequestBody, maxUploadPacks and + // maxRefListings override their defaults in tests, and refreshWaitOver, + // when set, replaces the RefreshWait timer. refCheckTimeout time.Duration refreshWaitOver <-chan time.Time lowSpeedTime int maxRequestBody int64 maxUploadPacks int + maxRefListings int } // Cache is the Git repository cache. internal/handler serves it at /git. @@ -90,6 +91,7 @@ type Cache struct { relay *relay fetches chan struct{} uploadPacks chan struct{} + refListings chan struct{} refCheckTimeout time.Duration lowSpeedTime int maxRequestBody int64 @@ -149,6 +151,11 @@ const maxConcurrentFetches = 16 // some packs wait on slow clients. const uploadPacksPerCPU = 2 +// refListingsPerCPU caps the upload-pack processes listing refs at once, for +// info/refs and ls-refs. They build no pack and so do not wait behind +// clones, but each is still a process. +const refListingsPerCPU = 4 + // New creates the cache, loading repositories already in CacheDir. func New(opts Options) (*Cache, error) { gitPath, err := exec.LookPath("git") @@ -186,6 +193,10 @@ func New(opts Options) (*Cache, error) { if opts.maxUploadPacks > 0 { uploadPacks = opts.maxUploadPacks } + refListings := refListingsPerCPU * runtime.GOMAXPROCS(0) + if opts.maxRefListings > 0 { + refListings = opts.maxRefListings + } c := &Cache{ opts: opts, client: &client, @@ -195,6 +206,7 @@ func New(opts Options) (*Cache, error) { lock: lock, fetches: make(chan struct{}, maxConcurrentFetches), uploadPacks: make(chan struct{}, uploadPacks), + refListings: make(chan struct{}, refListings), refCheckTimeout: defaultRefCheckTimeout, lowSpeedTime: defaultLowSpeedTime, maxRequestBody: defaultMaxRequestBody, diff --git a/internal/gitcache/helpers_test.go b/internal/gitcache/helpers_test.go index d5d82b73..55b73335 100644 --- a/internal/gitcache/helpers_test.go +++ b/internal/gitcache/helpers_test.go @@ -353,6 +353,15 @@ func testFront(c *Cache, upstreamURL string, fetchWants bool) http.Handler { http.Error(w, "not served", http.StatusForbidden) return } + var req *Request + if !infoRefs { + var err error + if req, err = c.ReadRequest(r); err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } + defer req.Close() + } lease, err := c.Open(r.Context(), "up/"+strings.TrimSuffix(repo, ".git")+".git", upstreamURL+"/"+repo, OpenOptions{Refresh: infoRefs, FetchWants: fetchWants}) switch { @@ -370,7 +379,7 @@ func testFront(c *Cache, upstreamURL string, fetchWants bool) http.Handler { if infoRefs { lease.ServeInfoRefs(w, r) } else { - lease.ServeUploadPack(w, r) + lease.ServeUploadPack(w, r, req) } }) } diff --git a/internal/gitcache/serve.go b/internal/gitcache/serve.go index 6d88df46..216edc12 100644 --- a/internal/gitcache/serve.go +++ b/internal/gitcache/serve.go @@ -7,6 +7,7 @@ import ( "errors" "io" "net/http" + "os" "path/filepath" "regexp" "slices" @@ -73,6 +74,11 @@ func (l *Lease) Release() { // ServeInfoRefs answers GET info/refs?service=git-upload-pack. func (l *Lease) ServeInfoRefs(w http.ResponseWriter, r *http.Request) { + c := l.cache + if !takeSlot(r.Context(), c.refListings) { + return + } + defer func() { <-c.refListings }() protocol := gitProtocol(r) setNoCache(w) w.Header().Set("Content-Type", advertisementType) @@ -81,34 +87,32 @@ func (l *Lease) ServeInfoRefs(w http.ResponseWriter, r *http.Request) { if !isProtocolV2(protocol) { _, _ = io.WriteString(w, pktLine("# service=git-upload-pack\n")+pktFlush) } - l.cache.uploadPack(w, r, l.repo, protocol, nil, "--advertise-refs") + c.uploadPack(w, r, l.repo, protocol, nil, "--advertise-refs") } -// ServeUploadPack answers POST git-upload-pack. With OpenOptions.FetchWants, -// commits the request wants that the cache lacks are fetched from the -// upstream first. -func (l *Lease) ServeUploadPack(w http.ResponseWriter, r *http.Request) { +// ServeUploadPack answers POST git-upload-pack with req, the request's body +// as ReadRequest read it. With OpenOptions.FetchWants, commits the request +// wants that the cache lacks are fetched from the upstream first. +func (l *Lease) ServeUploadPack(w http.ResponseWriter, r *http.Request, req *Request) { c := l.cache - // The raw body is limited too: a gzip stream of any length can decode - // to almost nothing. - raw := &bodyLimit{r: r.Body, remaining: c.maxRequestBody} - decoded := raw - if r.Header.Get("Content-Encoding") == "gzip" { - gz, err := gzip.NewReader(raw) + body := req.reader() + if req.gzip { + gz, err := gzip.NewReader(body) if err != nil { http.Error(w, "invalid gzip body", http.StatusBadRequest) return } defer func() { _ = gz.Close() }() - decoded = &bodyLimit{r: gz, remaining: c.maxRequestBody} + // A gzip stream of any length can decode to almost nothing. + decoded := &bodyLimit{r: gz, remaining: c.maxRequestBody} + defer func() { + if decoded.exceeded.Load() { + c.logger.Warn("git cache: request body passed the limit", + "repository", l.repo.key, "limit", c.maxRequestBody) + } + }() + body = decoded } - defer func() { - if raw.exceeded.Load() || decoded.exceeded.Load() { - c.logger.Warn("git cache: request body passed the limit", - "repository", l.repo.key, "limit", c.maxRequestBody) - } - }() - body := io.Reader(decoded) if l.fetchWants { var wants []string wants, body = readWants(body) @@ -119,20 +123,31 @@ func (l *Lease) ServeUploadPack(w http.ResponseWriter, r *http.Request) { } var command string command, body = requestCommand(body) - // ls-refs builds no pack, so it does not wait behind clones. - if command != "ls-refs" { - select { - case c.uploadPacks <- struct{}{}: - defer func() { <-c.uploadPacks }() - case <-r.Context().Done(): - return - } + // ls-refs builds no pack, so it waits for a ref listing slot rather than + // behind clones. + slots := c.uploadPacks + if command == "ls-refs" { + slots = c.refListings + } + if !takeSlot(r.Context(), slots) { + return } + defer func() { <-slots }() setNoCache(w) w.Header().Set("Content-Type", resultType) c.uploadPack(w, r, l.repo, gitProtocol(r), body) } +// takeSlot takes a slot from slots, or reports false once ctx is done. +func takeSlot(ctx context.Context, slots chan struct{}) bool { + select { + case slots <- struct{}{}: + return true + case <-ctx.Done(): + return false + } +} + // requestCommand returns the protocol v2 command an upload-pack request // starts with, such as "fetch", or "" for an earlier protocol, with a reader // that replays the whole body. @@ -175,7 +190,67 @@ func (c *Cache) uploadPack(w http.ResponseWriter, r *http.Request, rp *repo, pro // the request may run. const defaultMaxRequestBody = 100 << 20 -var errRequestTooLarge = errors.New("request body too large") +// maxMemoryRequest is the most of a request body kept in memory; the rest of +// a larger one goes to a temporary file. +const maxMemoryRequest = 1 << 20 + +// ErrRequestTooLarge is returned by ReadRequest for a body past the limit. +var ErrRequestTooLarge = errors.New("request body too large") + +// Request is the body of a POST git-upload-pack request, read in full +// before anything is started for it. Stateless requests are complete once +// sent, so a client that sends one slowly, or not at all, holds no +// upload-pack process and is bound by the server's read timeout. +type Request struct { + gzip bool + data []byte + file *os.File +} + +// ReadRequest reads the body of a POST git-upload-pack request, still +// encoded. A body past the limit fails with ErrRequestTooLarge. Close the +// request once it is served. +func (c *Cache) ReadRequest(r *http.Request) (*Request, error) { + req := &Request{gzip: r.Header.Get("Content-Encoding") == "gzip"} + body := &bodyLimit{r: r.Body, remaining: c.maxRequestBody} + var buf bytes.Buffer + if _, err := buf.ReadFrom(io.LimitReader(body, maxMemoryRequest)); err != nil { + return nil, err + } + if buf.Len() < maxMemoryRequest { + req.data = buf.Bytes() + return req, nil + } + file, err := os.CreateTemp("", "git-upload-pack-*") + if err != nil { + return nil, err + } + req.file = file + if _, err := buf.WriteTo(file); err != nil { + req.Close() + return nil, err + } + if _, err := io.Copy(file, body); err != nil { + req.Close() + return nil, err + } + return req, nil +} + +// Close removes any temporary file holding the request. +func (req *Request) Close() { + if req.file != nil { + _ = req.file.Close() + _ = os.Remove(req.file.Name()) + } +} + +func (req *Request) reader() io.Reader { + if req.file != nil { + return io.NewSectionReader(req.file, 0, 1<<63-1) + } + return bytes.NewReader(req.data) +} // bodyLimit fails reads past remaining bytes, where io.LimitReader would end // the body quietly and leave upload-pack a request cut short. @@ -187,7 +262,7 @@ type bodyLimit struct { func (b *bodyLimit) Read(p []byte) (int, error) { if b.exceeded.Load() { - return 0, errRequestTooLarge + return 0, ErrRequestTooLarge } // One byte past the limit tells a longer body from one that ends at it. if int64(len(p)) > b.remaining+1 { @@ -201,7 +276,7 @@ func (b *bodyLimit) Read(p []byte) (int, error) { n = int(b.remaining) b.remaining = 0 b.exceeded.Store(true) - return n, errRequestTooLarge + return n, ErrRequestTooLarge } func gitProtocol(r *http.Request) string { diff --git a/internal/gitcache/serve_test.go b/internal/gitcache/serve_test.go index b5779ae2..50b8f752 100644 --- a/internal/gitcache/serve_test.go +++ b/internal/gitcache/serve_test.go @@ -3,9 +3,11 @@ package gitcache import ( "bytes" "compress/gzip" + "errors" "fmt" "io" "net/http" + "os" "os/exec" "path/filepath" "slices" @@ -169,6 +171,105 @@ func TestUploadPackSlots(t *testing.T) { } } +// TestRefListingSlots holds the only slot for listing refs: info/refs and +// ls-refs wait for it. +func TestRefListingSlots(t *testing.T) { + upstream := newFakeUpstream(t) + upstream.newRepo("repo") + tc := newTestCache(t, upstream.server.URL, func(o *Options) { o.maxRefListings = 1 }) + tc.clone(t, "repo") + + post := func(body string) <-chan error { + done := make(chan error, 1) + go func() { + req, _ := http.NewRequest(http.MethodPost, tc.url("repo")+"/git-upload-pack", strings.NewReader(body)) + req.Header.Set("Content-Type", requestType) + req.Header.Set("Git-Protocol", "version=2") + resp, err := http.DefaultClient.Do(req) + if err != nil { + done <- err + return + } + out, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if !strings.Contains(string(out), "refs/heads/main") { + err = fmt.Errorf("status %d, body %q", resp.StatusCode, out) + } + done <- err + }() + return done + } + get := func() <-chan error { + done := make(chan error, 1) + go func() { + resp, err := http.Get(tc.url("repo") + "/info/refs?service=git-upload-pack") + if err != nil { + done <- err + return + } + out, _ := io.ReadAll(resp.Body) + _ = resp.Body.Close() + if !strings.Contains(string(out), "refs/heads/main") { + err = fmt.Errorf("status %d, body %q", resp.StatusCode, out) + } + done <- err + }() + return done + } + + tc.refListings <- struct{}{} + held := true + release := func() { + if held { + held = false + <-tc.refListings + } + } + defer release() + lsRefs := post(pktLine("command=ls-refs\n") + pktDelim + pktLine("ref-prefix refs/heads/\n") + pktFlush) + infoRefs := get() + select { + case err := <-lsRefs: + t.Fatalf("ls-refs finished while the slot was held: %v", err) + case err := <-infoRefs: + t.Fatalf("info/refs finished while the slot was held: %v", err) + case <-time.After(time.Second): + } + release() + for name, done := range map[string]<-chan error{"ls-refs": lsRefs, "info/refs": infoRefs} { + if err := <-done; err != nil { + t.Errorf("%s: %v", name, err) + } + } +} + +// TestReadRequest reads bodies kept in memory and, past maxMemoryRequest, in +// a temporary file removed on Close. +func TestReadRequest(t *testing.T) { + c := &Cache{maxRequestBody: 4 << 20} + for _, size := range []int{0, 100, maxMemoryRequest, 3 << 20} { + body := strings.Repeat("x", size) + r, _ := http.NewRequest(http.MethodPost, "/", strings.NewReader(body)) + req, err := c.ReadRequest(r) + if err != nil { + t.Fatalf("%d bytes: %v", size, err) + } + if got, _ := io.ReadAll(req.reader()); string(got) != body { + t.Errorf("%d bytes: read back %d", size, len(got)) + } + req.Close() + if req.file != nil { + if _, err := os.Stat(req.file.Name()); !errors.Is(err, os.ErrNotExist) { + t.Errorf("%d bytes: temporary file left: %v", size, err) + } + } + } + r, _ := http.NewRequest(http.MethodPost, "/", strings.NewReader(strings.Repeat("x", 5<<20))) + if _, err := c.ReadRequest(r); !errors.Is(err, ErrRequestTooLarge) { + t.Errorf("body past the limit: %v, want ErrRequestTooLarge", err) + } +} + func TestRequestCommand(t *testing.T) { for _, tt := range []struct{ body, want string }{ {pktLine("command=ls-refs\n") + pktDelim + pktFlush, "ls-refs"}, diff --git a/internal/handler/git.go b/internal/handler/git.go index 3fbacfd7..895be44a 100644 --- a/internal/handler/git.go +++ b/internal/handler/git.go @@ -21,10 +21,14 @@ const ( gitRequestType = "application/x-git-upload-pack-request" // gitRequestTimeout replaces the server's read and write timeouts for Git - // requests: a cold clone of a large repository keeps the client waiting - // before the first byte, a large pack takes a while to send, and the rest - // of a request body is read only after any fetch it needs. + // requests once any request body has been read: a cold clone of a large + // repository keeps the client waiting before the first byte, and a large + // pack takes a while to send. gitRequestTimeout = 2 * time.Hour + // gitWriteStallTimeout bounds each write of a Git response, so a client + // that stops reading does not hold an upload-pack process for + // gitRequestTimeout. + gitWriteStallTimeout = 2 * time.Minute ) // GitHandler serves Git repositories read-only over smart HTTP at @@ -35,6 +39,8 @@ type GitHandler struct { proxy *Proxy cache *gitcache.Cache upstreams map[string]gitUpstream + // writeStall is gitWriteStallTimeout, shorter in tests. + writeStall time.Duration } type gitUpstream struct { @@ -49,7 +55,12 @@ type gitUpstream struct { // NewGitHandler creates a Git handler serving upstream.git from cache. func NewGitHandler(proxy *Proxy, cache *gitcache.Cache, upstream config.UpstreamConfig) *GitHandler { - h := &GitHandler{proxy: proxy, cache: cache, upstreams: make(map[string]gitUpstream, len(upstream.Git))} + h := &GitHandler{ + proxy: proxy, + cache: cache, + upstreams: make(map[string]gitUpstream, len(upstream.Git)), + writeStall: gitWriteStallTimeout, + } for name, git := range upstream.Git { git.URL = strings.TrimSuffix(git.URL, "/") h.upstreams[name] = gitUpstream{ @@ -84,19 +95,28 @@ func (h *GitHandler) serve(w http.ResponseWriter, r *http.Request) { http.Error(w, msg, status) return } - deadline := time.Now().Add(gitRequestTimeout) + // An upload-pack request is read in full first, within the server's read + // timeout, so nothing waits on a client that is slow to send one. + var body *gitcache.Request + if !req.infoRefs { + var err error + if body, err = h.cache.ReadRequest(r); err != nil { + h.serveRequestError(w, req, err) + return + } + defer body.Close() + } + // The read deadline is extended too: once the body is read, an HTTP/1 + // server watches the connection, and a read timeout there would cancel + // the request. rc := http.NewResponseController(w) + deadline := time.Now().Add(gitRequestTimeout) if err := errors.Join(rc.SetReadDeadline(deadline), rc.SetWriteDeadline(deadline)); err != nil { h.proxy.Logger.Warn("git: cannot extend the request deadlines", "error", err) } - // upload-pack writes its response while its stdin is still attached to - // the request body. Without full duplex, an HTTP/1 server reads off and - // closes whatever body is left once the response is flushed. - if !req.infoRefs { - if err := rc.EnableFullDuplex(); err != nil { - h.proxy.Logger.Warn("git: cannot enable full duplex", "error", err) - } - } + w = &stallWriter{ResponseWriter: w, rc: rc, stall: h.writeStall, deadline: deadline} + // What is left buffered once the handler returns is bounded as well. + defer func() { _ = rc.SetWriteDeadline(time.Now().Add(h.writeStall)) }() lease, err := h.cache.Open(r.Context(), req.key, req.repoURL, gitcache.OpenOptions{Refresh: req.infoRefs, FetchWants: req.fetchWants}) @@ -106,7 +126,7 @@ func (h *GitHandler) serve(w http.ResponseWriter, r *http.Request) { } defer lease.Release() if !req.infoRefs { - lease.ServeUploadPack(w, r) + lease.ServeUploadPack(w, r, body) return } if lease.Hit() { @@ -194,6 +214,37 @@ func validGitSegment(segment string, last bool) bool { return true } +// serveRequestError answers an upload-pack request whose body could not be +// read. +func (h *GitHandler) serveRequestError(w http.ResponseWriter, req gitRequest, err error) { + if errors.Is(err, gitcache.ErrRequestTooLarge) { + h.proxy.Logger.Warn("git: request body passed the limit", "repository", req.key) + http.Error(w, "request body too large", http.StatusRequestEntityTooLarge) + return + } + http.Error(w, "cannot read the request body", http.StatusBadRequest) +} + +// stallWriter bounds each write of a response by stall, where the response +// as a whole may take until deadline. +type stallWriter struct { + http.ResponseWriter + rc *http.ResponseController + stall time.Duration + deadline time.Time +} + +func (w *stallWriter) Write(p []byte) (int, error) { + _ = w.rc.SetWriteDeadline(time.Now().Add(w.stall)) + n, err := w.ResponseWriter.Write(p) + _ = w.rc.SetWriteDeadline(w.deadline) + return n, err +} + +func (w *stallWriter) Unwrap() http.ResponseWriter { + return w.ResponseWriter +} + // serveError answers a request whose repository could not be opened. The // cache has already logged why. func (h *GitHandler) serveError(w http.ResponseWriter, r *http.Request, err error) { diff --git a/internal/handler/git_test.go b/internal/handler/git_test.go index ec0b6306..0aeff4ba 100644 --- a/internal/handler/git_test.go +++ b/internal/handler/git_test.go @@ -1,9 +1,12 @@ package handler import ( + "bufio" "fmt" "io" "log/slog" + "math/rand/v2" + "net" "net/http" "net/http/cgi" "net/http/httptest" @@ -131,9 +134,13 @@ type gitTestSetup struct { logs *syncLog // slow delays every upstream request by half a second. slow atomic.Bool + // active counts the requests the handler is serving. + active atomic.Int32 } -func newGitTestSetup(t *testing.T, readTimeout time.Duration) *gitTestSetup { +// newGitTestSetup serves the cache with the given server read timeout and, +// unless zero, GitHandler.writeStall. +func newGitTestSetup(t *testing.T, readTimeout, writeStall time.Duration) *gitTestSetup { t.Helper() execPath := runGitCommand(t, "", "--exec-path") backendPath := "" @@ -180,7 +187,15 @@ func newGitTestSetup(t *testing.T, readTimeout time.Duration) *gitTestSetup { } t.Cleanup(func() { _ = cache.Close() }) h := NewGitHandler(&Proxy{Logger: logger}, cache, config.UpstreamConfig{Git: map[string]config.GitUpstream{"up": {URL: upstream.URL}}}) - g.server = httptest.NewUnstartedServer(http.StripPrefix("/git", h.Routes())) + if writeStall > 0 { + h.writeStall = writeStall + } + routes := http.StripPrefix("/git", h.Routes()) + g.server = httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + g.active.Add(1) + defer g.active.Add(-1) + routes.ServeHTTP(w, r) + })) g.server.Config.ReadTimeout = readTimeout g.server.Start() t.Cleanup(g.server.Close) @@ -212,7 +227,7 @@ func (l *syncLog) String() string { // whose haves come after a want the cache must fetch first, so upload-pack // reads them after the server's read timeout has passed. func TestGitRequestBodyOutlastsServerReadTimeout(t *testing.T) { - g := newGitTestSetup(t, 200*time.Millisecond) + g := newGitTestSetup(t, 200*time.Millisecond, 0) runGitCommand(t, g.work, "commit", "--quiet", "--allow-empty", "-m", "pull request") runGitCommand(t, g.work, "push", "--quiet", "origin", "HEAD:refs/pull/1/head") sha := runGitCommand(t, g.work, "rev-parse", "HEAD") @@ -237,6 +252,76 @@ func TestGitRequestBodyOutlastsServerReadTimeout(t *testing.T) { } } +// TestGitSlowRequestBodyStartsNothing sends part of a request body and then +// nothing. The request ends at the server's read timeout, without an +// upload-pack waiting for the rest. +func TestGitSlowRequestBodyStartsNothing(t *testing.T) { + g := newGitTestSetup(t, 200*time.Millisecond, 0) + runGitCommand(t, "", "clone", "--quiet", g.url(), filepath.Join(t.TempDir(), "clone")) + + conn, err := net.Dial("tcp", g.server.Listener.Addr().String()) + if err != nil { + t.Fatal(err) + } + defer func() { _ = conn.Close() }() + part := pktLine("command=ls-refs\n") + _, err = fmt.Fprintf(conn, "POST /git/up/repo/git-upload-pack HTTP/1.1\r\nHost: proxy\r\n"+ + "Content-Type: %s\r\nGit-Protocol: version=2\r\nContent-Length: 100\r\n\r\n%s", gitRequestType, part) + if err != nil { + t.Fatal(err) + } + _ = conn.SetReadDeadline(time.Now().Add(10 * time.Second)) + resp, err := http.ReadResponse(bufio.NewReader(conn), nil) + if err != nil { + t.Fatalf("no answer to a request whose body stopped: %v", err) + } + _ = resp.Body.Close() + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("status %d, want %d", resp.StatusCode, http.StatusBadRequest) + } +} + +// TestGitStalledClientIsDropped fetches a pack larger than the connection +// buffers and never reads it. The request ends once a write has waited +// writeStall, rather than holding upload-pack. +func TestGitStalledClientIsDropped(t *testing.T) { + g := newGitTestSetup(t, 0, 200*time.Millisecond) + data := make([]byte, 16<<20) + _, _ = rand.NewChaCha8([32]byte{}).Read(data) + if err := os.WriteFile(filepath.Join(g.work, "large"), data, 0o600); err != nil { + t.Fatal(err) + } + runGitCommand(t, g.work, "add", "large") + runGitCommand(t, g.work, "commit", "--quiet", "-m", "large") + runGitCommand(t, g.work, "push", "--quiet", "origin", "HEAD:main") + sha := runGitCommand(t, g.work, "rev-parse", "HEAD") + runGitCommand(t, "", "clone", "--quiet", g.url(), filepath.Join(t.TempDir(), "clone")) + + conn, err := net.Dial("tcp", g.server.Listener.Addr().String()) + if err != nil { + t.Fatal(err) + } + defer func() { _ = conn.Close() }() + body := pktLine("want "+sha+" side-band-64k\n") + "0000" + pktLine("done\n") + _, err = fmt.Fprintf(conn, "POST /git/up/repo/git-upload-pack HTTP/1.1\r\nHost: proxy\r\n"+ + "Content-Type: %s\r\nContent-Length: %d\r\n\r\n%s", gitRequestType, len(body), body) + if err != nil { + t.Fatal(err) + } + for deadline := time.Now().Add(10 * time.Second); g.active.Load() == 0; { + if time.Now().After(deadline) { + t.Fatal("request never reached the handler") + } + time.Sleep(10 * time.Millisecond) + } + for deadline := time.Now().Add(10 * time.Second); g.active.Load() != 0; { + if time.Now().After(deadline) { + t.Fatal("the handler still serves a client that stopped reading") + } + time.Sleep(10 * time.Millisecond) + } +} + func pktLine(payload string) string { return fmt.Sprintf("%04x%s", len(payload)+4, payload) } From 4d0d4a20ae15d955f976eb41f059f703b971235c Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Sun, 25 Oct 2026 09:41:05 +0000 Subject: [PATCH 2/2] Spill large Git request bodies into the cache's temporary directory --- internal/gitcache/serve.go | 2 +- internal/gitcache/serve_test.go | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/internal/gitcache/serve.go b/internal/gitcache/serve.go index 216edc12..bb5cfb02 100644 --- a/internal/gitcache/serve.go +++ b/internal/gitcache/serve.go @@ -221,7 +221,7 @@ func (c *Cache) ReadRequest(r *http.Request) (*Request, error) { req.data = buf.Bytes() return req, nil } - file, err := os.CreateTemp("", "git-upload-pack-*") + file, err := os.CreateTemp(c.tmpDir(), "git-upload-pack-*") if err != nil { return nil, err } diff --git a/internal/gitcache/serve_test.go b/internal/gitcache/serve_test.go index 50b8f752..ca53e400 100644 --- a/internal/gitcache/serve_test.go +++ b/internal/gitcache/serve_test.go @@ -246,7 +246,10 @@ func TestRefListingSlots(t *testing.T) { // TestReadRequest reads bodies kept in memory and, past maxMemoryRequest, in // a temporary file removed on Close. func TestReadRequest(t *testing.T) { - c := &Cache{maxRequestBody: 4 << 20} + c := &Cache{dir: t.TempDir(), maxRequestBody: 4 << 20} + if err := os.Mkdir(c.tmpDir(), dirPermissions); err != nil { + t.Fatal(err) + } for _, size := range []int{0, 100, maxMemoryRequest, 3 << 20} { body := strings.Repeat("x", size) r, _ := http.NewRequest(http.MethodPost, "/", strings.NewReader(body))