Re: [PATCH 2/4] receive-pack: implement advertising and receiving push options

On Sun, Jul 10, 2016 at 10:06 AM, Shawn Pearce [off-list ref] wrote:

On Fri, Jul 8, 2016 at 5:31 PM, Stefan Beller [off-list ref] wrote:

quoted

+
+       /* NEEDSWORK: expose the limitations to be configurable. */
+       int max_options = 32;
+
+       /*
+        * NEEDSWORK: expose the limitations to be configurable;
+        * Once the limit can be lifted, include a way for payloads
+        * larger than one pkt, e.g allow a payload of up to
+        * LARGE_PACKET_MAX - 1 only, and reserve the last byte
+        * to indicate whether the next pkt continues with this
+        * push option.
+        */
+       int max_size = 1024;

Instead of this, what about a new config variable
receive.maxCommandBytes[1] that places a limit on the number of bytes
of pkt-line data the client can supply in both the command list ("old
new ref"), push signature framing, and option list?

Including the whole command list is pretty smart as it actually tackles the
DoS problem as a whole. We shortly discussed having just one upper bound
limit for the push options alone, but we were distracted by the discussion
on whether to advertise this number or just reject it on the server side
after it filled up so much data.

The design here with two bounds was used to not care about the oversized
push options for now. (I mean we can still just reject larger push
options even when
having a receive.maxCommandBytes setting.)

Memory demands for the server are proportional to the data sent. A
simple byte limit lets the user make the decision about how this gets
used. Longer ref names or option values means fewer refs or options
can be sent. Shorter ref names or option values means more values or
options can be sent.

I studied a lot of repositories[2] and most use ref names under 200
bytes in length. A 3 MiB default for receive.maxCommandBytes gives
users something like 11,115 references in a single git push invocation
if they used all 200 bytes in every name. Most users don't have ref
names this long. Unlike a cap on each ref, it allows users to use the
full 65449 bytes in a reference name available in pkt-line, but you
can only send 48 such references. Likewise for options. :)

In an earlier discussion Jeff said roughly "either make it work well,
or don't make it work at all, i.e. why are git push options better
than a `git push .. && curl <server>/REST-API` thing?"

And by having this design we could punt on the corner cases with
transmitting arbitrary large push options/binaries for now and claim
it's another next step that needs to be done when adding the config
option for it. By having a single receive.maxCommandBytes setting
we would sweep that problem under the rug and people could wonder
why it fails with the large push option.

As said in an earlier email as a side note, we could think about introducing
a v2 pkt line format which starts with a variable int to indicate the packet
size, such that the payload is not bound up to 64k.

I think 3MiB is a bit much for everyday use though and not enough for
corner cases?

[1] I may propose this to JGit.
[2] More than 3M, but maybe Peff has access to more.