diff mbox series

[3/4] t5541: add test for rejecting a push due to packfile size

Message ID 20240612115028.1169183-4-cmn@dwim.me (mailing list archive)
State New
Headers show
Series Report rejections over HTTP when the remote rejects during the transfer | expand

Commit Message

Carlos Martín Nieto June 12, 2024, 11:50 a.m. UTC
This rejection requires us to make sure we handle this kind of error
correctly rather than throw away the report in remote-curl and end up
with "Everything up-to-date" due to the lack of report.

Signed-off-by: Carlos Martín Nieto <cmn@dwim.me>
---
 t/t5546-receive-limits.sh | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

Comments

Taylor Blau June 12, 2024, 9:49 p.m. UTC | #1
On Wed, Jun 12, 2024 at 01:50:27PM +0200, Carlos Martín Nieto wrote:
> This rejection requires us to make sure we handle this kind of error
> correctly rather than throw away the report in remote-curl and end up
> with "Everything up-to-date" due to the lack of report.
>
> Signed-off-by: Carlos Martín Nieto <cmn@dwim.me>
> ---
>  t/t5546-receive-limits.sh | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

I haven't looked at the CGI stuff to know whether or not this behavior
can be reasonably emulated without the new Python script you wrote, but
a couple of small things in the meantime...

This patch's subject line states that it is modifying script t5541, but
the patch modifies t5546. I think the latter is correct, and there is
just a typo in the subject line, but wanted to make sure I pointed it
out regardless.

More importantly, this test fails after applying through this patch, but
not 4/4. After applying the final patch, it looks like it is still
failing for me. I figure that I am probably holding it wrong, but
regardless, here is the error message I see on my machine:

--- 8< ---
+ git init /home/ttaylorr/src/git/t/trash directory.t5546-receive-limits/httpd/www/error_too_large
Initialized empty Git repository in /home/ttaylorr/src/git/t/trash directory.t5546-receive-limits/httpd/www/error_too_large/.git/
+ git -C /home/ttaylorr/src/git/t/trash directory.t5546-receive-limits/httpd/www/error_too_large config receive.maxInputSize 128
+ test-tool genrandom foo 10485760
+ git add large-file
+ test_commit large-file
+ local notick=
+ local echo=echo
+ local append=
+ local author=
+ local signoff=
+ local indir=
+ local tag=light
+ test 1 != 0
+ break
+ indir=
+ local file=large-file.t
+ test -n
+ echo large-file
+ git add -- large-file.t
+ test -z
+ test_tick
+ test -z set
+ test_tick=1112912053
+ GIT_COMMITTER_DATE=1112912053 -0700
+ GIT_AUTHOR_DATE=1112912053 -0700
+ export GIT_COMMITTER_DATE GIT_AUTHOR_DATE
+ git commit -m large-file
[master 03a3078] large-file
 Author: A U Thor <author@example.com>
 2 files changed, 1 insertion(+)
 create mode 100644 large-file
 create mode 100644 large-file.t
