Re: [PATCH v2] utf8: use size_t for string width methods and callee sites.

Hardik Kumar <[email protected]> writes:

> utf8_strwidth() and utf8_strnwidth() return int, even though the
> return value is always non-negative:
>
> - utf8_strnwidth() accumulates the width into a size_t and otherwise
>   returns its size_t len parameter,
> - utf8_strwidth() just forwards its result.
>
> Change their signatures to return size_t instead.
>
> Update the types of the variables the said method is used to avoid
> potential UB caused by implicit conversion from size_t to int.

The goal looks attractive on the surface, and the change to make
utf8_strwidth() and utf8_strnwidth() return 'size_t' clears an
existing TODO.  However, the updates to the call sites to support
this change introduce several bugs due to unsigned integer underflow
and incorrect mixed-sign comparisons.

Consider just one example:

> diff --git a/diff.c b/diff.c
> index 589c196..4887958 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -2952,7 +2952,8 @@ static int utf8_ish_width(const char **start)
>  
>  static void show_stats(struct diffstat_t *data, struct diff_options *options)
>  {
> -	int i, len, add, del, adds = 0, dels = 0;
> +	int i, add, del, adds = 0, dels = 0;
> +	size_t len;
>  	uintmax_t max_change = 0, max_len = 0;
>  	int total_files = data->nr, count;
>  	int width, name_width, graph_width, number_width = 0, bin_width = 0;

The above change impacts code later in the function (among other
things):

		/*
		 * "scale" the filename
		 */
		len = name_width;
		name_len = utf8_strwidth(name);
		if (name_width < name_len) {
			char *slash;
			prefix = "...";
			len -= 3;
			if (len < 0)
				len = 0;

Here, 'len' used to be an 'int', but now it is 'size_t', which is
unsigned.  The safeguard to prevent 'len' from going down to an
unacceptably low value by clipping it to 0 never triggers, because
'if (len < 0)' can never be true.  If len is less than 3, len -= 3
will result in a fairly large value, and the subsequent computation
would go bananas to see a value with little relation to name_len.

Another example.

> diff --git a/pretty.c b/pretty.c
> index d8a9f37..f7d392d 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1805,11 +1805,12 @@ static size_t format_and_pad_commit(struct strbuf *sb, /* in UTF-8 */
>  {
>  	struct strbuf local_sb = STRBUF_INIT;
>  	size_t total_consumed = 0;
> -	int len, padding = c->padding;
> +	int padding = c->padding;
> +	size_t len;
>  
>  	if (padding < 0) {
>  		const char *start = strrchr(sb->buf, '\n');
> -		int occupied;
> +		size_t occupied;
>  		if (!start)
>  			start = sb->buf;
>  		occupied = utf8_strnwidth(start, strlen(start), 1);

After this post-context, 'occupied' is incremented, and then we have
this:

		padding = (-padding) - occupied;

If 'occupied' is sufficiently large, 'padding' can become negative
here.  Because padding remains an 'int' and can become negative, it
impacts code a bit further down in the same function (among other
similar comparisons):

	if (c->flush_type == flush_left_and_steal) {
		const char *ch = sb->buf + sb->len - 1;
		while (len > padding && ch > sb->buf) {
			const char *p;
			if (*ch == ' ') {
				ch--;
				padding++;
				continue;
			}

We compare 'len' and 'padding', first promoting 'padding' to
'size_t', so when 'padding' is negative, we compare 'len' with a
fairly large number due to unsigned wraparound.  We will fail to
"steal" spaces as we will not loop here.

I will stop here.  What makes reviewing this change so unpleasant is
that on the surface, changing variable definitions to flip int to
size_t looks pretty, yet the real breakage appears in places that
are not shown in the patch at all.

So, this needs more work to become acceptable, I am afraid.