patchmailinfo.c::convert_to_utf8: reuse strlen info

We're passing buffer from strbuf to reencode_string, which will call strlen(3) on that buffer, and discard the length of newly created buffer.

Then, we compute the length of the return buffer to attach to strbuf.

We can do better by reusing all the available information, and call the underlying lower level function that will be called indirectly by reencode_string.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 mailinfo.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Hi Danh (I think that's how you want to be addressed?),

On Sat, 18 Apr 2020 at 06:00, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

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.

This conversion is ok as such. I wondered why we pass in the same value twice (before and after this patch). Turns out this usage is wrong (as per the documentation in strbuf.h) but safe (as per my understanding of the implementation in strbuf.c). I'll follow up with a series that fell out of that investigation.

All in all, this conversion is correct and it doesn't leave the use of `strbuf_attach()` any less correct than it already was.

Martin

On Sat, 18 Apr 2020 at 21:56, Martin Ågren <martin.agren@gmail.com> wrote:

Here's that series. It could go in parallel to Danh's, or one could go on top of the other. Any of those would be ok with me.

I think `strbuf_attach()` is sufficiently hard to use that we might as well simplify it for the majority of the users that don't care about the distinction between the string's length and the size of the allocated memory, and avoid it for the few remaining users that are just as well off using `strbuf_add()`.

The summary is that this function takes `len` and `alloc`, where the latter must be greater than the former, yet several callers use the same value for both. I first thought this could cause quite hairy problems such as writing outside of allocated memory, but it doesn't look that way. See the patches for more information.

An alternative to the approach taken here would be to introduce `strbuf_attachstr()` and convert most existing users, then convert the few remaining ones to use the new function or to move in another direction. But the new name is a downer -- what else would you attach to a strbuf if not a string?

Another alternative would be to first convert certain users away from the function, then simplify it for the remainder. I preferred to first spend a few patches converting a few existing callers to make it clear that those are ok.

Martin

Martin Ågren (6):
  am: use `strbuf_attach()` correctly
  strbuf_attach: correctly pass in `strlen() + 1` for `alloc`
  strbuf: use `strbuf_attach()` correctly
  fast-import: avoid awkward use of `strbuf_attach()`
  rerere: avoid awkward use of `strbuf_attach()`
  strbuf: simplify `strbuf_attach()` usage

 strbuf.h             | 16 ++++++++--------
 apply.c              |  2 +-
 archive.c            |  2 +-
 blame.c              |  2 +-
 builtin/am.c         |  2 +-
 convert.c            |  4 ++--
 fast-import.c        |  7 ++++---
 imap-send.c          |  2 +-
 mailinfo.c           |  2 +-
 merge-recursive.c    |  2 +-
 path.c               |  3 +--
 pretty.c             |  4 ++--
 refs/files-backend.c |  2 +-
 rerere.c             |  3 ++-
 strbuf.c             | 11 +++++++----
 trailer.c            |  2 +-
 16 files changed, 35 insertions(+), 31 deletions(-)

Among other parameters, `strbuf_attach()` takes a length and an amount of allocated memory. In strbuf.h, it is documented that the latter "must be larger than the string length, because the string you pass is supposed to be a NUL-terminated string".

In builtin/am.c, we simply pass in the length of the string twice.

My first assumption was that we'd end up with `alloc == len` and that,
e.g., a subsequent `strbuf_avail(sb)` would evaluate `sb->alloc
- sb->len - 1`, resulting in a huge return value, which could be quite
bad. But luckily, we end up in `strbuf_grow()` where we reallocate a
larger buffer, after which we reinstate a '\0' and everything is fine.

One might ask if the function was documented incorrectly in dd613e6b87 ("Strbuf documentation: document most functions", 2008-06-04), but on the other hand, one really has to wonder whether it's actually useful to be able to pass in `alloc == len` only to end up performing the allocation, copying and freeing which this function very much looks like it would keep us from having to do.

Pass in a value one greater than the length for the `alloc` parameter. The string has been allocated correctly using the strbuf machinery in `read_commit_msg()` and we really do have an extra byte at the end with a NUL. This means both that the buffer is as large as we claim it to be and that the addition is safe.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/am.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

This is similar to the previous commit where we corrected a use of `strbuf_attach()` to not pass in the same value for `len` and `alloc`. The sites addressed in this commit all use `strlen()`, which makes it obvious that we're working with NUL-terminated strings and that we can safely switch to providing `len + 1` for `alloc`.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 mailinfo.c           | 2 +-
 path.c               | 2 +-
 refs/files-backend.c | 2 +-
 trailer.c            | 3 ++-
 4 files changed, 5 insertions(+), 4 deletions(-)

Similar to earlier commits, don't pass in the same value for `len` and `alloc` to `strbuf_attach()`. This string comes from `reencode_string_iconv()` which ensures we have an extra byte at the end.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 strbuf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

As explained in an earlier commit, per the documentation of `strbuf_attach()`, it is incorrect to pass in the same value for `alloc` as we do for `len`. But, and this was also explained earlier, doing so is still ok-ish because we'll end up allocating a large enough buffer under the hood.

But then one really has to wonder whether

  strbuf_attach(&sb, buf, size, size);

is any better than

  strbuf_reset(&sb);
  strbuf_add(&sb, buf, size);
  free(buf);

The latter is certainly a lot less subtle about what is going on, and if we're lucky, the strbuf's allocated buffer is large enough that we won't even need to allocate. So let's change to this more explicit form.

In short, this commit should not make things any worse.

Nearby commits are changing other callsites to pass in a larger 'alloc` parameter. Maybe that's safe here, too -- I admit I don't quite follow where this memory comes from. In the future, we could possibly switch back to `strbuf_attach()` here after looking into the allocations in more detail. The immediate reason for this commit is that we want to simplify the usage of `strbuf_attach()`, and we won't be able to pass in "size, size" any more.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 fast-import.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Similar to the previous commit, avoid passing `strbuf_attach()` the same value for `len` and `alloc` by switching to `strbuf_add()`. This avoids a subtle allocate+copy+free dance in favour of a much more obvious one.

Unlike in the previous commit, for this site, I know that it's not safe to pass in "len, len + 1". Trying that makes `strbuf_attach()` write a trailing '\0' at `result.ptr[len]`. Running our test suite with AddressSanitizer will spot an out-of-bounds write due to this.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 rerere.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

The last few commits have fixed a decent number of call sites that pass in the same value for `alloc` and `len` when attaching a string to a strbuf, despite the documentation being quite clear about this not being ok. As a result, all users currently pass in `len + 1`.

Because this interface was used incorrectly (albeit not necessarily insecurely) before, simplify it by making `strbuf_attach()` simply assume that the allocated length is exactly `len + 1`. If and when we do gain one or two call sites that know better, we can either keep on using this pessimistic assumption and it might not actually hurt much, or we could give them a dedicated function similar to the current more flexible (and proven error-prone) `strbuf_attach()`.

This means we can drop the call to `strbuf_grow()`. It really only served to do the alloc+copy+free dance that we were supposedly avoiding.

That said, we should consider the edge case of `buf == NULL`. Let's check for this and BUG. Even for a zero-length string, you need somewhere to stick the trailing NUL.

(Also: If someone used to pass `alloc == len` for the case `len == 0`, we used to end up leaking `buf` on the assumption that we were using `strbuf_slopbuf`. Not the biggest leak ever, but still.)

Finally, one might wonder if we could go further and make `strbuf_attach()` call `strlen()`. Some call sites wouldn't mind, but others do have the length already and we might as well use it. Those that don't have the length will figure it out as they call this function. Before this commit, they'd do two calls to `strlen()` so they either didn't care much or they trusted the compiler to optimize for them. They're not any worse off now.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 strbuf.h             | 16 ++++++++--------
 apply.c              |  2 +-
 archive.c            |  2 +-
 blame.c              |  2 +-
 builtin/am.c         |  2 +-
 convert.c            |  4 ++--
 imap-send.c          |  2 +-
 mailinfo.c           |  2 +-
 merge-recursive.c    |  2 +-
 path.c               |  3 +--
 pretty.c             |  4 ++--
 refs/files-backend.c |  2 +-
 strbuf.c             | 11 +++++++----
 trailer.c            |  3 +--
 14 files changed, 29 insertions(+), 28 deletions(-)

On Sat, 18 Apr 2020 at 22:18, Martin Ågren <martin.agren@gmail.com> wrote:

I realize now that some of the reasoning in this series is incorrect. `strbuf_grow()` will use `ALLOC_GROW` meaning we might reallocate cheaply rather than doing an entire allocate+copy+free cycle.

Still, there is some buggyness in the usage because we might reallocate (with a 50% overhead) even when we shouldn't really need to.

I'll resubmit this after tackling it from a different angle: I'll introduce `strbuf_attachstr()`, simplifying and robustifyng most users. Then I'll switch a few incorrect users of `strbuf_attach()` to `strbuf_attachstr()`. Then a small number of users will continue using `strbuf_attach()` with `alloc == len` for $reasons.

So this is what I'll do instead. The reasoning is, you can attach a string (a NUL-terminated buffer) or a non-string (non-NUL-terminated buffer).

Martin

This is v2 of my attempt to look into how `strbuf_attach()` works, how it's documented and how it's actually used.

This is based on top of Danh's series [1]. Both their patch 2/3 and my 4/4 reason about "NUL" at a particular spot. In short, they conflict about whether to use function "A" or "B" at one spot and a merge should (in my opinion) use function "C". That seemed odd enough to me that I decided to build on top instead.

`strbuf_attach()` has always handled `mem == len`. This was intentional. When the function was eventually documented, this usage was incorrectly documented as being wrong.

That documentation didn't stop lots of callers from using it "incorrectly", though. I first fix the documentation. I then try to avoid such usage anyway, partly because it can be confusing, partly because if we know we have enough memory, there's no need to reallocate a larger buffer.

Martin

[1] https://lore.kernel.org/git/cover.1587289680.git.congdanhqx@gmail.com/

Martin Ågren (4):
  strbuf: fix doc for `strbuf_attach()`
  strbuf: introduce `strbuf_attachstr_len()`
  strbuf: introduce `strbuf_attachstr()`
  strbuf_attach: prefer `strbuf_attachstr_len()`

 strbuf.h             | 40 ++++++++++++++++++++++++++++++++++++----
 apply.c              |  2 +-
 archive.c            |  2 +-
 blame.c              |  2 +-
 builtin/am.c         |  2 +-
 convert.c            |  4 ++--
 fast-import.c        |  2 +-
 imap-send.c          |  2 +-
 mailinfo.c           |  2 +-
 merge-recursive.c    |  2 +-
 path.c               |  3 +--
 pretty.c             |  4 ++--
 refs/files-backend.c |  3 +--
 trailer.c            |  2 +-
 14 files changed, 51 insertions(+), 21 deletions(-)

Commit 917c9a7133 ("New strbuf APIs: splice and attach.", 2007-09-15) added `strbuf_attach()`. In the commit message, it is explicitly discussed how using `mem == len` is ok, although possibly costly. When this function was documented in dd613e6b87 ("Strbuf documentation: document most functions", 2008-06-04), this distinction was missed and the documentation simply forbids this case.

The function handles reallocation and truncation, yet the docs say that the "amount [of allocated memory] must be larger than the string length, because the string you pass is supposed to be a NUL-terminated string".

Several callers pass in `mem == len` meaning they are compatible with the implementation and the original commit message, but not with the documented behavior. In fact, compared to the documentation, they look outright dangerous.

Later commits will convert most of those call sites to use new, simpler interfaces, but in at least one instance we really do want to use this possibility of passing the same value twice: In rerere.c, it is not safe to pass in "len, len + 1". Doing so makes `strbuf_attach()` write a trailing NUL at `result.ptr[len]` and running our test suite with AddressSanitizer will spot an out-of-bounds write due to this.

That is, rerere.c really has use for the flexibility that `mem == len` gives us. Let's update the documentation to clarify that this is ok.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 strbuf.h | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Martin Ågren <martin.agren@gmail.com> writes:

In other words, mem==len implies that the original lacks the terminating '\0', so attach needs to reallocate from the get go, so discouraging such use may make sense, but it is too strong to forbid it, as the strbuf invariant is held safely? If so, the description makes sense to me.

IOW, _attach() does not mind if the original lacks '\0' at the end.

NUL-truncated? Ah, if mem and len are the same, the string is reallocated to fit an extra byte to NUL-terminate, to make sure strlen(sb->buf)==len holds. Makes sense.

That's a good thing to explain.

On Mon, 20 Apr 2020 at 19:30, Junio C Hamano <gitster@pobox.com> wrote:

Right, nor if it lacks the space for it.

Exactly. NUL-terminated would be better. I think I'll split that last sentence and replace it with something like the following:

  If the two lengths are equal, reallocation will be handled as
  needed. And regardless, the string will be NUL-terminated at `len`.

(One might still have strlen(sb->buf) < len though. This just guarantees "<=")

Martin

Most callers of `strbuf_attach()` provide `len + 1` for the `mem` parameter. That's a bit tedious and, as we will see in the next commit, also fairly prone to mistakes.

Provide `strbuf_attachstr_len()` for this common case to simplify several callers of `strbuf_attach()` by making this new function simply assume that the size of the allocated buffer is one greater than the length.

We know that the string has already been allocated with space for the trailing NUL, meaning it is safe to compute `len + 1`.

Disallow NULL-pointers entirely. We could handle `(buf, len) == (NULL, 0)` specially, but none of the callers we convert here seem to worry about such a case. Handling this corner case specially can still be done using the regular `strbuf_attach()`.

Another edge case is where someone doesn't have a NUL at `buf[len]`, i.e., they are (hopefully) trying to attach a substring of some larger string. One could argue that they should be using `strbuf_attach()` and that this is BUG-worthy, or that it would be easy enough for us to place a NUL there for robustness and carry on. This commit does the latter, but does not have a strong opinion about it.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 strbuf.h          | 19 +++++++++++++++++++
 apply.c           |  2 +-
 archive.c         |  2 +-
 blame.c           |  2 +-
 convert.c         |  4 ++--
 imap-send.c       |  2 +-
 merge-recursive.c |  2 +-
 pretty.c          |  2 +-
 8 files changed, 27 insertions(+), 8 deletions(-)

Similar to the previous commit, introduce `strbuf_attachstr()` where we don't even have to pass in the length of the string that we want to attach. Convert existing callers of `strbuf_attachstr()` that use `strlen()`.

Note how only one caller passes in `mem == len + 1` and that the others have been using `strbuf_attach()` in direct contradiction to how it was (incorrectly) documented up until a few commits ago.

Now that the documentation has been fixed, you might say these are all fine. But the calling convention of `strbuf_attach()` seems sufficiently hard to get right that it's probably a good idea to introduce this helper.

This could help reduce reallocations and memory waste. When we pessimistically pass in `strlen(foo)` for `mem`, the strbuf will have `alloc == len` and will do a reallocation, not just to get one more byte for the NUL (which would have been a no-op), but because we're using `ALLOC_GROW` under the hood, we will ask for 16 more bytes and another 50% on top of that.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 strbuf.h             | 11 +++++++++++
 path.c               |  3 +--
 pretty.c             |  2 +-
 refs/files-backend.c |  3 +--
 trailer.c            |  2 +-
 5 files changed, 15 insertions(+), 6 deletions(-)

Martin Ågren <martin.agren@gmail.com> writes:

This is somewhat worrysome in that the interface is _so_ simple that people may fail to see that str must be allocated piece of memory, and it is preferrable if string fully fills the allocation.

We should repeat that (instead of just trusting "similar to ..." would tell them enough) in the doc, perhaps?

This is not wrong per-se, but it is unclear if use of strbuf_attach* family to avoid an explicit malloc/copy/free is buying much at this callsite. Simplifying the code here of course is not within the scope of this series.

On Mon, 20 Apr 2020 at 21:39, Junio C Hamano <gitster@pobox.com> wrote:

Yeah, that's a good point. I'll expand on this to try to better get through that there are things to consider here.

For the other patches in this series, I spent some time and effort investigating where strings came from, "do I really feel certain that they're NUL-terminated?". But for this patch, I more or less went "we've been using strlen on this all this time, surely if it wasn't guaranteed to be NUL-terminated we'd have messed up already". And I don't think I'm making anything worse. But yeah, I didn't really step back to look at what these sites are really doing, and how, as much as I did for the others.

Martin

After the last few commits, we don't have many users of `strbuf_attach()`.

Convert the sites in builtin/am.c, strbuf.c and mailinfo.c. They pass in the same length twice for `len` and `mem` and will eventually hit `realloc(3)`, which will be a no-op. The string in am.c has been constructed using the strbuf machinery in `read_commit_msg()`. In strbuf.c, we've used `reencode_string_iconv()`. In mailinfo.c, we used `reencode_string_len()`. So in all cases, we really do have an extra byte at the end with a NUL.

As explained in the previous commit, it's not just that we avoid calling `realloc()` to make room for a single NUL byte that we already have, we avoid asking it for 16 more bytes and another 50% on top of that.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 builtin/am.c  | 2 +-
 fast-import.c | 2 +-
 mailinfo.c    | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Martin Ågren <martin.agren@gmail.com> writes:

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?

On 2020-04-18 16:12:06-0700, Junio C Hamano <gitster@pobox.com> wrote:

- 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.

On Sun, 19 Apr 2020 at 04:48, Danh Doan <congdanhqx@gmail.com> wrote:

Yes. I sort of assumed it shouldn't ("because strbuf"), but it's a good question. Especially considering this is about various encodings..

This assumption that "strlen is length so the conversion is a no-op" could potentially be broken both for input (line->len) and output (out_len).

My knee-jerk reaction to Junio's question was along the same line: surely if we could have a NUL in there, the current `strlen()` would use it as an excuse to silently truncate the string, either before processing or afterwards. Thanks for looking into that more.

Good find.

Martin

Martin Ågren <martin.agren@gmail.com> writes:

Exactly.

The change introduces a behaviour change, and we should describe it in the log message to help future developers know that we did this change, knowingly that we are changing the behaviour, and believing that the new behaviour is a better one.

Thanks.

On 2020-04-18 22:32:30-0700, Junio C Hamano <gitster@pobox.com> wrote:

Well, I intended to do something else first (because of Thunderbird started with HTML email and replace space with non-breaking-space).

I made this change without thinking that much :( And I weren't sure if this change is well received.

I've rephased the commit message, added some test, and disallow another NUL characters mailinfo.

-------------------8<------------------------

As of current state, Git disallow NUL character in commit message.

This indirectly disallow NUL in the body of commit message that sent by email in UTF-8 encoding.

In those below cases:

* NUL character in header field (be it Subject, Author, Email, etc...)
* NUL in the body of commit message that originally sent in other than UTF-8
  encoding

Git silently accepts them, albeit, truncate at the NUL character.

Those email is clearly not generated by Git, they must be crafted.

Complains loudly about those NUL characters.

Đoàn Trần Công Danh (3):
  t4254: merge 2 steps of a single test
  mailinfo.c::convert_to_utf8: reuse strlen info
  mailinfo: disallow NUL character in mail's header

 mailinfo.c            | 11 +++++++--
 t/t4254-am-corrupt.sh | 52 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 57 insertions(+), 6 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  d1bc31e692 t4254: merge 2 steps of a single test
1:  9ce4b7c350 ! 2:  e3e542d388 mailinfo.c::convert_to_utf8: reuse strlen info
    @@ Commit message
         We're passing buffer from strbuf to reencode_string,
         which will call strlen(3) on that buffer,
         and discard the length of newly created buffer.
    -
         Then, we compute the length of the return buffer to attach to strbuf.
     
    +    During this process, we introduce a discrimination between mail
    +    originally written in utf-8 and other encoding.
    +
    +    * if the email was written in utf-8, we leave it as is. If there is
    +      a NUL character in that line, we complains loudly:
    +
    +            error: a NUL byte in commit log message not allowed.
    +
    +    * if the email was written in other encoding, we truncate the data as
    +      the NUL character in that line, then we used the truncated line for
    +      the metadata.
    +
         We can do better by reusing all the available information,
         and call the underlying lower level function that will be called
    -    indirectly by reencode_string.
    +    indirectly by reencode_string. By doing this, we will also postpone
    +    the NUL character processing to the commit step, which will
    +    complains about the faulty metadata.
     
         Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
    @@ mailinfo.c: static int convert_to_utf8(struct mailinfo *mi,
      	return 0;
      }
      
    +
    + ## t/t4254-am-corrupt.sh ##
    +@@
    + test_description='git am with corrupt input'
    + . ./test-lib.sh
    + 
    ++write_nul_patch() {
    ++	space=' '
    ++	qNUL=
    ++	case "$1" in
    ++		subject) qNUL='=00' ;;
    ++	esac
    ++	cat <<-EOF
    ++	From ec7364544f690c560304f5a5de9428ea3b978b26 Mon Sep 17 00:00:00 2001
    ++	From: A U Thor <author@example.com>
    ++	Date: Sun, 19 Apr 2020 13:42:07 +0700
    ++	Subject: [PATCH] =?ISO-8859-1?q?=C4=CB${qNUL}=D1=CF=D6?=
    ++	MIME-Version: 1.0
    ++	Content-Type: text/plain; charset=ISO-8859-1
    ++	Content-Transfer-Encoding: 8bit
    ++
    ++	EOF
    ++	if test "$1" = body
    ++	then
    ++		printf "%s\0%s\n" abc def
    ++	fi
    ++	cat <<-\EOF
    ++	---
    ++	diff --git a/afile b/afile
    ++	new file mode 100644
    ++	index 0000000000..e69de29bb2
    ++	--$space
    ++	2.26.1
    ++	EOF
    ++}
    ++
    + test_expect_success setup '
    + 	# Note the missing "+++" line:
    + 	cat >bad-patch.diff <<-\EOF &&
    +@@ t/t4254-am-corrupt.sh: test_expect_success 'try to apply corrupted patch' '
    + 	test_i18ncmp expected actual
    + '
    + 
    ++test_expect_success "NUL in commit message's body" '
    ++	test_when_finished "git am --abort" &&
    ++	write_nul_patch body >body.patch &&
    ++	test_must_fail git am body.patch 2>err &&
    ++	grep "a NUL byte in commit log message not allowed" err
    ++'
    ++
    ++test_expect_failure "NUL in commit message's header" '
    ++	test_when_finished "git am --abort" &&
    ++	write_nul_patch subject >subject.patch &&
    ++	test_must_fail git am subject.patch 2>err &&
    ++	grep "a NUL byte in Subject is not allowed" err
    ++'
    ++
    + test_done
-:  ---------- > 3:  cb96947b36 mailinfo: disallow NUL character in mail's header

While we are at it, make sure we run a clean up after testing. In a later patch, we will test for more corrupted patch.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t4254-am-corrupt.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

On Sun, 19 Apr 2020 at 13:03, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

(This comment looks a bit unnecessary, but that's not new.)

Makes sense. The first mini-test used to just set up "actual" and there was only one test using it, so it's hard to argue it was a "setup" phase. This looks better.

Martin

On 2020-04-19 14:25:41+0200, Martin Ågren <martin.agren@gmail.com> wrote:

I haven't looked very hard into this part, I guess I could take more time to look at it and decide if we should remove it or not.

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(-)

On Sun, 19 Apr 2020 at 13:05, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

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:

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.

We're passing buffer from strbuf to reencode_string, which will call strlen(3) on that buffer, and discard the length of newly created buffer. Then, we compute the length of the return buffer to attach to strbuf.

During this process, we introduce a discrimination between mail originally written in utf-8 and other encoding.

* if the email was written in utf-8, we leave it as is. If there is
  a NUL character in that line, we complains loudly:

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

* if the email was written in other encoding, we truncate the data as
  the NUL character in that line, then we used the truncated line for
  the metadata.

We can do better by reusing all the available information, and call the underlying lower level function that will be called indirectly by reencode_string. By doing this, we will also postpone the NUL character processing to the commit step, which will complains about the faulty metadata.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 mailinfo.c            |  6 ++++--
 t/t4254-am-corrupt.sh | 44 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

On Sun, 19 Apr 2020 at 13:03, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

This makes sense to me.

I think the subject could be adapted though? Now it's not about "reusing info" anymore, it's about using *other*, *better* info. Maybe

  mailinfo.c: avoid strlen on strings that might contain NUL

? Could probably be improved further..

Same diff as before, ok.

Here it's case/esac...

Here it's if/fi. Looks a bit inconsistent.

I suppose you tried to have a case for "body" above but couldn't get it to work? Somehow, it would seem more consistent to have a qSubject and qBody and handle them the same way, but maybe that's not possible? Maybe using q_to_nul to create qBody containing a NUL?

I think this helper function should use &&-chaining.

Makes sense.

Interesting. :-)

Martin

On 2020-04-19 14:29:06+0200, Martin Ågren <martin.agren@gmail.com> wrote:

This is better than mine. I couldn't think about any other alternative.

Those patch was written in different time, with different thought in mind.

I need to send a re-roll to update the subject and moving oneline in test code to later patch.

I'll update this hunk with that re-roll. Your new series _won't_ be affected, though.

Correct.

Going through the mail now make me think about moving the last grep to next patch in order to clearly indicate that we expect "git-am" is failling but it's passed instead.

And that message isn't introduced until the very next change.

Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

Style: SP on both sides of (), i.e.

	write_nul_patch () {

But isn't this misnamed? You are interested in injecting '\0' byte in the e-mail headers and bodies, not necessarily part of the patch, but "nul-patch" somehow hints readers that we are writing out a Null Patch (something that does not do anything, perhaps?).

sample_mbox_with_nul is the best alternative I can come up with offhand, which is not great either, but at least it does not say patch.

Style: case/esac aligns with case arms, i.e.

	case "$1" in
	subject) qNUL='=00' ;;
	esac

Since the above does have ${qNUL} interpolated, not quoting <<-EOF is correct. Good.

OK. So we won't be able to inject NUL byte in both header and body at the same time. If you wanted to allow it, you could write

	case ",$1," in
	*,subject,*)	qNUL="=00" ;;
	esac

