NullPointerException at TemporaryBuffer.last on an HTTP fetch that exceeds http.postBuffer with nothing to fetch

[lvh]

Reproduced on 7.7.1.202607240634-r.

Summary

A fetch over smart HTTP throws NullPointerException when two conditions hold at
once:

1. The request exceeded http.postBuffer (1 MiB by default), so JGit streamed it
   rather than buffering it.
2. The fetch needs no objects, so sendWants() returned false.

java.lang.NullPointerException: Cannot invoke "java.util.ArrayList.size()" because "this.blocks" is null
    at org.eclipse.jgit.util.TemporaryBuffer.last(TemporaryBuffer.java:367)
    at org.eclipse.jgit.util.TemporaryBuffer.write(TemporaryBuffer.java:117)
    at org.eclipse.jgit.util.io.TimeoutOutputStream.write(TimeoutOutputStream.java:89)
    at org.eclipse.jgit.transport.PacketLineOut.end(PacketLineOut.java:208)
    at org.eclipse.jgit.transport.BasePackConnection.close(BasePackConnection.java:656)
    at org.eclipse.jgit.transport.BasePackFetchConnection.close(BasePackFetchConnection.java:665)
    at org.eclipse.jgit.transport.FetchProcess.closeConnection(FetchProcess.java:327)
    at org.eclipse.jgit.transport.FetchProcess.executeImp(FetchProcess.java:226)
    at org.eclipse.jgit.transport.FetchProcess.execute(FetchProcess.java:105)
    at org.eclipse.jgit.transport.Transport.fetch(Transport.java:1459)

Mechanism

TransportHttp writes each request into a TemporaryBuffer sized by
http.postBuffer. Past that limit, switchToOverflow() sets blocks to null and
sends subsequent writes to the overflow stream. close() closes the overflow
stream and sets it to null, leaving both fields null.

BasePackConnection.init() sets outNeedsEnd. doFetch and doFetchV2 clear it
after sendWants() returns true. When sendWants() returns false, doFetch
returns early with outNeedsEnd still set, and close() calls pckOut.end().
That 4-byte write finds overflow == null, takes the in-core path, and calls
last() on a null blocks.

Under protocol v2 the request that overflows is the ls-refs request that opens a
fetch. JGit derives four ref-prefix lines from each refspec, about 154 bytes per
refspec, so the refspec count determines whether the buffer overflows, independent
of repository size. The threshold is near 6,800 refspecs. The reproduction leaves
protocol.version unset and the server logs version=2 for every request.

A client that builds refspecs by diffing remote refs against local refs reaches
that count on repositories with tens of thousands of refs/pull/* refs. The
exception is thrown before any ref is updated, so the following run computes the
same refspecs and fails the same way.

Reproduction

Everything below is at
https://gist.github.com/lvh/6bce39bf5a75d308c8d6acdcf4cb84c2, where sh run.sh
downloads JGit from Maven Central, builds the repository and runs the fetches.

Three files, requiring a JDK, Python 3 and git:

• make-repo.py builds a bare repository with 10,000 refs, each at its own root
  commit.
• smart_http.py serves it by wrapping git http-backend as CGI, and logs the
  framing, body size and ref-prefix count of each request. An HTTP remote is
  required: file:// and git:// write the request directly to the stream.
• FetchOverflowNpe.java runs the fetches, in source-file mode.

python3 make-repo.py /tmp/serve/synth.git 10000
python3 smart_http.py /tmp/serve 8181 &
java -cp org.eclipse.jgit-7.7.1.202607240634-r.jar:slf4j-api-2.0.18.jar:JavaEWAH-1.2.3.jar:commons-codec-1.22.0.jar \
     FetchOverflowNpe.java http://127.0.0.1:8181/synth.git /tmp/jgit-npe

Every scenario uses the same repository and server. The refspec count,
http.postBuffer, and whether the refs are already local vary. Body sizes and
framing come from the server log.

fetch	ls-refs body	framing	outcome
10,000 refspecs, nothing local yet	1,540,117 B	chunked	OK, 0 to 10,000 refs
6,000 refspecs, all already local	924,098 B	Content-Length, gzip	OK
8,000 refspecs, all already local	1,232,098 B	chunked	NullPointerException
10,000 refspecs, all already local	1,540,098 B	chunked	NullPointerException
10,000 refspecs, all already local, http.postBuffer 64 MiB	1,540,098 B	Content-Length, gzip	OK
10,000 refspecs, all already local, in batches of 500	77,106 B each	Content-Length, gzip	OK

Row 1 isolates condition 2: the same 1.5 MB chunked request succeeds when the
fetch has objects to ask for, and is followed by a want list of 10,000 wants. Row
5 isolates condition 1: the same fetch succeeds when the request fits in the
buffer. Rows 2 and 3 bracket the threshold. The overflow decision uses the
uncompressed size, 1,540,098 B in row 5, where the buffered request is 121,170 B
on the wire after gzip.

Minimal case

TemporaryBufferNpe.java drives a TemporaryBuffer subclass directly: overflow
it, close it, write once more. It prints the two field values at each step.

JGit 7.7.1.202607240634-r
fresh:          blocks=present overflow=null
after overflow: blocks=null overflow=present
after close:    blocks=null overflow=null
RESULT: NullPointerException: Cannot invoke "java.util.ArrayList.size()" because "this.blocks" is null
        at org.eclipse.jgit.util.TemporaryBuffer.last(TemporaryBuffer.java:367)

Relationship to #239

#239 reported this NullPointerException from the symptom side. Commit 0f88fca
added a destroyed flag and checkDestroyed() to TemporaryBuffer, released in
7.6.0. Those guards cover use after destroy(). On this path destroy() is never
called, close() leaves destroyed false, so checkDestroyed() passes and
write() reaches last() with blocks null. Both reproductions above run
against 7.7.1, which contains that commit.

An IOException at that point would still fail the fetch. The remaining write is
a flush packet for a request that was never opened.

ref-prefix expansion

ls-refs applies search-path expansion to refspec sources that are already fully
qualified. For +refs/synth/000004:refs/synth/000004, JGit sends four prefixes:

ref-prefix refs/tags/refs/synth/000004
ref-prefix refs/refs/synth/000004
ref-prefix refs/heads/refs/synth/000004
ref-prefix refs/synth/000004

DUMP_PREFIXES_FOR=refs/synth/000004 python3 smart_http.py /tmp/serve 8181 logs
these lines per request.

The first three match no ref. Skipping the expansion for sources that already
begin with refs/ reduces the request to a quarter of its size, moving the
threshold from about 6,800 refspecs to about 27,000, with no change to which refs
a fetch can find. This is separate from the NullPointerException and raises the
refspec count needed to reach it.

Suggested fixes

1. Clear outNeedsEnd on the early return in doFetch and doFetchV2 when
   sendWants() returns false, or track that the request stream is closed, so
   close() writes no flush packet for a request that was never opened.
2. Extend checkDestroyed() to cover close-after-overflow, so a write to a closed
   TemporaryBuffer raises IOException. This changes the diagnosis only.
3. Skip search-path expansion for fully qualified refspec sources.

Workaround in use

Splitting the refspecs across several FetchCommand calls keeps each request
under the buffer. At 500 refspecs the ls-refs request is 77 kB. The cost is
request count: about 12 s across 20 batches, against about 2.5 s for the same
fetch as one request that fits the buffer. It requires no server or configuration
change.