| Message ID | cb96947b36b8ab314f5692daee6f627273ca66d3.1587289680.git.congdanhqx@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mailinfo: disallow and complains about NUL character | expand |
On Sun, 19 Apr 2020 at 13:05, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote: > --- a/t/t4254-am-corrupt.sh > +++ b/t/t4254-am-corrupt.sh > @@ -69,9 +69,11 @@ test_expect_success "NUL in commit message's body" ' > grep "a NUL byte in commit log message not allowed" err > ' > > -test_expect_failure "NUL in commit message's header" ' > +test_expect_success "NUL in commit message's header" ' > test_when_finished "git am --abort" && > write_nul_patch subject >subject.patch && > + test_must_fail git mailinfo msg patch <subject.patch 2>err && > + grep "a NUL byte in Subject is not allowed" err && > test_must_fail git am subject.patch 2>err && > grep "a NUL byte in Subject is not allowed" err > ' We used to fail for some reason and it's sort of clear from the size of the test what that reason is. Now that we flip "failure" to "success" we can add some more stuff here that works. Makes sense. Of course, somewhere is a line for stuffing too much into the test, i.e., you'll reach a point where you should have separate tests, but I think this is ok. Martin
On 2020-04-19 14:30:19+0200, Martin Ågren <martin.agren@gmail.com> wrote: > On Sun, 19 Apr 2020 at 13:05, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote: > > --- a/t/t4254-am-corrupt.sh > > +++ b/t/t4254-am-corrupt.sh > > @@ -69,9 +69,11 @@ test_expect_success "NUL in commit message's body" ' > > grep "a NUL byte in commit log message not allowed" err > > ' > > > > -test_expect_failure "NUL in commit message's header" ' > > +test_expect_success "NUL in commit message's header" ' > > test_when_finished "git am --abort" && > > write_nul_patch subject >subject.patch && > > + test_must_fail git mailinfo msg patch <subject.patch 2>err && > > + grep "a NUL byte in Subject is not allowed" err && > > test_must_fail git am subject.patch 2>err && > > grep "a NUL byte in Subject is not allowed" err > > ' > > We used to fail for some reason and it's sort of clear from the size > of the test what that reason is. Now that we flip "failure" to "success" > we can add some more stuff here that works. Makes sense. Of course, > somewhere is a line for stuffing too much into the test, i.e., you'll > reach a point where you should have separate tests, but I think this is > ok. I'm not really keen to check the "git-mailinfo" failure here. Since, another developer may come and decide that we should keep the mailinfo as is, i.e. keep NUL character in Subject, Author, Email, etc... And let's git-commit complains instead. Adding a check for git-mailinfo here may limit future development. Other's feedback is welcomed.
diff --git a/mailinfo.c b/mailinfo.c index 0e9911df6d..c31991e621 100644 --- a/mailinfo.c +++ b/mailinfo.c @@ -1138,6 +1138,11 @@ static void handle_info(struct mailinfo *mi) else continue; + if (memchr(hdr->buf, '\0', hdr->len)) { + error("a NUL byte in %s is not allowed.", header[i]); + mi->input_error = -1; + } + if (!strcmp(header[i], "Subject")) { if (!mi->keep_subject) { cleanup_subject(mi, hdr); diff --git a/t/t4254-am-corrupt.sh b/t/t4254-am-corrupt.sh index 98cda32d0a..d9d1ac6c7d 100755 --- a/t/t4254-am-corrupt.sh +++ b/t/t4254-am-corrupt.sh @@ -69,9 +69,11 @@ test_expect_success "NUL in commit message's body" ' grep "a NUL byte in commit log message not allowed" err ' -test_expect_failure "NUL in commit message's header" ' +test_expect_success "NUL in commit message's header" ' test_when_finished "git am --abort" && write_nul_patch subject >subject.patch && + test_must_fail git mailinfo msg patch <subject.patch 2>err && + grep "a NUL byte in Subject is not allowed" err && test_must_fail git am subject.patch 2>err && grep "a NUL byte in Subject is not allowed" err '
Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com> --- mailinfo.c | 5 +++++ t/t4254-am-corrupt.sh | 4 +++- 2 files changed, 8 insertions(+), 1 deletion(-)