+ git tag large-file
+ test_must_fail git push --porcelain http://127.0.0.1:5546/error_too_large HEAD:refs/tags/will-fail
+ _test_ok=
+ test_must_fail_acceptable git push --porcelain http://127.0.0.1:5546/error_too_large HEAD:refs/tags/will-fail
+ test git = env
+ return 0
+ git push --porcelain http://127.0.0.1:5546/error_too_large HEAD:refs/tags/will-fail
Enumerating objects: 8, done.
Counting objects: 100% (8/8), done.
Delta compression using up to 20 threads
Compressing objects: 100% (6/6), done.
error: RPC failed; curl 55 Send failure: Broken pipe
send-pack: unexpected disconnect while reading sideband packet
Writing objects: 100% (8/8), 10.00 MiB | 34.04 MiB/s, done.
Total 8 (delta 0), reused 0 (delta 0), pack-reused 0 (from 0)
fatal: the remote end hung up unexpectedly
fatal: the remote end hung up unexpectedly
error: failed to push some refs to 'http://127.0.0.1:5546/error_too_large'
+ exit_code=1
+ test 1 -eq 0
+ test_match_signal 13 1
+ test 1 = 141
+ test 1 = 269
+ return 1
+ test 1 -gt 129
+ test 1 -eq 127
+ test 1 -eq 126
+ return 0
+ test_must_fail git -C /home/ttaylorr/src/git/t/trash directory.t5546-receive-limits/httpd/www/error_too_large rev-parse --verify refs/tags/will-fail
+ _test_ok=
+ test_must_fail_acceptable git -C /home/ttaylorr/src/git/t/trash directory.t5546-receive-limits/httpd/www/error_too_large rev-parse --verify refs/tags/will-fail
+ test git = env
+ return 0
+ git -C /home/ttaylorr/src/git/t/trash directory.t5546-receive-limits/httpd/www/error_too_large rev-parse --verify refs/tags/will-fail
fatal: Needed a single revision
+ exit_code=128
+ test 128 -eq 0
+ test_match_signal 13 128
+ test 128 = 141
+ test 128 = 269
+ return 1
+ test 128 -gt 129
+ test 128 -eq 127
+ test 128 -eq 126
+ return 0
+ cat
+ test_cmp expect actual
+ test 2 -ne 2
+ eval diff -u "$@"
+ diff -u expect actual
--- expect	2024-06-12 21:48:50.005929827 +0000
+++ actual	2024-06-12 21:48:49.677930723 +0000
@@ -1,3 +0,0 @@
-To http://127.0.0.1:5546/error_too_large
-!	HEAD:refs/tags/will-fail	[remote rejected] (unpacker error)
-Done
error: last command exited with $?=1
not ok 18 - reject too-large push over HTTP
--- >8 ---

Thanks,
Taylor
Jeff King June 13, 2024, 9:21 a.m. UTC | #2
On Wed, Jun 12, 2024 at 01:50:27PM +0200, Carlos Martín Nieto wrote:

> diff --git a/t/t5546-receive-limits.sh b/t/t5546-receive-limits.sh
> index 9fc9ba552f1..ccbdf3945ab 100755
> --- a/t/t5546-receive-limits.sh
> +++ b/t/t5546-receive-limits.sh
> @@ -5,6 +5,11 @@ test_description='check receive input limits'
>  TEST_PASSES_SANITIZE_LEAK=true
>  . ./test-lib.sh
>  
> +
> +ROOT_PATH="$PWD"

I don't think this ROOT_PATH is doing anything?

> +. "$TEST_DIRECTORY"/lib-httpd.sh
> +start_serve_git

Since you're adding to an existing script, these should go near the
bottom (or possibly even in their own script). If we don't have apache
or curl support, then loading lib-httpd.sh at all will cause us to bail
from the test script immediately. So we'll never run these existing
tests at all on such platforms, even though we could (and do now).

-Peff
Jeff King June 13, 2024, 10:07 a.m. UTC | #3
On Wed, Jun 12, 2024 at 01:50:27PM +0200, Carlos Martín Nieto wrote:

> +test_expect_success 'reject too-large push over HTTP' '
> +	git init "$HTTPD_DOCUMENT_ROOT_PATH/error_too_large" &&
> +	git -C "$HTTPD_DOCUMENT_ROOT_PATH/error_too_large" config receive.maxInputSize 128 &&
> +	test-tool genrandom foo $((10*1024*1024)) >large-file &&
> +	git add large-file &&
> +	test_commit large-file &&
> +	test_must_fail git push --porcelain \
> +		$GIT_SERVE_URL/error_too_large \
> +		HEAD:refs/tags/will-fail >actual &&
> +	test_must_fail git -C "$HTTPD_DOCUMENT_ROOT_PATH/error_too_large" \
> +		rev-parse --verify refs/tags/will-fail &&
> +	cat >expect <<-EOF &&
> +	To $GIT_SERVE_URL/error_too_large
> +	!	HEAD:refs/tags/will-fail	[remote rejected] (unpacker error)
> +	Done
> +	EOF
> +	test_cmp expect actual
> +'

