* [PATCH] apply: improve error messages when reading patch
@ 2023-06-26 9:37 Phillip Wood via GitGitGadget
2023-06-26 15:58 ` Junio C Hamano
0 siblings, 1 reply; 2+ messages in thread
From: Phillip Wood via GitGitGadget @ 2023-06-26 9:37 UTC (permalink / raw
To: git; +Cc: Taylor Blau, Phillip Wood, Phillip Wood
From: Phillip Wood <phillip.wood@dunelm.org.uk>
Commit f1c0e3946e (apply: reject patches larger than ~1 GiB, 2022-10-25)
added a limit on the size of patch that apply will process to avoid
integer overflows. The implementation re-used the existing error message
for when we are unable to read the patch. This is unfortunate because (a) it
does not signal to the user that the patch is being rejected because it
is too large and (b) it uses error_errno() without setting errno.
This patch adds a specific error message for the case when a patch is
too large. It also updates the existing message to make it clearer that
it is the patch that cannot be read rather than any other file and marks
both messages for translation. The "git apply" prefix is also dropped to
match most of the rest of the error messages in apply.c (there are still
a few error messages that prefixed with "git apply" and are not marked
for translation after this patch). The test added in f1c0e3946e is
updated accordingly.
Reported-by: Premek Vysoky <Premek.Vysoky@microsoft.com>
Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
---
apply: improve error messages when reading patch
I haven't changed it but I found the test a bit confusing as it
superficially looks like it is generating a valid patch that is too
large, but it isn't actually a valid patch because the changed line is
lacking a leading '+' and a trailing '\n'. As we are checking for the
error message that does not matter but it might be clearer to just do
sz=$((1024 * 1024 * 1023)) &&
test-tool genzeros $sz | test_must_fail git apply &&
grep "patch too large" err
if we don't care about the patch being valid. Alternatively we could
create a valid patch with
sz=$((1024 * 1024 * 1023)) &&
{
cat <<-\EOF &&
diff --git a/file b/file
new file mode 100644
--- /dev/null
+++ b/file
@@ -0,0 +1 @@
EOF
printf "+%${sz}s\n" x
} | test_must_fail git apply 2>err &&
grep "patch too large" err
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1552%2Fphillipwood%2Fapply-error-message-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1552/phillipwood/apply-error-message-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1552
apply.c | 7 ++++---
t/t4141-apply-too-large.sh | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/apply.c b/apply.c
index 6212ab3a1b3..21c7f92ada8 100644
--- a/apply.c
+++ b/apply.c
@@ -410,9 +410,10 @@ static void say_patch_name(FILE *output, const char *fmt, struct patch *patch)
static int read_patch_file(struct strbuf *sb, int fd)
{
- if (strbuf_read(sb, fd, 0) < 0 || sb->len >= MAX_APPLY_SIZE)
- return error_errno("git apply: failed to read");
-
+ if (strbuf_read(sb, fd, 0) < 0)
+ return error_errno(_("failed to read patch"));
+ else if (sb->len >= MAX_APPLY_SIZE)
+ return error(_("patch too large"));
/*
* Make sure that we have some slop in the buffer
* so that we can do speculative "memcmp" etc, and
diff --git a/t/t4141-apply-too-large.sh b/t/t4141-apply-too-large.sh
index 58742d4fc5d..20cc1209f62 100755
--- a/t/t4141-apply-too-large.sh
+++ b/t/t4141-apply-too-large.sh
@@ -17,7 +17,7 @@ test_expect_success EXPENSIVE 'git apply rejects patches that are too large' '
EOF
test-tool genzeros
} | test_copy_bytes $sz | test_must_fail git apply 2>err &&
- grep "git apply: failed to read" err
+ grep "patch too large" err
'
test_done
base-commit: fe86abd7511a9a6862d5706c6fa1d9b57a63ba09
--
gitgitgadget
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] apply: improve error messages when reading patch
2023-06-26 9:37 [PATCH] apply: improve error messages when reading patch Phillip Wood via GitGitGadget
@ 2023-06-26 15:58 ` Junio C Hamano
0 siblings, 0 replies; 2+ messages in thread
From: Junio C Hamano @ 2023-06-26 15:58 UTC (permalink / raw
To: Phillip Wood via GitGitGadget; +Cc: git, Taylor Blau, Phillip Wood
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> Commit f1c0e3946e (apply: reject patches larger than ~1 GiB, 2022-10-25)
> added a limit on the size of patch that apply will process to avoid
> integer overflows. The implementation re-used the existing error message
> for when we are unable to read the patch. This is unfortunate because (a) it
> does not signal to the user that the patch is being rejected because it
> is too large and (b) it uses error_errno() without setting errno.
Good description of the issue, and ...
> static int read_patch_file(struct strbuf *sb, int fd)
> {
> - if (strbuf_read(sb, fd, 0) < 0 || sb->len >= MAX_APPLY_SIZE)
> - return error_errno("git apply: failed to read");
> -
> + if (strbuf_read(sb, fd, 0) < 0)
> + return error_errno(_("failed to read patch"));
> + else if (sb->len >= MAX_APPLY_SIZE)
> + return error(_("patch too large"));
... quite obvious improvement. Nice.
Will queue. Thanks.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-06-26 15:58 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-26 9:37 [PATCH] apply: improve error messages when reading patch Phillip Wood via GitGitGadget
2023-06-26 15:58 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).