in the early part, and then rewrite this one like so:

	case ",$1," in
	*,body,*)	printf "..." ;;
	esac

Then those callers who want to ask for both can say

	sample_mbox_with_nul subject,body

Doesn't this want to interpolate $space in the output? I think you want to say <<-EOF, without quoting.

    cd t && sh t4254-am-corrupt.sh -d && cat trash*.t4254-*/body.patch

tells me that "--$space" is left in the output, not "-- ".

On 2020-04-20 12:59:37-0700, Junio C Hamano <gitster@pobox.com> wrote:

Originally, this function was written to create a file named "nul.patch", but it's prohibited in Windows land.

It's still misnamed, though.

I prefer having a verb, but make_sample_mbox_with_nul is too long. I'll take make_mbox_with_nul. Naming is hard.

I recalled it now. Originially, I wrote "-- " in that line, When I try git-am(1) the mail, I saw a warning about trailing space. I want to get rid of it but forget to change "-\EOF"

Those last 2 lines isn't strictly required, I wanted to mimic a real patch created by git-format-patch, though.

As of current state, Git disallow NUL character in commit message.

This indirectly disallow NUL in the body of commit message that sent by email in UTF-8 encoding.

In those below cases:

* NUL character in header field (be it Subject, Author, Email, etc...)
* NUL in the body of commit message that originally sent in other than UTF-8
  encoding

