git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] mailinfo.c::convert_to_utf8: reuse strlen info

From
Danh Doan <congdanhqx@gmail.com>
Date
Apr 19, 2020, 02:48 UTC
Message-ID
<20200419024805.GC9169@danh.dev>
In-Reply-To
<xmqqk12ctmm1.fsf@gitster.c.googlers.com>
On 2020-04-18 16:12:06-0700, Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
> Martin Ågren <martin.agren@gmail.com> writes:
> 
> > This is equivalent as long as `line->len` is equal to
> > `strlen(line->buf)`, which it will be (should be) because it's a
> > strbuf. Ok.
> 
> For the guarantee to hold true, line->buf[0..line->len] should not
> have any '\0' byte in it.  
> 
> This helper has two callers, but in either case, it needs to be
> prepared to work on output of decode_[bq]_segment().  Is there code
> anywhere that guarntees that the decoding won't stuff '\0' in the
> line?
- First caller: `decode_header`, we don't have such guarantee,
  The old code will discard everything after first NUL characters.
  I'm _not_ really familiar with email standard, does email allow
  UTF-16 (albeit in [bq]-encoded) in header? If yes, the current code
  is likely disallow it. The new code accidentally, accept
  [bq]-encoded-utf-16 header and reencode to utf-8 for commit.
  Yes, it's very likely only UTF-8, ISO-8859-1, some variant of
  ISO-2022 is used nowaday in email.
  *If* we would like to not exclude UTF-16, the new question is should
  we trust the length of newly converted utf-8 string.
- Second caller: `handle_commit_msg`: everything get interesting from
  here.

Get back to the question of trusting the length of newly converted utf-8 string.

I _think_ we should, because I _think_ there shouldn't be any discrimination against any encoding (be it utf-8, ISO-8859-1, etc...), the current code allows NUL character in utf-8 [bq]-encoded string in this function (early return) and its caller, and report an error later:

	error: a NUL byte in commit log message not allowed.

meanwhile, if the email was sent in other encoding, the current code discards everything after NUL in that line, thus silently accept broken commit message.

Attached is the faulty patch in ISO-8859-1, which was used to demonstrate my words. The current code will make a commit with only "Áb" in the body, while the new code rightly claims we have a faulty email.

-- 
Danh


From e27fb59f5a4854efc0badd902673fd636bcab52e Mon Sep 17 00:00:00 2001
From: =?ISO-8859-1?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?=
 <congdanhqx@gmail.com>
Date: Sun, 19 Apr 2020 09:31:22 +0700
Subject: [PATCH] =?ISO-8859-1?q?=C4=CB=D1=CF=D6?=
MIME-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 8bit

�b�d�fg

---
 afile | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 afile

diff --git a/afile b/afile
new file mode 100644
index 0000000000..e69de29bb2
-- 
2.26.1.301.g55bc3eb7cb
Previous: Junio C HamanoNext: Martin Ågren
Message 21 of 39 in “mailinfo.c::convert_to_utf8: reuse strlen info”
  1. mailinfo.c::convert_to_utf8: reuse strlen infoĐoàn Trần Công Danh, Apr 18, 2020
  2. Martin ÅgrenApr 18, 2020
  3. 0/6 strbuf: simplify `strbuf_attach()` usageMartin Ågren, Apr 18, 2020
  4. 1/6 am: use `strbuf_attach()` correctlyMartin Ågren, Apr 18, 2020
  5. 2/6 strbuf_attach: correctly pass in `strlen() + 1` for `alloc`Martin Ågren, Apr 18, 2020
  6. 3/6 strbuf: use `strbuf_attach()` correctlyMartin Ågren, Apr 18, 2020
  7. 4/6 fast-import: avoid awkward use of `strbuf_attach()`Martin Ågren, Apr 18, 2020
  8. 5/6 rerere: avoid awkward use of `strbuf_attach()`Martin Ågren, Apr 18, 2020
  9. 6/6 strbuf: simplify `strbuf_attach()` usageMartin Ågren, Apr 18, 2020
  10. Martin ÅgrenApr 19, 2020
  11. 0/4 strbuf: fix doc for `strbuf_attach()` and avoid itMartin Ågren, Apr 19, 2020
  12. 1/4 strbuf: fix doc for `strbuf_attach()`Martin Ågren, Apr 19, 2020
  13. Junio C HamanoApr 20, 2020
  14. Martin ÅgrenApr 21, 2020
  15. 2/4 strbuf: introduce `strbuf_attachstr_len()`Martin Ågren, Apr 19, 2020
  16. 3/4 strbuf: introduce `strbuf_attachstr()`Martin Ågren, Apr 19, 2020
  17. Junio C HamanoApr 20, 2020
  18. Martin ÅgrenApr 21, 2020
  19. 4/4 strbuf_attach: prefer `strbuf_attachstr_len()`Martin Ågren, Apr 19, 2020
  20. Junio C HamanoApr 18, 2020
  21. Danh DoanApr 19, 2020
  22. Martin ÅgrenApr 19, 2020
  23. Junio C HamanoApr 19, 2020
  24. 0/3 mailinfo: disallow and complains about NUL characterĐoàn Trần Công Danh, Apr 19, 2020
  25. 1/3 t4254: merge 2 steps of a single testĐoàn Trần Công Danh, Apr 19, 2020
  26. Martin ÅgrenApr 19, 2020
  27. Danh DoanApr 19, 2020
  28. 3/3 mailinfo: disallow NUL character in mail's headerĐoàn Trần Công Danh, Apr 19, 2020
  29. Martin ÅgrenApr 19, 2020
  30. Danh DoanApr 19, 2020
  31. 2/3 mailinfo.c::convert_to_utf8: reuse strlen infoĐoàn Trần Công Danh, Apr 19, 2020
  32. Martin ÅgrenApr 19, 2020
  33. Danh DoanApr 19, 2020
  34. Junio C HamanoApr 20, 2020
  35. Danh DoanApr 20, 2020
  36. 0/3 Disallow NUL character in mailinfoĐoàn Trần Công Danh, Apr 20, 2020
  37. 1/3 t4254: merge 2 steps of a single testĐoàn Trần Công Danh, Apr 20, 2020
  38. 2/3 mailinfo.c: avoid strlen on strings that can contains NULĐoàn Trần Công Danh, Apr 20, 2020
  39. 3/3 mailinfo: disallow NUL character in mail's headerĐoàn Trần Công Danh, Apr 20, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.