On Mon, Aug 15, 2016 at 09:57:29PM +0200, Christian Couder wrote:

> From: Jeff King <p...@peff.net>
> 
> Receive-pack feeds its input to either index-pack or
> unpack-objects, which will happily accept as many bytes as
> a sender is willing to provide. Let's allow an arbitrary
> cutoff point where we will stop writing bytes to disk.
> 
> What has already been written to disk can be cleaned
> outside of receive-pack.

This second paragraph hints at a related problem.

Git is generally happy to leave tmp_pack_* around to be cleaned up later
next time git-gc runs. Including its default 2-week grace time.

So imagine that tries to "git push" in a loop. And each time they push,
you say "nope, that's too big". And each time you acquire a new 2GB
tmp_pack file. If your goal was to prevent somebody from streaming
straight to your filesystem and filling up your disk, then it wasn't
very successful. :)

The simple fix is to call register_tempfile() in open_pack_file(), and
just have index-pack clean up the file on its way out.

But there are harder cases. For instance, imagine somebody pushes a
500MB file, and you have a pre-receive hook that says "too big; I won't
accept this". And then they push in a loop, as before. You've accepted
the incoming pack into the repository by the time the pre-receive runs.
You can't just delete it, because you don't know if other simultaneous
processes have started to depend on the objects.

To solve that, I have patches that put incoming packfiles into a
"quarantine" area, then run the connectivity check and pre-receive hooks
with the quarantine accessible via GIT_ALTERNATE_OBJECT_DIRECTORIES. And
then we either move the quarantine packs into the real repo, or blow
away the tmpdir, depending on whether the hooks said the objects were
OK.

Those are patches I plan to share upstream but just haven't gotten
around to yet.

> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 92e1213..7627f7f 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -46,6 +46,7 @@ static int transfer_unpack_limit = -1;
>  static int advertise_atomic_push = 1;
>  static int advertise_push_options;
>  static int unpack_limit = 100;
> +static off_t max_input_size;
>  static int report_status;
>  static int use_sideband;
>  static int use_atomic;
> @@ -212,6 +213,11 @@ static int receive_pack_config(const char *var, const 
> char *value, void *cb)
>               return 0;
>       }
>  
> +     if (strcmp(var, "receive.maxsize") == 0) {
> +             max_input_size = git_config_ulong(var, value);
> +             return 0;
> +     }

Another off_t/ulong mismatch. I think you want git_config_int64() here.

> @@ -1650,6 +1656,9 @@ static const char *unpack(int err_fd, struct 
> shallow_info *si)
>               if (fsck_objects)
>                       argv_array_pushf(&child.args, "--strict%s",
>                               fsck_msg_types.buf);
> +             if (max_input_size)
> +                     argv_array_pushf(&child.args, "--max-input-size=%lu",
> +                             max_input_size);

And here, PRIuMAX and uintmax_t. Or perhaps simpler, just store the
value as a string here and pass it on to index-pack (which would then
need to learn to handle suffixes like "2g"). We do a similar trick in
repack; see b861e23 (repack: propagate pack-objects options as strings,
2014-01-22).

> diff --git a/t/t5546-push-limits.sh b/t/t5546-push-limits.sh
> new file mode 100755
> index 0000000..d3a4d1a
> --- /dev/null
> +++ b/t/t5546-push-limits.sh
> @@ -0,0 +1,47 @@
> +#!/bin/sh
> +
> +test_description='check input limits for pushing'
> +. ./test-lib.sh
> +
> +test_expect_success 'create known-size commit' '
> +     test-genrandom foo 1024 >file &&
> +     git add file &&
> +     test_commit one-k
> +'
> +
> +test_expect_success 'create remote repository' '
> +     git init --bare dest &&
> +     git --git-dir=dest config receive.unpacklimit 1
> +'

We're going to do basically the same battery of tests against an
unpacklimit of "1" (to catch index-pack) and of "10" (to catch
unpack-objects). It might be clearer to just have a for-loop like:

  for unpacklimit in 1 100
  do
        test_expect_success 'create remote repository' '
                rm -rf dest &&
                git init --bare dest &&
                git -C dest config receive.unpacklimit $unpacklimit
        '

        test_expect_success 'receive.maxsize rejects push' '
                git -C dest config receive.maxsize 512 &&
                test_must_fail git push dest HEAD &&
        '

        test_expect_success 'bumping limit allows push' '
                git -C dest config receive.maxsize 4k &&
                git push dest HEAD
        '
  done

and it's probably worth a comment at the top of the loop explaining what
the heck those numbers mean. :)

-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to