Git silently accepts them, albeit, truncate at the NUL character.

Those email is clearly not generated by Git, they must be crafted.

Complains loudly about those NUL characters.

Change from v2:

* reword 2/3 commit message
* rename helper for crafting faulty patch, use single heredoc,
  and allow quoted NUL in both subject and body
* postpone a grep in test for NUL in subject to clearly indicate that git-am
  is failing instead of wrong error message.
* quote the header that contains NUL in the error message.

Đoàn Trần Công Danh (3):
  t4254: merge 2 steps of a single test
  mailinfo.c: avoid strlen on strings that can contains NUL
  mailinfo: disallow NUL character in mail's header

 mailinfo.c            | 11 +++++++--
 t/t4254-am-corrupt.sh | 53 +++++++++++++++++++++++++++++++++++++++----
 2 files changed, 58 insertions(+), 6 deletions(-)

Range-diff against v2:
1:  d1bc31e692 = 1:  d1bc31e692 t4254: merge 2 steps of a single test
2:  e3e542d388 ! 2:  97ede4aab2 mailinfo.c::convert_to_utf8: reuse strlen info
    @@ Metadata
     Author: Đoàn Trần Công Danh <congdanhqx@gmail.com>
     
      ## Commit message ##
    -    mailinfo.c::convert_to_utf8: reuse strlen info
    +    mailinfo.c: avoid strlen on strings that can contains NUL
     
         We're passing buffer from strbuf to reencode_string,
         which will call strlen(3) on that buffer,
    @@ t/t4254-am-corrupt.sh
      test_description='git am with corrupt input'
      . ./test-lib.sh
      
    -+write_nul_patch() {
    ++make_mbox_with_nul () {
     +	space=' '
    -+	qNUL=
    -+	case "$1" in
    -+		subject) qNUL='=00' ;;
    -+	esac
    ++	q_nul_in_subject=
    ++	q_nul_in_body=
    ++	while test $# -ne 0
    ++	do
    ++		case "$1" in
    ++		subject) q_nul_in_subject='=00' ;;
    ++		body)    q_nul_in_body='=00' ;;
    ++		esac &&
    ++		shift
    ++	done &&
     +	cat <<-EOF
     +	From ec7364544f690c560304f5a5de9428ea3b978b26 Mon Sep 17 00:00:00 2001
     +	From: A U Thor <author@example.com>
     +	Date: Sun, 19 Apr 2020 13:42:07 +0700
    -+	Subject: [PATCH] =?ISO-8859-1?q?=C4=CB${qNUL}=D1=CF=D6?=
    ++	Subject: [PATCH] =?ISO-8859-1?q?=C4=CB${q_nul_in_subject}=D1=CF=D6?=
     +	MIME-Version: 1.0
     +	Content-Type: text/plain; charset=ISO-8859-1
    -+	Content-Transfer-Encoding: 8bit
    ++	Content-Transfer-Encoding: quoted-printable
     +
    -+	EOF
    -+	if test "$1" = body
    -+	then
    -+		printf "%s\0%s\n" abc def
    -+	fi
    -+	cat <<-\EOF
    ++	abc${q_nul_in_body}def
     +	---
     +	diff --git a/afile b/afile
     +	new file mode 100644
    @@ t/t4254-am-corrupt.sh: test_expect_success 'try to apply corrupted patch' '
      
     +test_expect_success "NUL in commit message's body" '
     +	test_when_finished "git am --abort" &&
    -+	write_nul_patch body >body.patch &&
    ++	make_mbox_with_nul body >body.patch &&
     +	test_must_fail git am body.patch 2>err &&
     +	grep "a NUL byte in commit log message not allowed" err
     +'
     +
    -+test_expect_failure "NUL in commit message's header" '
    -+	test_when_finished "git am --abort" &&
    -+	write_nul_patch subject >subject.patch &&
    -+	test_must_fail git am subject.patch 2>err &&
    -+	grep "a NUL byte in Subject is not allowed" err
    -+'
    ++test_expect_failure "NUL in commit message's header" "
    ++	test_when_finished 'git am --abort' &&
    ++	make_mbox_with_nul subject >subject.patch &&
    ++	test_must_fail git am subject.patch
    ++"
     +
      test_done
