patchutf8.c: fix strbuf_utf8_replace copying the last NUL to dst string

When utf8_width(&src) is called with *src == NULL (because the source string ends with an ansi sequence), it returns 0 and steps 'src' by one. This stepping makes strbuf_utf8_replace add NUL to the destination string at the end of the loop. Check and break the loop early.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 utf8.c | 2 ++
 1 file changed, 2 insertions(+)

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

I am not sure what you mean by "because" here. Do you mean somebody (who?) decides to call the utf8_width() with NULL pointer stored in *src because of "ansi sequence"?

What do you mean by "ansi sequence"? I'll assume that you mean those terminal control that all use bytes with hi-bit clear.

At the very beginning of utf8_width(), *start can be cleared to point at a NULL pointer by pick_one_utf8_char() if the pointer that comes into utf8_width() originally points at an invalid UTF-8 string, but as far as I can see, ESC (or any bytes that would be used in those terminal control sequences like colors and cursor control) will simply be returned as a single byte, without going into error path that clears *start = NULL.

Puzzled...

Here "it" refers to utf8_width()?  Who steps 'src' by one?

Ahh, did you mean *src == NUL, i.e. "already at the end of the string"?

I think utf8_width() called with an empty string should not move the pointer past that end-of-string NUL in the first place. It makes me wonder if it would be a better fix to make it not to do that (and return 0), but if we declare it is the caller's fault, perhaps we may want to add

	if (!**start)
        	die("BUG: empty string to utf8_width()???");

at the very beginning of utf8_width(), even before it calls pick-one-utf8-char.

Still puzzled...

Tests?

On Tue, Jul 29, 2014 at 12:56:24PM -0700, Junio C Hamano wrote:

OK let's try with an example, strbuf_utf8_replace(&sb, 0, 0, "") where "sb" contains "\033[m". The expected result is exactly the same string with length of 3. After this block

		while ((n = display_mode_esc_sequence_len(src))) {
			memcpy(dst, src, n);
			src += n;
			dst += n;
		}

src will be right after 'm', *src is NUL (iow. src == end). After

		n = utf8_width((const char**)&src, NULL);

n is zero but 'src' is advanced by another character, so src - old_src == 1. This NUL character is counted as part of the string, so after the _setlen, we have the same string with length of _4_ (instead of 3).

utf8_width() steps 'src'.

Yes.. I guess you have a better commit message prepared for me now :)

I don't know. I mean, in a UTF-8 byte sequence, NUL is a valid character (part of the ASCII subset), so advancing '*start' by one makes sense. Whether we have any call sites crazy enough to do that on purpose is a different matter.

Ugh.. this was triggered by one of the alignment specifier in log --pretty. Probably easier to export strbuf_utf8_replace() as a test-command and verify the output?

-- Duy

Duy Nguyen <pclouds@gmail.com> writes:

Heh.  At least you realized that the log message was uninformative ;-)

Dubious. NUL may be valid but it is valid only in the sense that it will mark the end of the string, i.e. the caller is expected to do:

        const char *string = ...;

        while (not reached the end of the string) {
                this_char = pick_one_char(&string);
                do something to this_char;
        }

and there are two ways to structure such a loop:

 (1) Make pick-one-char signal the termination condition.  i.e.

	while ((this_char = pick_one(&string)) != EOF)
		do something to this_char;

     That's a typical getchar()-style loop.  It would better not
     advance string when it returns EOF.

 (2) Make it the responsibility of the caller not to call beyond the
     end. i.e.

	while (string < end) {
        	this_char = pick_one_char(&string);
		do something to this char;
	}

and your patch takes the latter, which I think is in line with the way how existing callchain works. So the addition of "BUG" merely is to make sure that everybody follows that same convention.

We cannot declare that it is caller's responsibility to feed sensible input to the function only at one callsite and allow other callsites feed garbage to the same function, after all, no?

The main loop in strbuf_utf8_replace() could summed up as:

  while ('src' is still valid) {
    1) advance 'src' to copy ANSI escape sequences
    2) advance 'src' to copy/replace visible characters
  }

The problem is after #1, 'src' may have reached the end of the string (so 'src' points to NUL) and #2 will continue to copy that NUL as if it's a normal character. Because the output is stored in a strbuf, this NULL accounted in the 'len' field as well. Check after #1 and break the loop if necessary.

The test does not look obvious, but the combination of %>>() should make a call trace like this

  show_log()
  pretty_print_commit()
  format_commit_message()
  strbuf_expand()
  format_commit_item()
  format_and_pad_commit()
  strbuf_utf8_replace()

where %C(auto)%d would insert a color reset escape sequence in the end of the string given to strbuf_utf8_replace() and show_log() uses fwrite() to send everything to stdout (including the incorrect NUL inserted by strbuf_utf8_replace)

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t4205-log-pretty-formats.sh | 7 +++++++
 utf8.c                        | 3 +++
 2 files changed, 10 insertions(+)