@matheusbsbs / matheus-bagliano / commits / 3ef1494685

mailinfo -b: fix an out of bounds access

To remove bracketed strings containing "PATCH" from the subject line cleanup_subject() scans the subject for the opening bracket using an offset from the beginning of the line. It then searches for the closing bracket with strchr(). To calculate the length of the bracketed string it unfortunately adds rather than subtracts the offset from the result of strchr(). This leads to an out of bounds access in memmem() when looking to see if the brackets contain "PATCH". We have tests that trigger this bug that were added in ae52d57f0b (t5100: add some more mailinfo tests, 2017-05-31). The commit message mentions that they are marked test_expect_failure as they trigger an assertion in strbuf_splice(). While it is reassuring that strbuf_splice() detects the problem and dies in retrospect that should perhaps have warranted a little more investigation. The bug was introduced by 17635fc900 (mailinfo: -b option keeps [bracketed] strings that is not a [PATCH] marker, 2009-07-15). I think the reason it has survived so long is that '-b' is not a popular option and without it the offset is always zero. This was found by the address sanitizer while I was cleaning up the test_todo idea in [1]. [1] https://lore.kernel.org/git/db558292-2783-3270-4824-43757822a389@gmail.com/ Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Phillip Wood committed Oct 3, 2022 at 09:23 UTC 3ef1494685dea925d4e98ed06d9ea3fb5b3ecb89
2 files changed +3 -3
mailinfo.c
+1 -1
@@ -317,7 +317,7 @@ static void cleanup_subject(struct mailinfo *mi, struct strbuf *subject)
317 pos = strchr(subject->buf + at, ']');
318 if (!pos)
319 break;
320 - remove = pos - subject->buf + at + 1;
320 + remove = pos - (subject->buf + at) + 1;
321 if (!mi->keep_non_patch_brackets_in_subject ||
322 (7 <= remove &&
323 memmem(subject->buf + at, remove, "PATCH", 5)))
t/t5100-mailinfo.sh
+2 -2
@@ -201,13 +201,13 @@ test_expect_success 'mailinfo -b double [PATCH]' '
201 test z"$subj" = z"Subject: message"
202 '
203
204 -test_expect_failure 'mailinfo -b trailing [PATCH]' '
204 +test_expect_success 'mailinfo -b trailing [PATCH]' '
205 subj="$(echo "Subject: [other] [PATCH] message" |
206 git mailinfo -b /dev/null /dev/null)" &&
207 test z"$subj" = z"Subject: [other] message"
208 '
209
210 -test_expect_failure 'mailinfo -b separated double [PATCH]' '
210 +test_expect_success 'mailinfo -b separated double [PATCH]' '
211 subj="$(echo "Subject: [PATCH] [other] [PATCH] message" |
212 git mailinfo -b /dev/null /dev/null)" &&
213 test z"$subj" = z"Subject: [other] message"