This test fails for me (even with the fix in the next patch) with:

  Exception occurred during processing of request from ('127.0.0.1', 47480)
  Traceback (most recent call last):
    File "/usr/lib/python3.11/socketserver.py", line 317, in _handle_request_noblock
      self.process_request(request, client_address)
    File "/usr/lib/python3.11/socketserver.py", line 348, in process_request
      self.finish_request(request, client_address)
    File "/usr/lib/python3.11/socketserver.py", line 361, in finish_request
      self.RequestHandlerClass(request, client_address, self)
    File "/home/peff/compile/git/t/lib-httpd/serve-git.py", line 35, in __init__
      super().__init__(*args, **kwargs)
    File "/usr/lib/python3.11/socketserver.py", line 755, in __init__
      self.handle()
    File "/usr/lib/python3.11/http/server.py", line 436, in handle
      self.handle_one_request()
    File "/usr/lib/python3.11/http/server.py", line 424, in handle_one_request
      method()
    File "/home/peff/compile/git/t/lib-httpd/serve-git.py", line 117, in do_GET
      self.do_receive_pack(responder, gitdir, True, protocol)
    File "/home/peff/compile/git/t/lib-httpd/serve-git.py", line 243, in do_receive_pack
      self._run_command(resp, 'receive-pack', gitdir, advertisement, protocol)
    File "/home/peff/compile/git/t/lib-httpd/serve-git.py", line 192, in _run_command
      with subprocess.Popen(argv, stdin=stdin, stdout=subprocess.PIPE, cwd=gitdir, env=env) as proc:
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/usr/lib/python3.11/subprocess.py", line 1026, in __init__
      self._execute_child(args, executable, preexec_fn, close_fds,
    File "/usr/lib/python3.11/subprocess.py", line 1955, in _execute_child
      raise child_exception_type(errno_num, err_msg, err_filename)
  FileNotFoundError: [Errno 2] No such file or directory: 'git'

which seems...odd. It should be finding "git" in bin-wrappers/, but I
think it is using a restricted/vanilla path. If I put "git" into
/usr/bin, it stops complaining (but obviously this is very wrong, as we
are not running the Git we're trying to test!).

Ah, I think I see it. You set up an environment for the Popen like:

  env = {k:v for k, v in os.environ.items() if k.startswith('GIT_')}
  if protocol is not None:
      env['GIT_PROTOCOL'] = protocol

so it does not contain $PATH (nor other possibly useful things!), so
presumably a fallback $PATH is used. I think you'd want to start with
"env = os.environ.copy()" and then modify it from there.

-Peff
diff mbox series

Patch

diff --git a/t/t5546-receive-limits.sh b/t/t5546-receive-limits.sh
index 9fc9ba552f1..ccbdf3945ab 100755
--- a/t/t5546-receive-limits.sh
+++ b/t/t5546-receive-limits.sh
@@ -5,6 +5,11 @@  test_description='check receive input limits'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+
+ROOT_PATH="$PWD"
+. "$TEST_DIRECTORY"/lib-httpd.sh
+start_serve_git
+
 # Let's run tests with different unpack limits: 1 and 10000
 # When the limit is 1, `git receive-pack` will call `git index-pack`.
 # When the limit is 10000, `git receive-pack` will call `git unpack-objects`.
@@ -83,4 +88,23 @@  test_expect_success "create known-size (1024 bytes) commit" '
 test_pack_input_limit index
 test_pack_input_limit unpack
 
+test_expect_success 'reject too-large push over HTTP' '
+	git init "$HTTPD_DOCUMENT_ROOT_PATH/error_too_large" &&
+	git -C "$HTTPD_DOCUMENT_ROOT_PATH/error_too_large" config receive.maxInputSize 128 &&
+	test-tool genrandom foo $((10*1024*1024)) >large-file &&
+	git add large-file &&
+	test_commit large-file &&
+	test_must_fail git push --porcelain \
+		$GIT_SERVE_URL/error_too_large \
+		HEAD:refs/tags/will-fail >actual &&
+	test_must_fail git -C "$HTTPD_DOCUMENT_ROOT_PATH/error_too_large" \
+		rev-parse --verify refs/tags/will-fail &&
+	cat >expect <<-EOF &&
+	To $GIT_SERVE_URL/error_too_large
+	!	HEAD:refs/tags/will-fail	[remote rejected] (unpacker error)
+	Done
+	EOF
+	test_cmp expect actual
+'
+
 test_done