3:  cb96947b36 ! 3:  b2e760227e mailinfo: disallow NUL character in mail's header
    @@ mailinfo.c: static void handle_info(struct mailinfo *mi)
      			continue;
      
     +		if (memchr(hdr->buf, '\0', hdr->len)) {
    -+			error("a NUL byte in %s is not allowed.", header[i]);
    ++			error("a NUL byte in '%s' is not allowed.", header[i]);
     +			mi->input_error = -1;
     +		}
     +
    @@ t/t4254-am-corrupt.sh: 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_expect_failure "NUL in commit message's header" "
    ++test_expect_success "NUL in commit message's header" "
    + 	test_when_finished 'git am --abort' &&
    + 	make_mbox_with_nul subject >subject.patch &&
    +-	test_must_fail git am 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
    - '
    ++	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
    + "
    + 
    + test_done

While we are at it, make sure we run a clean up after testing. In a later patch, we will test for more corrupted patch.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 t/t4254-am-corrupt.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

We're passing buffer from strbuf to reencode_string, which will call strlen(3) on that buffer, and discard the length of newly created buffer. Then, we compute the length of the return buffer to attach to strbuf.

During this process, we introduce a discrimination between mail originally written in utf-8 and other encoding.

* if the email was written in utf-8, we leave it as is. If there is
  a NUL character in that line, we complains loudly:

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

* if the email was written in other encoding, we truncate the data as
  the NUL character in that line, then we used the truncated line for
  the metadata.

We can do better by reusing all the available information, and call the underlying lower level function that will be called indirectly by reencode_string. By doing this, we will also postpone the NUL character processing to the commit step, which will complains about the faulty metadata.

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 mailinfo.c            |  6 ++++--
 t/t4254-am-corrupt.sh | 44 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+), 2 deletions(-)

Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 mailinfo.c            | 5 +++++
 t/t4254-am-corrupt.sh | 7 +++++--
 2 files changed, 10 insertions(+), 2 deletions(-)