From: Junio C Hamano <hidden> Date: 2016-06-15 22:51:14
Johan Herland [off-list ref] writes:
The new receive.denyObjectLimit config variable defines an upper limit on the
number of objects to accept in a single push. If the number of objects in a
push exceeds this limit, the entire push is immediately aborted without
storing the pushed objects on the server at all.
Where does the error message go? Can clients pushing over various
transports receive the reason without your server consuming the data from
them? Don't you want to "receive-in-core-and-discard" instead?
For the purpose of "preventing an accidental push", I suspect that people
would expect you to limit either by number of commits (i.e. depth of
history) or by the total size of the data being transferred.
The name "objectlimit" sounds as if you are doing the latter and we can
use "200MB" there, but you are only limiting by count, so it is somewhat
misleading. We would want to see "count" or "number" somewhere in its
name.
From: Johan Herland <hidden> Date: 2016-06-15 22:51:14
On Friday 13 May 2011, Junio C Hamano wrote:
Johan Herland [off-list ref] writes:
quoted
The new receive.denyObjectLimit config variable defines an upper limit
on the number of objects to accept in a single push. If the number of
objects in a push exceeds this limit, the entire push is immediately
aborted without storing the pushed objects on the server at all.
Where does the error message go? Can clients pushing over various
transports receive the reason without your server consuming the data from
them? Don't you want to "receive-in-core-and-discard" instead?
Yes. Will be fixed in the re-roll.
For the purpose of "preventing an accidental push", I suspect that people
would expect you to limit either by number of commits (i.e. depth of
history) or by the total size of the data being transferred.
Yes, I agree that limiting by #commits, or by pack size would be more
intuitive. However, neither of those values are available to me at the point
where I have to decide what to do with the pack data (only the pack header
is available, and that only contains the object count).
The name "objectlimit" sounds as if you are doing the latter and we can
use "200MB" there, but you are only limiting by count, so it is somewhat
misleading. We would want to see "count" or "number" somewhere in its
name.
Agreed. Will be renamed to receive.objectCountLimit in the re-roll.
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Johan Herland <hidden> Date: 2016-06-15 22:51:14
The new receive.objectCountLimit config variable defines an upper limit
on the number of objects to accept in a single push. If the number of
objects in a push exceeds this limit, the entire push is discarded
without storing the pushed objects on the server at all.
This is meant to prevent an unintended large push (typically a result
of the user not being aware of exactly what is being pushed,
e.g. pushing a large rewritten history) from entering the repo.
Usually, this kind of limit could be imposed by a pre-receive or update
hook, but both of those run _after_ the pack has been received and
stored by receive-pack, so they cannot prevent the pack from being
stored on the server.
Documentation and tests are included.
Improved-by: Junio C Hamano [off-list ref]
Signed-off-by: Johan Herland <redacted>
---
Here is the re-roll with "receive-in-core-and-discard", plus the config
variable rename.
Have fun! :)
...Johan
Documentation/config.txt | 9 +++++++++
builtin/receive-pack.c | 14 +++++++++++++-
t/t5400-send-pack.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 66 insertions(+), 1 deletions(-)
@@ -1591,6 +1591,15 @@ receive.unpackLimit:: especially on slow filesystems. If not set, the value of `transfer.unpackLimit` is used instead.+receive.objectCountLimit::+ If the number of objects received in a push exceeds this limit,+ then the entire push will be refused. This is meant to prevent+ an unintended large push (typically a result of the user not+ being aware of exactly what is being pushed, e.g. pushing a+ large rewritten history) from entering the repo. If not set,+ there is no upper limit on the number of objects transferred+ in a single push.+ receive.denyDeletes:: If set to true, git-receive-pack will deny a ref update that deletes the ref. Use this to prevent such a ref deletion via a push.
@@ -222,4 +222,48 @@ test_expect_success 'deny pushing to delete current branch' ')'+test_expect_success'deny pushing when receive.objectCountLimit is exceeded''+rewound_push_setup&&+(+cdparent&&+gitconfigreceive.objectCountLimit1+)&&+(+cdchild&&+gitreset--hardorigin/master&&+echothree>file&&gitcommit-a-mthree&&+test_must_failgitsend-pack../parentmaster2>errs&&+grep-q"receive\\.objectCountLimit"errs+)&&+parent_head=$(cdparent&&gitrev-parse--verifymaster)&&+child_head=$(cdchild&&gitrev-parse--verifymaster)&&+test"$parent_head"!="$child_head"+'++test_expect_success'repeated push failure proves that objects were not stored remotely''+(+cdchild&&+test_must_failgitsend-pack../parentmaster2>errs&&+grep-q"receive\\.objectCountLimit"errs+)&&+parent_head=$(cdparent&&gitrev-parse--verifymaster)&&+child_head=$(cdchild&&gitrev-parse--verifymaster)&&+test"$parent_head"!="$child_head"+'++test_expect_success'increasing receive.objectCountLimit allows the push''+(+cdparent&&+gitconfigreceive.objectCountLimit10+)&&+(+cdchild&&+gitsend-pack../parentmaster2>errs&&+test_must_failgrep-q"receive\\.objectCountLimit"errs+)&&+parent_head=$(cdparent&&gitrev-parse--verifymaster)&&+child_head=$(cdchild&&gitrev-parse--verifymaster)&&+test"$parent_head"="$child_head"+'+ test_done
On Fri, May 13, 2011 at 19:03, Johan Herland [off-list ref] wrote:
The new receive.objectCountLimit config variable defines an upper limit
on the number of objects to accept in a single push. If the number of
objects in a push exceeds this limit, the entire push is discarded
without storing the pushed objects on the server at all.
Discarding in core is painful. We are still consuming bandwidth to
toss away the data.
I wonder... should we instead export the objectCountLimit as part of
the advertisement to the client, and teach send-pack to look at this
and pass it down to pack-objects? If pack-objects winds up with more
than this limit, it aborts and the client doesn't even transmit data.
Newer clients would abort cleanly with a nice error.
I don't see it as a problem to advertise to a client "This server will
only accept X objects from you, sending X + 1 is an error and will be
rejected." If we are worried about an evil client using this
advertisement to try and DoS a server... he can easily do that with a
single giant blob. Or a very long delta chain of a single blob and
many, many tiny deltas applied onto it. Knowing what the remote's
objectCountLimit is doesn't increase the risk of a DoS attack.
For older clients that don't know this new advertised capability, they
should fail hard and not transfer all of this data. In my experience
when a user gets these strange errors from his Git client, he contacts
his server administrator with the screen output. At which point the
administrator can see the Counting objects line, check the repository
configuration, and tell the user what the problem is... and encourage
them to upgrade their client to a newer version.
If we are going to put limits in, does it make sense to try and push
these limits back to pack-objects in a more detailed way? You talked
about depth of history, or size of pack. pack-objects could
approximate both. If its depth of history, it might even be able to
cut off before it enumerates too many commits. :-)
The remote side obviously cannot abort early with number of commits or
pack size limits, but if those were soft limits suggested to a client,
while the object count was a hard limit, you might get a better
approximation for what you want. A server administrator might
configure a soft limit of 10 commits, but a hard limit of 5,000
objects. For most users, a bad push would abort very early on the soft
limit of 10 commits if they did an incorrect rebase. Meanwhile a user
who made 1 commit but changed every GPL header in the linux-2.6
repository (26,000+ files) would also be stopped for exceeding the
(hard) 5000 object limit.
--
Shawn.
From: Johan Herland <hidden> Date: 2016-06-15 22:51:14
On Saturday 14 May 2011, Shawn Pearce wrote:
I wonder... should we instead export the objectCountLimit as part of
the advertisement to the client, and teach send-pack to look at this
and pass it down to pack-objects? If pack-objects winds up with more
than this limit, it aborts and the client doesn't even transmit data.
Newer clients would abort cleanly with a nice error.
Good idea (although it grows the scope from the quick-fix I initially
intended it to be...)
I'm planning to add a new capability collection/namespace, called "limit-*",
where the server can communicate capabilities to the client, like so:
limit-object-count_100000
limit-commit-count_1000
limit-pack-size_500000000
(I'd prefer to s/_/=/ or s/_/:/, but according to pack-protocol.txt, a
capability may not contain "=" or ":")
However, you say:
For older clients that don't know this new advertised capability, they
should fail hard and not transfer all of this data.
AFAICS this is not the case. If a client does not understand a capability,
it simply ignores it, and carries on doing its usual thing.
IINM there are only two ways to prevent an older client from transferring
all the data:
1. Change the pack protocol in an incompatible way, that causes older client
to abort with a pack format error prior to transmitting the pack.
2. (as in initial patch) Abort receive-pack when the server detects a limit
violation, leaving the client with a broken pipe. I haven't read the pack
protocol closely, but I wouldn't be surprised if this behavior is strictly
in violation of the protocol.
In my experience
when a user gets these strange errors from his Git client, he contacts
his server administrator with the screen output. At which point the
administrator can see the Counting objects line, check the repository
configuration, and tell the user what the problem is... and encourage
them to upgrade their client to a newer version.
Hmm... Not ideal, but I guess we can live with that. At least we should warn
the server administrator of this in the documentation of the config
variable(s).
Otherwise, I agree with everything you wrote.
Have fun! :)
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
On Sat, May 14, 2011 at 06:17, Johan Herland [off-list ref] wrote:
I'm planning to add a new capability collection/namespace, called "limit-*",
where the server can communicate capabilities to the client, like so:
limit-object-count_100000
limit-commit-count_1000
limit-pack-size_500000000
(I'd prefer to s/_/=/ or s/_/:/, but according to pack-protocol.txt, a
capability may not contain "=" or ":")
I forget why = and : are forbidden here. I think its just because we
wanted the options to be "simple". I agree, I would prefer = here too,
and probably would have written the patch that way myself. There
shouldn't be a technical reason why = isn't allowed here. Its just
documented as being not a good idea because at one time someone wrote
that down.
However, you say:
quoted
For older clients that don't know this new advertised capability, they
should fail hard and not transfer all of this data.
AFAICS this is not the case. If a client does not understand a capability,
it simply ignores it, and carries on doing its usual thing.
By this I meant #2 below (the initial patch).
IINM there are only two ways to prevent an older client from transferring
all the data:
1. Change the pack protocol in an incompatible way, that causes older client
to abort with a pack format error prior to transmitting the pack.
This is not a good idea. We still want the client to be able to talk
to the server if it would be within the limits.
2. (as in initial patch) Abort receive-pack when the server detects a limit
violation, leaving the client with a broken pipe. I haven't read the pack
protocol closely, but I wouldn't be surprised if this behavior is strictly
in violation of the protocol.
It is a violation of the protocol... sort of. Its allowed for the
server to up and die in the middle of a push. What happens if the
remote system loses power due to a grid failure while you are writing
to it? The remote system can't tell you "I'm going away now" first. It
just freezes and stops ACK'ing the TCP packets. Or if the remote
system gets overloaded and the Linux OOM killer kicks in... the remote
might get one of the processes elected for killing, and your TCP
connection breaks.
I don't think its as bad as it sounds. Its not a great user
experience, sure. And we maybe should also look at changing the
send-pack code to check the pipe for received data from the remote
peer if pack-objects dies (today it doesn't)... just in case the
reason pack-objects died is because an error message was written and
then the stream was closed.
--
Shawn.
From: Johan Herland <hidden> Date: 2016-06-15 22:51:14
On Sunday 15 May 2011, Shawn Pearce wrote:
On Sat, May 14, 2011 at 06:17, Johan Herland [off-list ref] wrote:
quoted
I'm planning to add a new capability collection/namespace, called
"limit-*", where the server can communicate capabilities to the
client, like so:
limit-object-count_100000
limit-commit-count_1000
limit-pack-size_500000000
(I'd prefer to s/_/=/ or s/_/:/, but according to pack-protocol.txt, a
capability may not contain "=" or ":")
I forget why = and : are forbidden here. I think its just because we
wanted the options to be "simple". I agree, I would prefer = here too,
and probably would have written the patch that way myself. There
shouldn't be a technical reason why = isn't allowed here. Its just
documented as being not a good idea because at one time someone wrote
that down.
Ok. I'll use '='.
quoted
However, you say:
quoted
For older clients that don't know this new advertised capability, they
should fail hard and not transfer all of this data.
AFAICS this is not the case. If a client does not understand a
capability, it simply ignores it, and carries on doing its usual
thing.
By this I meant #2 below (the initial patch).
Ah, I see. Will use that in the re-roll.
I don't think its as bad as it sounds. Its not a great user
experience, sure. And we maybe should also look at changing the
send-pack code to check the pipe for received data from the remote
peer if pack-objects dies (today it doesn't)... just in case the
reason pack-objects died is because an error message was written and
then the stream was closed.
This will be in the re-roll.
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
@@ -391,7 +391,8 @@ The reference discovery phase is done nearly the same way as it is in the fetching protocol. Each reference obj-id and name on the server is sent in packet-line format to the client, followed by a flush-pkt. The only real difference is that the capability listing is different - the only-possible values are 'report-status', 'delete-refs' and 'ofs-delta'.+possible values are 'report-status', 'delete-refs', 'side-band-64k' and+'ofs-delta'. Reference Update Request and Packfile Transfer ----------------------------------------------
@@ -21,8 +21,8 @@ NOT advertise capabilities it does not understand. The 'report-status' and 'delete-refs' capabilities are sent and recognized by the receive-pack (push to server) process.-The 'ofs-delta' capability is sent and recognized by both upload-pack-and receive-pack protocols.+The 'side-band-64k' and 'ofs-delta' capabilities are sent and+recognized by both upload-pack and receive-pack protocols. All other capabilities are only recognized by the upload-pack (fetch from server) process.
From: Johan Herland <hidden> Date: 2016-06-15 22:51:14
Here's the next iteration of what started out as a simple patch to let
the server refuse pushes that exceeded a configurable limit on the
#objects in a pack.
The current patch series allows limiting pushes by
- size of pack
- #objects in pack
- #commits in pack
The limits are controlled by corresponding (new) config variables:
- receive.packSizeLimit
- receive.objectCountLimit
- receive.commitCountLimit
Setting one or more of these config variables causes receive-pack to
advertise the corresponding (new) capabilities:
- limit-pack-size=<num>
- limit-object-count=<num>
- limit-commit-count=<num>
These capabilities are parsed by the send-pack client, which pass them
on to pack-objects, using the corresponding (mostly new) pack-objects
options:
--max-pack-size=<num> (extended to be usable in this context)
--max-object-count=<num> (new)
--max-commit-count=<num> (new)
When one or more of those options are given together with --stdout to
pack-objects, pack-objects will check the generated pack against those
limits, and abort the pack generation if any limit is exceeded.
In addition, the server will also verify the object count limit,
if enabled, and abort the push if the pack exceeds the limit.
Currently, the server cannot easily check the other two limits without
changing the pack format (left as an exercise to the reader... ;-]),
so exceeding the pack size limit or the commit count limit will not be
caught by the server.
Finally, a quick run-through of the patches:
- #1 is a very minor fix to the pack protocol docs.
- #2 attempts to retrieve and display the remote status, even when
pack-objects fail. This patch touches the same code as the recent
send-pack deadlock fixes, so I'd like Peff or JSixt to review them.
- #3 - #5 teaches pack-objects some new options to impose the above
limits on the generated pack.
- #6 contains some general preparation for the "limit-*" capabilities.
- #7 - #9 adds the new limits to receive-pack, send-pack, and the
corresponding protocol capabilites.
Have fun! :)
...Johan
Johan Herland (9):
Update technical docs to reflect side-band-64k capability in receive-pack
send-pack: Attempt to retrieve remote status even if pack-objects fails
pack-objects: Allow --max-pack-size to be used together with --stdout
pack-objects: Teach new option --max-object-count, similar to --max-pack-size
pack-objects: Teach new option --max-commit-count, limiting #commits in pack
receive-pack: Prepare for addition of the new 'limit-*' family of capabilities
send-pack/receive-pack: Allow server to refuse pushes with too many objects
send-pack/receive-pack: Allow server to refuse pushing too large packs
send-pack/receive-pack: Allow server to refuse pushes with too many commits
Documentation/config.txt | 35 ++++
Documentation/git-pack-objects.txt | 20 +++
Documentation/git-repack.txt | 6 +
Documentation/technical/pack-protocol.txt | 5 +-
Documentation/technical/protocol-capabilities.txt | 30 +++-
builtin/pack-objects.c | 53 +++++-
builtin/receive-pack.c | 45 +++++-
builtin/send-pack.c | 44 ++++--
cache.h | 2 +-
connect.c | 7 +-
git-repack.sh | 27 ++--
send-pack.h | 3 +
t/t5300-pack-object.sh | 121 +++++++++++++
t/t5400-send-pack.sh | 191 +++++++++++++++++++++
14 files changed, 543 insertions(+), 46 deletions(-)
--
1.7.5.rc1.3.g4d7b
From: Johan Herland <hidden> Date: 2016-06-15 22:51:14
When pushing, send-pack uses pack-objects to write the pack data to the
receive-pack process running on the remote end. The scenarios where
pack-objects dies unexpectedly, can be roughly divided based whether the
reason for the failure is _local_ (i.e. something in pack-objects caused
it to fail of its own accord), or _remote_ (i.e. something in the remote
receive-pack process caused it to fail, leaving the local pack-objects
process with a broken pipe)
If the reason for the failure is local, we expect pack-objects to report
an appropriate error message to the user.
However, if the reason for the failure is remote, pack-objects will merely
abort because of the broken pipe, and the user is left with no clue as to
the reason why the remote receive-pack process died.
In certain cases, though, the receive-pack process on the other end may have
produced an error message immediately before exiting. This error message may
be currently waiting to be read by the local send-pack process.
Therefore, we should try to read from the remote end, even when pack-objects
dies unexepectedly. We accomplish this by _always_ calling receive_status()
after pack_objects(). If the remote end managed to produce a well-formed
status report before exiting, then receive_status() simply presents that to
the user. Even if the data from the remote end cannot be understood by
receive_status(), it will print that data as part of its error message. In
any case, we give the user as much information about the failure as possible.
Signed-off-by: Johan Herland <redacted>
---
I first wrote this patch on a base where e07fd15 (Peff's "send-pack:
unbreak push over stateless rpc") was not present, and then resolved
a conflict when rebasing this patch onto current master. I hope Peff
or Johannes (Sixt) can verify that my patch does not reintroduce the
deadlock they fixed.
...Johan
builtin/send-pack.c | 18 +++++-------------
1 files changed, 5 insertions(+), 13 deletions(-)
@@ -251,7 +251,7 @@ int send_pack(struct send_pack_args *args,intstatus_report=0;intuse_sideband=0;unsignedcmds_sent=0;-intret;+intret=0;structasyncdemux;/* Does the other end support the reporting? */
@@ -339,23 +339,15 @@ int send_pack(struct send_pack_args *args,}if(new_refs&&cmds_sent){-if(pack_objects(out,remote_refs,extra_have,args)<0){-for(ref=remote_refs;ref;ref=ref->next)-ref->status=REF_STATUS_NONE;-if(args->stateless_rpc)-close(out);-if(use_sideband)-finish_async(&demux);-return-1;-}+ret=pack_objects(out,remote_refs,extra_have,args);+if(ret&&args->stateless_rpc)+close(out);}if(args->stateless_rpc&&cmds_sent)packet_flush(out);if(status_report&&cmds_sent)-ret=receive_status(in,remote_refs);-else-ret=0;+ret|=receive_status(in,remote_refs);if(args->stateless_rpc)packet_flush(out);
From: Johan Herland <hidden> Date: 2016-06-15 22:51:14
Currently we refuse combining --max-pack-size with --stdout since there's
no way to make multiple packs when the pack is written to stdout. However,
we want to be able to limit the maximum size of the pack created by
--stdout (and abort pack-objects if we are unable to meet that limit).
Therefore, when used together with --stdout, we reinterpret --max-pack-size
to indicate the maximum pack size which - if exceeded - will cause
pack-objects to abort with an error message.
Signed-off-by: Johan Herland <redacted>
---
Documentation/git-pack-objects.txt | 3 ++
builtin/pack-objects.c | 9 ++++---
t/t5300-pack-object.sh | 43 ++++++++++++++++++++++++++++++++++++
3 files changed, 51 insertions(+), 4 deletions(-)
@@ -112,6 +112,9 @@ base-name:: If specified, multiple packfiles may be created. The default is unlimited, unless the config variable `pack.packSizeLimit` is set.+++When used together with --stdout, the command will fail with an error+message if the pack output exceeds the given limit. --honor-pack-keep:: This flag causes an object already in a local pack that
@@ -229,7 +229,7 @@ static unsigned long write_object(struct sha1file *f,if(!entry->delta)usable_delta=0;/* no delta */-elseif(!pack_size_limit)+elseif(!pack_size_limit||pack_to_stdout)usable_delta=1;/* unlimited packfile */elseif(entry->delta->idx.offset==(off_t)-1)usable_delta=0;/* base was written to another pack */
@@ -478,6 +478,9 @@ static void write_pack_file(void)*Ifso,rewriteitlikeinfast-import*/if(pack_to_stdout){+if(nr_written!=nr_remaining)+die("unable to make pack within the pack size"+" limit (%lu bytes)",pack_size_limit);sha1close(f,sha1,CSUM_CLOSE);}elseif(nr_written==nr_remaining){sha1close(f,sha1,CSUM_FSYNC);
@@ -2315,9 +2318,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)if(!pack_to_stdout&&!pack_size_limit)pack_size_limit=pack_size_limit_cfg;-if(pack_to_stdout&&pack_size_limit)-die("--max-pack-size cannot be used to build a pack for transfer.");-if(pack_size_limit&&pack_size_limit<1024*1024){+if(!pack_to_stdout&&pack_size_limit&&pack_size_limit<1024*1024){warning("minimum pack size limit is 1 MiB");pack_size_limit=1024*1024;}
From: Johan Herland <hidden> Date: 2016-06-15 22:51:14
The new --max-object-count option behaves similarly to --max-pack-size,
except that the decision to split packs is determined by the number of
objects in the pack, and not by the size of the pack.
The new option also has a corresponding configuration variable, named
pack.objectCountLimit, which works similarly to pack.packSizeLimit,
subject to the difference mentioned above.
As with --max-pack-size, you can use --max-object-count together with
--stdout to put a limit on the number of objects in the pack written to
stdout. If the pack would exceed this limit, pack-objects will abort with
an error message.
Finally, for completeness, the new option is also added to git-repack,
which simply forwards it to pack-objects
Documentation and tests are included.
Signed-off-by: Johan Herland <redacted>
---
Documentation/config.txt | 8 ++++++
Documentation/git-pack-objects.txt | 9 +++++++
Documentation/git-repack.txt | 6 +++++
builtin/pack-objects.c | 20 ++++++++++++++++
git-repack.sh | 27 +++++++++++----------
t/t5300-pack-object.sh | 44 ++++++++++++++++++++++++++++++++++++
6 files changed, 101 insertions(+), 13 deletions(-)
@@ -1523,6 +1523,14 @@ pack.packSizeLimit:: Common unit suffixes of 'k', 'm', or 'g' are supported.+pack.objectCountLimit::+ The maximum number of objects in a pack. This setting only+ affects packing to a file when repacking, i.e. the git://+ protocol is unaffected. It can be overridden by the+ `\--max-object-count` option of linkgit:git-repack[1].+ The default is unlimited. Common unit suffixes of 'k', 'm',+ or 'g' are supported.+ pager.<cmd>:: If the value is boolean, turns on or off pagination of the output of a particular git subcommand when writing to a tty.
@@ -116,6 +116,15 @@ base-name:: When used together with --stdout, the command will fail with an error message if the pack output exceeds the given limit.+--max-object-count=<n>::+ Maximum number of objects in each output pack file. The number+ can be suffixed with "k", "m", or "g". If specified, multiple+ packfiles may be created. The default is unlimited, unless+ the config variable `pack.objectCountLimit` is set.+++When used together with --stdout, the command will fail with an error+message if the pack output exceeds the given limit.+ --honor-pack-keep:: This flag causes an object already in a local pack that has a .keep file to be ignored, even if it would have
@@ -109,6 +109,12 @@ other objects in that pack they already have locally. The default is unlimited, unless the config variable `pack.packSizeLimit` is set.+--max-object-count=<n>::+ Maximum number of objects in each output pack file. The number+ can be suffixed with "k", "m", or "g". If specified, multiple+ packfiles may be created. The default is unlimited, unless+ the config variable `pack.objectCountLimit` is set.+ Configuration -------------
@@ -227,6 +228,10 @@ static unsigned long write_object(struct sha1file *f,elselimit=pack_size_limit-write_offset;+/* Trigger new pack when we reach object count limit */+if(object_count_limit&&nr_written>=object_count_limit)+return0;+if(!entry->delta)usable_delta=0;/* no delta */elseif(!pack_size_limit||pack_to_stdout)
@@ -2322,6 +2337,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)warning("minimum pack size limit is 1 MiB");pack_size_limit=1024*1024;}+if(!pack_to_stdout&&!object_count_limit)+object_count_limit=object_count_limit_cfg;if(!pack_to_stdout&&thin)die("--thin cannot be used to build an indexable pack.");
@@ -2349,6 +2366,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)if(non_empty&&!nr_result)return0;+if(pack_to_stdout&&object_count_limit&&object_count_limit<nr_result)+die("unable to make pack within the object count limit"+" (%lu objects)",object_count_limit);if(nr_result)prepare_pack(window,depth);write_pack_file();
From: Johan Herland <hidden> Date: 2016-06-15 22:51:14
The new --max-commit-count option behaves similarly to --max-object-count,
when used together with --stdout: It limits the number of commits in the
pack written to stdout. If the pack would exceed this limit, pack-objects
will abort with an error message.
Unlike --max-pack-size and --max-object-count, --max-commit-count must
always be used together with --stdout. This is because using the commit
count to split packs is not at all a good heuristic, since Git does not
necessarily distribute commit objects uniformly across packs.
Documentation and tests are included.
Signed-off-by: Johan Herland <redacted>
---
Documentation/git-pack-objects.txt | 8 ++++++++
builtin/pack-objects.c | 24 +++++++++++++++++++++---
t/t5300-pack-object.sh | 34 ++++++++++++++++++++++++++++++++++
3 files changed, 63 insertions(+), 3 deletions(-)
@@ -125,6 +125,14 @@ message if the pack output exceeds the given limit. When used together with --stdout, the command will fail with an error message if the pack output exceeds the given limit.+--max-commit-count=<n>::+ This option is only useful together with --stdout.+ Specifies the maximum number of commits allowed in the created+ pack. If the number of commits would exceed the given limit,+ pack-objects will fail with an error message.+ The number can be suffixed with "k", "m", or "g".+ The default is unlimited.+ --honor-pack-keep:: This flag causes an object already in a local pack that has a .keep file to be ignored, even if it would have
@@ -2340,6 +2352,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)if(!pack_to_stdout&&!object_count_limit)object_count_limit=object_count_limit_cfg;+if(!pack_to_stdout&&commit_count_limit)+die("--max-commit-count is only useful together with --stdout.");+if(!pack_to_stdout&&thin)die("--thin cannot be used to build an indexable pack.");
@@ -2369,6 +2384,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)if(pack_to_stdout&&object_count_limit&&object_count_limit<nr_result)die("unable to make pack within the object count limit"" (%lu objects)",object_count_limit);+if(pack_to_stdout&&commit_count_limit&&commit_count_limit<nr_commits)+die("unable to make pack within the commit count limit"+" (%lu commits)",commit_count_limit);if(nr_result)prepare_pack(window,depth);write_pack_file();
@@ -483,6 +483,40 @@ test_expect_success '--stdout fails when pack exceeds --max-object-count' 'grep-q"object count limit"errs'+test_expect_success'make a few more commits''+gitreset--hard$commit&&+echo"change">file&&+gitaddfile&&+gitcommit-msecond&&+commit2=`gitrev-parse--verifyHEAD`&&+echo"more change">>file&&+gitcommit-a-mthird&&+commit3=`gitrev-parse--verifyHEAD`&&+echo"even more change">>file&&+gitcommit-a-mfourth&&+commit4=`gitrev-parse--verifyHEAD`&&{+echo$commit&&+echo$commit2&&+echo$commit3&&+echo$commit4+}>>commit-list+'++test_expect_success'--stdout works with large enough --max-commit-count''+gitpack-objects--revs--stdout--max-commit-count=4<commit-list>test-22.pack&&+gitindex-pack--stricttest-22.pack+'++test_expect_success'verify resulting pack''+gitverify-packtest-22.pack+'++test_expect_success'--stdout fails when pack exceeds --max-commit-count''+test_must_failgitpack-objects--revs--stdout--max-commit-count=3<commit-list>test-23.pack2>errs&&+test_must_failgitindex-pack--stricttest-23.pack&&+grep-q"commit count limit"errs+'+## WARNING!#
From: Johan Herland <hidden> Date: 2016-06-15 22:51:14
Add a new receive.commitCountLimit config variable which defines an upper
limit on the number of commits to accept in a single push.
This limit is advertised to clients, using the new "limit-commit-count=<num>"
capability. The client side - aka. send-pack - parses this capability and
forwards it to pack-objects, using the recently added --max-commit-count
option. pack-objects then checks the generated pack against the limit and
aborts the pack generation if the pack would have too many objects.
However, older clients that do not understand the capability will not check
their pack against the limit, and will end up pushing the pack to the server.
Currently there is no extra check on the server to detect a push that exceeds
receive.commitCountLimit. However, such a check could be done in a pre-receive
or update hook.
Documentation and tests are included.
Signed-off-by: Johan Herland <redacted>
---
Documentation/config.txt | 9 +++
Documentation/technical/protocol-capabilities.txt | 1 +
builtin/receive-pack.c | 10 +++-
builtin/send-pack.c | 10 +++-
send-pack.h | 1 +
t/t5400-send-pack.sh | 63 +++++++++++++++++++++
6 files changed, 92 insertions(+), 2 deletions(-)
@@ -1617,6 +1617,15 @@ receive.objectCountLimit:: there is no upper limit on the number of objects transferred in a single push.+receive.commitCountLimit::+ If the number of commits received in a push exceeds this limit,+ then the entire push will be refused. This is meant to prevent+ an unintended large push (typically a result of the user not+ being aware of exactly what is being pushed, e.g. pushing a+ large rewritten history) from entering the repo. If not set,+ there is no upper limit on the number of commits transferred+ in a single push.+ receive.denyDeletes:: If set to true, git-receive-pack will deny a ref update that deletes the ref. Use this to prevent such a ref deletion via a push.
@@ -207,6 +207,7 @@ The following "limit-*" capabilites are recognized: - limit-pack-size=<num> (Maximum size (in bytes) of uploaded pack) - limit-object-count=<num> (Maximum number of objects in a pack)+ - limit-commit-count=<num> (Maximum number of commits in a pack) More "limit-*" capabilities may be added in the future. The client is free to ignore any "limit-*" capabilities it does not understand.
@@ -281,6 +287,8 @@ int send_pack(struct send_pack_args *args,args->max_pack_size=strtoul(p+16,NULL,10);if((p=server_supports("limit-object-count=")))args->max_object_count=strtoul(p+19,NULL,10);+if((p=server_supports("limit-commit-count=")))+args->max_commit_count=strtoul(p+19,NULL,10);if(!remote_refs){fprintf(stderr,"No refs in common and none specified; doing nothing.\n"
@@ -350,4 +350,67 @@ test_expect_success 'push is allowed when pack size is not exceeded' 'test"$parent_head"="$child_head"'+test_expect_success'verify that limit-commit-count capability is not advertised by default''+rewound_push_setup&&+(+cdparent&&+test_might_failgitreceive-pack.<../pkt-flush>output&&+test_must_failgrep-q"limit-commit-count"output+)+'++test_expect_success'verify that receive.commitCountLimit triggers limit-commit-count capability''+(+cdparent&&+gitconfigreceive.commitCountLimit1&&+test_might_failgitreceive-pack.<../pkt-flush>output&&+grep-q"limit-commit-count=1"output+)+'++test_expect_success'deny pushing when receive.commitCountLimit is exceeded''+(+cdchild&&+gitreset--hardorigin/master&&+echothree>file&&gitcommit-a-mthree&&+echofour>file&&gitcommit-a-mfour&&+test_must_failgitsend-pack../parentmaster2>errs&&+grep-q"commit count limit"errs+)&&+parent_head=$(cdparent&&gitrev-parse--verifymaster)&&+child_head=$(cdchild&&gitrev-parse--verifymaster)&&+test"$parent_head"!="$child_head"+'++test_expect_success'repeated push failure proves that objects were not stored remotely''+(+cdchild&&+test_must_failgitsend-pack../parentmaster2>errs&&+grep-q"commit count limit"errs+)&&+parent_head=$(cdparent&&gitrev-parse--verifymaster)&&+child_head=$(cdchild&&gitrev-parse--verifymaster)&&+test"$parent_head"!="$child_head"+'++test_expect_success'increase receive.commitCountLimit''+(+cdparent&&+gitconfigreceive.commitCountLimit2&&+test_might_failgitreceive-pack.<../pkt-flush>output&&+grep-q"limit-commit-count=2"output+)+'++test_expect_success'push is allowed when commit limit is not exceeded''+(+cdchild&&+gitsend-pack../parentmaster2>errs&&+test_must_failgrep-q"commit count limit"errs+)&&+parent_head=$(cdparent&&gitrev-parse--verifymaster)&&+child_head=$(cdchild&&gitrev-parse--verifymaster)&&+test"$parent_head"="$child_head"+'+ test_done
From: Johan Herland <hidden> Date: 2016-06-15 22:51:14
Add a new receive.objectCountLimit config variable which defines an upper
limit on the number of objects to accept in a single push.
This limit is advertised to clients, using the new "limit-object-count=<num>"
capability. The client side - aka. send-pack - parses this capability and
forwards it to pack-objects, using the recently added --max-object-count
option. pack-objects then checks the generated pack against the limit and
aborts the pack generation if the pack would have too many objects.
Additionally - for older clients that do not understand the capability - the
server aborts the transfer if the number of objects in the transferred pack
exceeds the limit. This is a suboptimal fallback solution, as it leaves the
client with a broken pipe, and likely a confused user.
Server administrators might want to use this config variable to prevent
unintended large pushes from entering the repo (typically a result of the
user not being aware of exactly what is being pushed, e.g. pushing a large
rewritten history). Note that this config variable is not intended to protect
against DoS attacks, since there are countless other ways to attempt to DoS a
server without violating this limit.
Traditionally, this kind of limit would be imposed by a pre-receive or update
hook, but both of those run _after_ the pack has been received and stored by
receive-pack, so they cannot prevent the pack from being stored on the server.
Documentation and tests are included.
Signed-off-by: Johan Herland <redacted>
---
Documentation/config.txt | 9 +++
Documentation/technical/protocol-capabilities.txt | 2 +
builtin/receive-pack.c | 13 ++++-
builtin/send-pack.c | 10 +++
send-pack.h | 1 +
t/t5400-send-pack.sh | 66 +++++++++++++++++++++
6 files changed, 100 insertions(+), 1 deletions(-)
@@ -1599,6 +1599,15 @@ receive.unpackLimit:: especially on slow filesystems. If not set, the value of `transfer.unpackLimit` is used instead.+receive.objectCountLimit::+ If the number of objects received in a push exceeds this limit,+ then the entire push will be refused. This is meant to prevent+ an unintended large push (typically a result of the user not+ being aware of exactly what is being pushed, e.g. pushing a+ large rewritten history) from entering the repo. If not set,+ there is no upper limit on the number of objects transferred+ in a single push.+ receive.denyDeletes:: If set to true, git-receive-pack will deny a ref update that deletes the ref. Use this to prevent such a ref deletion via a push.
@@ -205,5 +205,7 @@ the server. If the check fails, the client must abort the upload, and report the reason for the aborted push back to the user. The following "limit-*" capabilites are recognized:+ - limit-object-count=<num> (Maximum number of objects in a pack)+ More "limit-*" capabilities may be added in the future. The client is free to ignore any "limit-*" capabilities it does not understand.
@@ -253,6 +260,7 @@ int send_pack(struct send_pack_args *args,unsignedcmds_sent=0;intret=0;structasyncdemux;+constchar*p;/* Does the other end support the reporting? */if(server_supports("report-status"))
@@ -263,6 +271,8 @@ int send_pack(struct send_pack_args *args,args->use_ofs_delta=1;if(server_supports("side-band-64k"))use_sideband=1;+if((p=server_supports("limit-object-count=")))+args->max_object_count=strtoul(p+19,NULL,10);if(!remote_refs){fprintf(stderr,"No refs in common and none specified; doing nothing.\n"
@@ -222,4 +222,70 @@ test_expect_success 'deny pushing to delete current branch' ')'+echo"0000">pkt-flush++test_expect_success'verify that limit-object-count capability is not advertised by default''+rewound_push_setup&&+(+cdparent&&+test_might_failgitreceive-pack.<../pkt-flush>output&&+test_must_failgrep-q"limit-object-count"output+)+'++test_expect_success'verify that receive.objectCountLimit triggers limit-object-count capability''+(+cdparent&&+gitconfigreceive.objectCountLimit1&&+test_might_failgitreceive-pack.<../pkt-flush>output&&+grep-q"limit-object-count=1"output+)+'++test_expect_success'deny pushing when receive.objectCountLimit is exceeded''+(+cdchild&&+gitreset--hardorigin/master&&+echothree>file&&gitcommit-a-mthree&&+test_must_failgitsend-pack../parentmaster2>errs&&+grep-q"object count limit"errs+)&&+parent_head=$(cdparent&&gitrev-parse--verifymaster)&&+child_head=$(cdchild&&gitrev-parse--verifymaster)&&+test"$parent_head"!="$child_head"+'++test_expect_success'repeated push failure proves that objects were not stored remotely''+(+cdchild&&+test_must_failgitsend-pack../parentmaster2>errs&&+grep-q"object count limit"errs+)&&+parent_head=$(cdparent&&gitrev-parse--verifymaster)&&+child_head=$(cdchild&&gitrev-parse--verifymaster)&&+test"$parent_head"!="$child_head"+'++test_expect_success'increase receive.objectCountLimit''+(+cdparent&&+gitconfigreceive.objectCountLimit10&&+test_might_failgitreceive-pack.<../pkt-flush>output&&+grep-q"limit-object-count=10"output+)+'++test_expect_success'push is allowed when object limit is not exceeded''+(+cdchild&&+gitsend-pack../parentmaster2>errs&&+test_must_failgrep-q"object count limit"errs&&+# Also no error message from remote receive-pack+test_must_failgrep-q"receive\\.objectCountLimit"errs+)&&+parent_head=$(cdparent&&gitrev-parse--verifymaster)&&+child_head=$(cdchild&&gitrev-parse--verifymaster)&&+test"$parent_head"="$child_head"+'+ test_done
From: Johan Herland <hidden> Date: 2016-06-15 22:51:14
This adds some technical documentation on the 'limit-*' family of
capabilities that will be added in the following commits.
Also refactor the generation of the capabilities declaration in receive-pack.
This will also be further expanded in the following commits.
Finally, change the return type of server_supports() to allow the caller to
more closely examine the found capability, e.g. by calling
server_supports("limit-foo="), and then use the return value to parse the
value following the '='.
Signed-off-by: Johan Herland <redacted>
---
Documentation/technical/pack-protocol.txt | 6 ++--
Documentation/technical/protocol-capabilities.txt | 22 +++++++++++++++++++++
builtin/receive-pack.c | 16 +++++++++++---
cache.h | 2 +-
connect.c | 7 +++--
5 files changed, 42 insertions(+), 11 deletions(-)
@@ -391,8 +391,8 @@ The reference discovery phase is done nearly the same way as it is in the fetching protocol. Each reference obj-id and name on the server is sent in packet-line format to the client, followed by a flush-pkt. The only real difference is that the capability listing is different - the only-possible values are 'report-status', 'delete-refs', 'side-band-64k' and-'ofs-delta'.+possible values are 'report-status', 'delete-refs', 'side-band-64k',+'ofs-delta' and 'limit-*'. Reference Update Request and Packfile Transfer ----------------------------------------------
@@ -21,6 +21,9 @@ NOT advertise capabilities it does not understand. The 'report-status' and 'delete-refs' capabilities are sent and recognized by the receive-pack (push to server) process.+Any 'limit-*' capabilities may only be sent by the receive-pack+process. It is never requested by client.+ The 'side-band-64k' and 'ofs-delta' capabilities are sent and recognized by both upload-pack and receive-pack protocols.
@@ -185,3 +188,22 @@ it is capable of accepting a zero-id value as the target value of a reference update. It is not sent back by the client, it simply informs the client that it can be sent zero-id values to delete references.++limit-*+-------++If the server sends one or more capabilities that start with "limit-",+it means that there are certain limits to what kind of pack the server+will receive. More specifically, these capabilities must be of the form+"limit-<what>=<num>" where "<what>" (a sequence of lower-case letters,+digits and "-") describes which property of the pack is limited, and+"<num>" (a sequence of decimal digits) specifies the limit value.+Capabilities of this type are not sent back by the client; instead the+client must verify that the created packfile does not exceed the given+limits. This check should happen prior to transferring the packfile to+the server. If the check fails, the client must abort the upload, and+report the reason for the aborted push back to the user.+The following "limit-*" capabilites are recognized:++More "limit-*" capabilities may be added in the future. The client+is free to ignore any "limit-*" capabilities it does not understand.
From: Johan Herland <hidden> Date: 2016-06-15 22:51:14
Add a new receive.packSizeLimit config variable which defines an upper
limit on the pack size to accept in a single push.
This limit is advertised to clients, using the new "limit-pack-size=<num>"
capability. The client side - aka. send-pack - parses this capability and
forwards it to pack-objects, using the --max-pack-size option.
pack-objects then checks the generated pack against the limit and aborts
the pack transmission if the pack becomes too large.
However, older clients that do not understand the capability will not check
their pack against the limit, and will end up pushing the pack to the server.
Currently there is no extra check on the server to detect a push that exceeds
receive.packSizeLimit. However, such a check could be done in a pre-receive
or update hook.
Documentation and tests are included.
Signed-off-by: Johan Herland <redacted>
---
Documentation/config.txt | 9 +++
Documentation/technical/protocol-capabilities.txt | 1 +
builtin/receive-pack.c | 10 +++-
builtin/send-pack.c | 14 ++++-
send-pack.h | 1 +
t/t5400-send-pack.sh | 62 +++++++++++++++++++++
6 files changed, 93 insertions(+), 4 deletions(-)
@@ -1599,6 +1599,15 @@ receive.unpackLimit:: especially on slow filesystems. If not set, the value of `transfer.unpackLimit` is used instead.+receive.packSizeLimit::+ If the pack file transferred in a push exceeds this limit,+ then the entire push will be refused. This is meant to prevent+ an unintended large push (typically a result of the user not+ being aware of exactly what is being pushed, e.g. pushing a+ large rewritten history) from entering the repo. If not set,+ there is no upper limit on the size of the pack transferred+ in a single push.+ receive.objectCountLimit:: If the number of objects received in a push exceeds this limit, then the entire push will be refused. This is meant to prevent
@@ -205,6 +205,7 @@ the server. If the check fails, the client must abort the upload, and report the reason for the aborted push back to the user. The following "limit-*" capabilites are recognized:+ - limit-pack-size=<num> (Maximum size (in bytes) of uploaded pack) - limit-object-count=<num> (Maximum number of objects in a pack) More "limit-*" capabilities may be added in the future. The client
@@ -288,4 +288,66 @@ test_expect_success 'push is allowed when object limit is not exceeded' 'test"$parent_head"="$child_head"'+test_expect_success'verify that limit-pack-size capability is not advertised by default''+rewound_push_setup&&+(+cdparent&&+test_might_failgitreceive-pack.<../pkt-flush>output&&+test_must_failgrep-q"limit-pack-size"output+)+'++test_expect_success'verify that receive.packSizeLimit triggers limit-pack-size capability''+(+cdparent&&+gitconfigreceive.packSizeLimit10&&+test_might_failgitreceive-pack.<../pkt-flush>output&&+grep-q"limit-pack-size=10"output+)+'++test_expect_success'deny pushing when receive.packSizeLimit is exceeded''+(+cdchild&&+gitreset--hardorigin/master&&+echothree>file&&gitcommit-a-mthree&&+test_must_failgitsend-pack../parentmaster2>errs&&+grep-q"pack size limit"errs+)&&+parent_head=$(cdparent&&gitrev-parse--verifymaster)&&+child_head=$(cdchild&&gitrev-parse--verifymaster)&&+test"$parent_head"!="$child_head"+'++test_expect_success'repeated push failure proves that objects were not stored remotely''+(+cdchild&&+test_must_failgitsend-pack../parentmaster2>errs&&+grep-q"pack size limit"errs+)&&+parent_head=$(cdparent&&gitrev-parse--verifymaster)&&+child_head=$(cdchild&&gitrev-parse--verifymaster)&&+test"$parent_head"!="$child_head"+'++test_expect_success'increase receive.packSizeLimit''+(+cdparent&&+gitconfigreceive.packSizeLimit1000000&&+test_might_failgitreceive-pack.<../pkt-flush>output&&+grep-q"limit-pack-size=1000000"output+)+'++test_expect_success'push is allowed when pack size is not exceeded''+(+cdchild&&+gitsend-pack../parentmaster2>errs&&+test_must_failgrep-q"pack size limit"errs+)&&+parent_head=$(cdparent&&gitrev-parse--verifymaster)&&+child_head=$(cdchild&&gitrev-parse--verifymaster)&&+test"$parent_head"="$child_head"+'+ test_done
On Sun, May 15, 2011 at 23:37, Johan Herland [off-list ref] wrote:
Here's the next iteration of what started out as a simple patch to let
the server refuse pushes that exceeded a configurable limit on the
#objects in a pack.
The current patch series allows limiting pushes by
- size of pack
- #objects in pack
- #commits in pack
FWIW I'd find this very useful. I recently spent a fair amount of time
cleaning up the mess created by a user pushing all the tags from
repository A to repository B (don't ask), the options you've
implemented here would have stopped that.
And as you point out even if you refuse these sort of things with a
hook (which I later implemented) that doesn't stop the server from
accepting the objects and keeping them around.
On Sun, May 15, 2011 at 14:37, Johan Herland [off-list ref] wrote:
Currently we refuse combining --max-pack-size with --stdout since there's
no way to make multiple packs when the pack is written to stdout. However,
we want to be able to limit the maximum size of the pack created by
--stdout (and abort pack-objects if we are unable to meet that limit).
Therefore, when used together with --stdout, we reinterpret --max-pack-size
to indicate the maximum pack size which - if exceeded - will cause
pack-objects to abort with an error message.
...
if (pack_to_stdout) {
+ if (nr_written != nr_remaining)
+ die("unable to make pack within the pack size"
+ " limit (%lu bytes)", pack_size_limit);
I think this is too late. We have already output a bunch of data, up
to the size limit at this point. If the size limit is non-trivial
(e.g. 5 MB) we have already sent most of that to the remote side, and
its already written some of that out to disk.
I'd like this to be a soft limit derived from the reused object sizes.
When planning the pack by looking at where we will reuse an object
from, sum those sizes. If the sum of these sizes would break this
limit, then we abort before even writing the pack header out.
--
Shawn.
On Sun, May 15, 2011 at 14:37, Johan Herland [off-list ref] wrote:
The new --max-object-count option behaves similarly to --max-pack-size,
except that the decision to split packs is determined by the number of
objects in the pack, and not by the size of the pack.
Like my note about pack size for this case... I think doing this
during writing is too late. We should be aborting the counting phase
if the output pack is to stdout and we are going to exceed this limit.
--
Shawn.
From: Johan Herland <hidden> Date: 2016-06-15 22:51:15
On Monday 16 May 2011, Shawn Pearce wrote:
On Sun, May 15, 2011 at 14:37, Johan Herland [off-list ref] wrote:
quoted
The new --max-object-count option behaves similarly to --max-pack-size,
except that the decision to split packs is determined by the number of
objects in the pack, and not by the size of the pack.
Like my note about pack size for this case... I think doing this
during writing is too late. We should be aborting the counting phase
if the output pack is to stdout and we are going to exceed this limit.
The patch actually does this in the --stdout case. Look at the last
hunk in builtin/pack-objects.c:
@@ -2349,6 +2366,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix) if (non_empty && !nr_result) return 0;+ if (pack_to_stdout && object_count_limit && object_count_limit < nr_result)+ die("unable to make pack within the object count limit"+ " (%lu objects)", object_count_limit); if (nr_result) prepare_pack(window, depth); write_pack_file();
So in the --stdout case, we have already aborted before we start
writing the pack (i.e. after the counting phase).
The commit message you quote above, are for the case where someone uses
--max-object-count _without_ --stdout, in which case we compare
nr_written to object_count_limit to determine when to split the pack.
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
On Sun, May 15, 2011 at 15:31, Johan Herland [off-list ref] wrote:
quoted hunk
On Monday 16 May 2011, Shawn Pearce wrote:
quoted
On Sun, May 15, 2011 at 14:37, Johan Herland [off-list ref] wrote:
quoted
The new --max-object-count option behaves similarly to --max-pack-size,
except that the decision to split packs is determined by the number of
objects in the pack, and not by the size of the pack.
Like my note about pack size for this case... I think doing this
during writing is too late. We should be aborting the counting phase
if the output pack is to stdout and we are going to exceed this limit.
The patch actually does this in the --stdout case. Look at the last
hunk in builtin/pack-objects.c:
if (non_empty && !nr_result)
return 0;
+ if (pack_to_stdout && object_count_limit && object_count_limit < nr_result)
+ die("unable to make pack within the object count limit"
+ " (%lu objects)", object_count_limit);
if (nr_result)
prepare_pack(window, depth);
write_pack_file();
So in the --stdout case, we have already aborted before we start
writing the pack (i.e. after the counting phase).
The commit message you quote above, are for the case where someone uses
--max-object-count _without_ --stdout, in which case we compare
nr_written to object_count_limit to determine when to split the pack.
Thanks for the clarification. Its Sunday, I am clearly not scanning
patches with the level of detail I should be. :-)
Given that this block is in here, most of the series looks pretty good
to me. Thanks for following up with this round, I know its a lot more
than you originally wanted to do for this "simple" limit, but I think
its a worthwhile improvement.
--
Shawn.
From: Johan Herland <hidden> Date: 2016-06-15 22:51:15
On Monday 16 May 2011, Shawn Pearce wrote:
On Sun, May 15, 2011 at 14:37, Johan Herland [off-list ref] wrote:
quoted
Currently we refuse combining --max-pack-size with --stdout since
there's no way to make multiple packs when the pack is written to
stdout. However, we want to be able to limit the maximum size of the
pack created by --stdout (and abort pack-objects if we are unable to
meet that limit).
Therefore, when used together with --stdout, we reinterpret
--max-pack-size to indicate the maximum pack size which - if exceeded
- will cause pack-objects to abort with an error message.
...
quoted
if (pack_to_stdout) {
+ if (nr_written != nr_remaining)
+ die("unable to make pack within the pack size"
+ " limit (%lu bytes)", pack_size_limit);
I think this is too late. We have already output a bunch of data, up
to the size limit at this point. If the size limit is non-trivial
(e.g. 5 MB) we have already sent most of that to the remote side, and
its already written some of that out to disk.
I'd like this to be a soft limit derived from the reused object sizes.
When planning the pack by looking at where we will reuse an object
from, sum those sizes. If the sum of these sizes would break this
limit, then we abort before even writing the pack header out.
I agree, but it's currently late Sunday (early Monday), and after
looking at this for a while, I'm no longer thinking straight.
If someone that groks the pack-objects internal could help out, I'd be
really grateful. AFAICS, we need to drill into prepare_pack() to find
the details needed to estimate the total pack size, but I don't know
exactly which data structure(s) holds the data needed. We probably need
to accumulate a pack size estimate in find_deltas(), and then sum those
across the threads, before we finally compare the total estimate to
pack_size_limit prior to calling write_pack_file().
...Johan
--
Johan Herland, [off-list ref]
www.herland.net
From: Jeff King <hidden> Date: 2016-06-15 22:51:15
On Sun, May 15, 2011 at 11:37:13PM +0200, Johan Herland wrote:
I first wrote this patch on a base where e07fd15 (Peff's "send-pack:
unbreak push over stateless rpc") was not present, and then resolved
a conflict when rebasing this patch onto current master. I hope Peff
or Johannes (Sixt) can verify that my patch does not reintroduce the
deadlock they fixed.
I don't think it reintroduces the deadlock we fixed, but I am worried
that it produces a new, similar one. That is, imagine pack-objects fails
horribly, maybe or maybe not producing any output. We close its pipe
outgoing pipe at the end of run_command, and per Johannes' 09c9957, we
are sure that the sideband demuxer does not hold a pipe end open,
either.
So the remote side sees us close our end of the pipe, knows there is no
more pack data, and then closes their end. So our receive_status should
either get some error message, or EOF, either of which is fine. So no
deadlock there. Essentially, we have done a half-duplex shutdown of the
connection to the remote, and that is enough for everybody to keep
going.
But what if we are not using pipes, but have an actual TCP socket? In
that case, I'm not sure what happens. We don't seem to do a half-duplex
shutdown() anywhere. So I'm concerned that we are still open for sending
from the remote's perspective, and we may deadlock.
However, that would not necessarily be something introduced by your
patch; you would deadlock in receive_status, but prior to that it would
deadlock in the sideband demuxer.
AFAICT, the only way to have an actual TCP connection instead of pipes
is for the push to go over git://, which is enabled almost nowhere. But
we should perhaps check for deadlock on failed pack-objects in that
case, both with and without your patch.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:51:15
On Mon, May 16, 2011 at 12:07:45AM -0400, Jeff King wrote:
But what if we are not using pipes, but have an actual TCP socket? In
that case, I'm not sure what happens. We don't seem to do a half-duplex
shutdown() anywhere. So I'm concerned that we are still open for sending
from the remote's perspective, and we may deadlock.
However, that would not necessarily be something introduced by your
patch; you would deadlock in receive_status, but prior to that it would
deadlock in the sideband demuxer.
AFAICT, the only way to have an actual TCP connection instead of pipes
is for the push to go over git://, which is enabled almost nowhere. But
we should perhaps check for deadlock on failed pack-objects in that
case, both with and without your patch.
Ugh, yeah, yet another deadlock. I can reproduce reliably with this:
[in one terminal]
mkdir daemon &&
git init --bare daemon/repo.git &&
git --git-dir=daemon/repo.git config daemon.receivepack true &&
git daemon --base-path=$PWD/daemon --export-all --verbose
[in another]
git init repo &&
cd repo &&
git remote add origin git://localhost/repo.git &&
echo content >file && git add file && git commit -a -m one &&
git push -f origin HEAD &&
echo content >>file && git commit -a -m two &&
sha1=`git rev-parse HEAD:file` &&
file=`echo $sha1 | sed 's,..,&/,'` &&
rm -fv .git/objects/$file &&
git push
and this patch fixes it:
@@ -345,6 +345,13 @@ int send_pack(struct send_pack_args *args,ref->status=REF_STATUS_NONE;if(args->stateless_rpc)close(out);+/* in case we actually have a full-duplex socket+*andnottwopipes;wecan'tuse"out"because+*ithasbeenclosedalready,butinthefull-duplex+*case,"in"and"out"aremerelydupsofeachother.+*Wecan'tdirectlyuse"in"becauseitmaybe+*pointingtothesidebanddemuxernow*/+shutdown(fd[0],SHUT_WR);if(use_sideband)finish_async(&demux);return-1;
It does call shutdown() on a non-socket in the pipe case. That should be
a harmless noop, AFAIK.
-Peff
@@ -345,6 +345,13 @@ int send_pack(struct send_pack_args *args,ref->status=REF_STATUS_NONE;if(args->stateless_rpc)close(out);+/* in case we actually have a full-duplex socket+*andnottwopipes;wecan'tuse"out"because+*ithasbeenclosedalready,butinthefull-duplex+*case,"in"and"out"aremerelydupsofeachother.+*Wecan'tdirectlyuse"in"becauseitmaybe+*pointingtothesidebanddemuxernow*/+shutdown(fd[0],SHUT_WR);if(use_sideband)finish_async(&demux);return-1;
It does call shutdown() on a non-socket in the pipe case. That should be
a harmless noop, AFAIK.
If we do care (or if we just want to be cleaner), this patch series also
works (and goes on top of the same deadlock topic, i.e., e07fd15):
[1/3]: connect: treat generic proxy processes like ssh processes
[2/3]: connect: let callers know if connection is a socket
[3/3]: send-pack: avoid deadlock on git:// push with failed pack-objects
Another approach would be to actually spawn a pipe-based helper for tcp
connections (sort of a "git netcat"). That would mean all git-protocol
connections would get the same descriptor semantics, in case any other
bugs are lurking. I'm not sure if the ugliness (extra process to manage)
and decreased efficiency (pointlessly proxying data through an extra set
of pipes) are worth it. The only thing which makes me not reject it out
of hand is that it is already how git-over-ssh works (and not unlike
git-over-http), so the extra process and inefficiency are probably not
_that_ big a deal. It just feels ugly. I wish there were a portable way
to split a full-duplex socket into two half-duplex halves, but AFAIK,
that is not possible.
-Peff
From: Jeff King <hidden> Date: 2016-06-15 22:51:15
The git_connect function returns two ends of a pipe for
talking with a remote, plus a struct child_process
representing the other end of the pipe. If we have a direct
socket connection, then this points to a special "no_fork"
child process.
The code path for doing git-over-pipes or git-over-ssh sets
up this child process to point to the child git command or
the ssh process. When we call finish_connect eventually, we
check wait() on the command and report its return value.
The code path for git://, on the other hand, always sets it
to no_fork. In the case of a direct TCP connection, this
makes sense; we have no child process. But in the case of a
proxy command (configured by core.gitproxy), we do have a
child process, but we throw away its pid, and therefore
ignore its return code.
Instead, let's keep that information in the proxy case, and
respect its return code, which can help catch some errors
(though depending on your proxy command, it will be errors
reported by the proxy command itself, and not propagated
from git commands. Still, it is probably better to propagate
such errors than to ignore them).
It also means that the child_process field can reliably be
used to determine whether the returned descriptors are
actually a full-duplex socket, which means we should be
using shutdown() instead of a simple close.
Signed-off-by: Jeff King <redacted>
---
Obviously I am interested mainly in the last bit for this series. But I
consider the rest of it a minor bugfix on its own.
connect.c | 27 +++++++++++++++------------
1 files changed, 15 insertions(+), 12 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:51:15
They might care because they want to do a half-duplex close.
With pipes, that means simply closing the output descriptor;
with a socket, you must actually call shutdown.
Instead of exposing the magic no_fork child_process struct,
let's encapsulate the test in a function.
Signed-off-by: Jeff King <redacted>
---
An more object-oriented refactoring would be something like:
struct git_connection {
struct child_process child;
int in;
int out;
};
void git_connection_read_fd(struct git_connection *c)
{
return c->in;
}
void git_connect_write_fd(struct git_connect *c)
{
return c->child.pid ? c->out : c->in;
}
void git_connection_half_duplex_close(struct git_connection *c)
{
if (!c->child.pid)
shutdown(c->in, SHUT_WR);
else
close(c->out);
}
but the idea that a git connection is defined by two file descriptors
runs throughout the code (in fact, we don't even explicitly do the
half-duplex close in the pipe case; we hand the descriptor off to the
pack-objects run-command, which takes ownership). So trying to be fancy
and abstracted is not worth it in this case.
cache.h | 1 +
connect.c | 7 ++++++-
2 files changed, 7 insertions(+), 1 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:51:15
Commit 09c9957c fixes a deadlock in which pack-objects
fails, the remote end is still waiting for pack data, and we
are still waiting for the remote end to say something (see
that commit for a much more in-depth explanation).
We solved the problem there by making sure the output pipe
is closed on error; thus the remote sees EOF, and proceeds
to complain and close its end of the connection.
However, in the special case of push over git://, we don't
have a pipe, but rather a full-duplex socket, with another
dup()-ed descriptor in place of the second half of the pipe.
In this case, closing the second descriptor signals nothing
to the remote end, and we still deadlock.
This patch calls shutdown() explicitly to signal EOF to the
other side.
Signed-off-by: Jeff King <redacted>
---
And if you wanted to drop the first two patches, this probably works OK
without the conditional, as the shutdown is just a no-op on a pipe
descriptor then.
builtin-send-pack.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
From: Johannes Sixt <hidden> Date: 2016-06-15 22:51:15
Am 16.05.2011 08:46, schrieb Jeff King:
The git_connect function returns two ends of a pipe for
talking with a remote, plus a struct child_process
representing the other end of the pipe. If we have a direct
socket connection, then this points to a special "no_fork"
child process.
The code path for doing git-over-pipes or git-over-ssh sets
up this child process to point to the child git command or
the ssh process. When we call finish_connect eventually, we
check wait() on the command and report its return value.
The code path for git://, on the other hand, always sets it
to no_fork. In the case of a direct TCP connection, this
makes sense; we have no child process. But in the case of a
proxy command (configured by core.gitproxy), we do have a
child process, but we throw away its pid, and therefore
ignore its return code.
Instead, let's keep that information in the proxy case, and
respect its return code, which can help catch some errors
This patch looks strikingly familiar. I had written an almost identical
change more than 3 years ago and forgot about it, though the
justification I noted in the commit was more to properly shutdown the
proxy process rather than to abandon it and let it be collected by
init(8). Your justification is much better.
There's one problem with your implementation, though:
At this point, proxy->argv would point to automatic storage; but we
need argv[0] in finish_command() for error reporting. In my
implementation, I xmalloced the pointer array and leaked it. (And
that's probably the reason that I never submitted the patch.) I
wouldn't dare to make argv just static because this limits us to have
just one open connection at a time established via git_proxy_connect().
Dunno...
Below is the interdiff that turns your patch into mine, mostly for
exposition: The two hunks in git_connect() are just cosmetic
differences. But the first hunk should be squashed into your patch to
fix a potential crash in an error situation (e.g., when the proxy
dies from a signal) at the cost of a small memory leak.
From: Johannes Sixt <hidden> Date: 2016-06-15 22:51:15
Am 16.05.2011 08:52, schrieb Jeff King:
quoted hunk
And if you wanted to drop the first two patches, this probably works OK
without the conditional, as the shutdown is just a no-op on a pipe
descriptor then.
builtin-send-pack.c | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
From: Jeff King <hidden> Date: 2016-06-15 22:51:15
On Mon, May 16, 2011 at 09:57:58PM +0200, Johannes Sixt wrote:
quoted
The code path for git://, on the other hand, always sets it
to no_fork. In the case of a direct TCP connection, this
makes sense; we have no child process. But in the case of a
proxy command (configured by core.gitproxy), we do have a
child process, but we throw away its pid, and therefore
ignore its return code.
Instead, let's keep that information in the proxy case, and
respect its return code, which can help catch some errors
This patch looks strikingly familiar. I had written an almost identical
change more than 3 years ago and forgot about it, though the
justification I noted in the commit was more to properly shutdown the
proxy process rather than to abandon it and let it be collected by
init(8). Your justification is much better.
Thanks, I had no idea your patch existed. I hate to duplicate work, but
at least it's a sanity check that it's not a totally stupid idea. ;)
[...]
At this point, proxy->argv would point to automatic storage; but we
need argv[0] in finish_command() for error reporting.
Ick. Good catch.
In my implementation, I xmalloced the pointer array and leaked it.
(And that's probably the reason that I never submitted the patch.) I
wouldn't dare to make argv just static because this limits us to have
just one open connection at a time established via
git_proxy_connect(). Dunno...
We also need to worry about the contents of each argv[] element, no? So
we should be xstrdup()ing the host and port, which point into some
string which gets passed to us. I didn't trace its provenance but I
think it is better to be defensive.
The leak is probably OK in a practical sense (you generally make no more
than one such connection per command), but it does seem ugly. I would
not be surprised if many other run-command invocations leak similarly.
The interdiff with the strdups is:
From: Jeff King <hidden> Date: 2016-06-15 22:51:15
On Mon, May 16, 2011 at 10:02:16PM +0200, Johannes Sixt wrote:
quoted
+ if (git_connection_is_socket(conn))
+ shutdown(fd[0], SHUT_WR);
We probably need a wrapper for shutdown() on Windows. I'll look into
this tomorrow.
FWIW, we already make an identical call in transport-helper.c (I was
tempted even to use "1" instead of SHUT_WR for portability, but it seems
nobody has complained so far about the use in transport-helper).
-Peff
From: Johannes Sixt <hidden> Date: 2016-06-15 22:51:15
Am 17.05.2011 07:54, schrieb Jeff King:
On Mon, May 16, 2011 at 09:57:58PM +0200, Johannes Sixt wrote:
quoted
In my implementation, I xmalloced the pointer array and leaked it.
I noticed that it actually isn't leaked because finish_connect() frees
it. For this reason, I actually have to wonder why your version that
stored a pointer to automatic storage in ->argv worked.
We also need to worry about the contents of each argv[] element, no? So
we should be xstrdup()ing the host and port, which point into some
string which gets passed to us. I didn't trace its provenance but I
think it is better to be defensive.
I would not worry too much today. Of course, functions other than
start_command() might begin to access ->argv[i] with i > 0 later, but
then we have to audit all users of struct child_process anyway.
Currently, only start_command() uses these values, which is always
called at a time when they are still valid.
-- Hannes
From: Jeff King <hidden> Date: 2016-06-15 22:51:16
On Tue, May 17, 2011 at 10:14:43PM +0200, Johannes Sixt wrote:
Am 17.05.2011 07:54, schrieb Jeff King:
quoted
On Mon, May 16, 2011 at 09:57:58PM +0200, Johannes Sixt wrote:
quoted
In my implementation, I xmalloced the pointer array and leaked it.
I noticed that it actually isn't leaked because finish_connect() frees
it. For this reason, I actually have to wonder why your version that
stored a pointer to automatic storage in ->argv worked.
It probably didn't. As a simple refactoring, I didn't test the proxy
codepath, and apparently nothing in our test suite does, either. :(
We can put the patch below on top (it fails with my original series, but
passes with the bugfix you noticed).
I would not worry too much today. Of course, functions other than
start_command() might begin to access ->argv[i] with i > 0 later, but
then we have to audit all users of struct child_process anyway.
Currently, only start_command() uses these values, which is always
called at a time when they are still valid.
That makes sense.
-- >8 --
Subject: [PATCH] test core.gitproxy configuration
This is just a basic sanity test to see whether
core.gitproxy works at all. Until now, we were not testing
anywhere.
Signed-off-by: Jeff King <redacted>
---
This is really basic. Apparently you can do horrible things like
git config core.gitproxy "./proxy for kernel.org"
git config core.gitproxy "./other-proxy for example.com"
I can make it more elaborate if we really want to care.
t/t5532-fetch-proxy.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
1 files changed, 42 insertions(+), 0 deletions(-)
create mode 100755 t/t5532-fetch-proxy.sh
@@ -0,0 +1,42 @@+#!/bin/sh++test_description='fetching via git:// using core.gitproxy'+../test-lib.sh++test_expect_success'setup remote repo''+gitinitremote&&+(cdremote&&+echocontent>file&&+gitaddfile&&+gitcommit-mone+)+'++cat>proxy<<'EOF'+#!/bin/sh+echo>&2"proxying for $*"+cmd=`perl-e'+read(STDIN,$buf,4);+my$n=hex($buf)-4;+read(STDIN,$buf,$n);+my($cmd,$other)=split/\0/,$buf;+# drop absolute-path on repo name+$cmd=~s{/}{};+print$cmd;+'`+exec$cmd+EOF+chmod+xproxy+test_expect_success'setup local repo''+gitremoteaddfakegit://example.com/remote&&+gitconfigcore.gitproxy./proxy+'++test_expect_success'fetch through proxy works''+gitfetchfake&&+echoone>expect&&+gitlog-1--format=%sFETCH_HEAD>actual&&+test_cmpexpectactual+'++test_done
From: Johannes Sixt <hidden> Date: 2016-06-15 22:51:16
Even though Windows's socket functions look like their POSIX counter parts,
they do not operate on file descriptors, but on "socket objects". To bring
the functions in line with POSIX, we have proxy functions that wrap and
unwrap the socket objects in file descriptors using open_osfhandle and
get_osfhandle. But shutdown() was not proxied, yet. Fix this.
Signed-off-by: Johannes Sixt <redacted>
---
Am 17.05.2011 07:56, schrieb Jeff King:
On Mon, May 16, 2011 at 10:02:16PM +0200, Johannes Sixt wrote:
quoted
quoted
+ if (git_connection_is_socket(conn))
+ shutdown(fd[0], SHUT_WR);
We probably need a wrapper for shutdown() on Windows. I'll look into
this tomorrow.
We have a shutdown() on Windows, but it does not work out of the box.
Making it work is not a big deal, but it is an academic excercise if it
were only for this topic: On Windows, send-pack hangs when network
connections are involved for unknown reasons as long as side-band-64k
is enabled. If it is not enabled, the deadlock that you fixed does not
happen in the first place and it is irrelevant whether shutdown works.
FWIW, we already make an identical call in transport-helper.c (I was
tempted even to use "1" instead of SHUT_WR for portability, but it seems
nobody has complained so far about the use in transport-helper).
Yes, and for the reasons mentioned above, this patch is intended for
the 1.7.4 series, where transport-helper.c went public. (But it should
apply cleanly to current master as well.) We do not have tests that
exercise the code in transport-helper.c, but I did test that this
shutdown implementation does something reasonable when it is merged
with your send-pack deadlock topic branch.
compat/mingw.c | 7 +++++++
compat/mingw.h | 3 +++
2 files changed, 10 insertions(+), 0 deletions(-)
@@ -1219,6 +1219,13 @@ int mingw_setsockopt(int sockfd, int lvl, int optname, void *optval, int optlen)returnsetsockopt(s,lvl,optname,(constchar*)optval,optlen);}+#undef shutdown+intmingw_shutdown(intsockfd,inthow)+{+SOCKETs=(SOCKET)_get_osfhandle(sockfd);+returnshutdown(s,how);+}+#undef listenintmingw_listen(intsockfd,intbacklog){