Re: [Patch size_t V3 19/19] Convert xdiff-interface to size_t

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 17, 2017, 17:49 UTC
Message-ID: <xmqqwp62gk3c.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1502914591-26215-20-git-send-email-martin@mail.zuhause>

Martin Koegler <martin.koegler@chello.at> writes:

Show 13 quoted lines

> diff --git a/combine-diff.c b/combine-diff.c
> index acf39ec..ad5d177 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -343,7 +343,7 @@ struct combine_diff_state {
>  	struct sline *lost_bucket;
>  };
>  
> -static void consume_line(void *state_, char *line, unsigned long len)
> +static void consume_line(void *state_, char *line, size_t len)
>  {
>  	struct combine_diff_state *state = state_;
>  	if (5 < len && !memcmp("@@ -", line, 4)) {

This is a correct logical consequence of making consume_fn to take size_t.

Show 10 quoted lines

> diff --git a/diff.c b/diff.c
> index c12d062..f665f8d 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -406,7 +406,7 @@ static struct diff_tempfile {
>  	struct tempfile tempfile;
>  } diff_temp[2];
>  
> -typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
> +typedef size_t (*sane_truncate_fn)(char *line, size_t len);

It turns out that this type, the member of this type in ecb struct, and a conditional call to the function when it is not NULL, are all unused. If this were used, this conversion is correct ;-)

Show 9 quoted lines

> @@ -4536,7 +4536,7 @@ struct patch_id_t {
>  	int patchlen;
>  };
>  
> -static int remove_space(char *line, int len)
> +static size_t remove_space(char *line, size_t len)
>  {
>  	int i;
>  	char *dst = line;

This function may also want to be rewritten. The loop counter and array index "i" is used this way:

	for (i = 0; i < len i++)
		if (!isspace((c = line[i]))
			*dst++ = c;
	return dst - line;

So you are still risking data loss (later parts of line[] may not be scanned due to int possibly being narrower than size_t).

Turning "len" and the return type into size_t is absolutely the right thing to do, and it is tempting to turn "i" also into size_t, but we may just want to scan the thing with another pointer, i.e.

        size_t remove_space(char *line, size_t len)
        {
                char *src = line;
                char *dst = line;

                while (src < line + len) {
                        char c = *src++;
                        if (!isspace(c))
                                *dst++ = c;
                }
                return dst - line;
        }

Changes to size_t from types other than "ulong" is worth looking at twice for a potential issue like this.

Show 13 quoted lines

> diff --git a/xdiff-interface.c b/xdiff-interface.c
> index d82cd4a..f12285d 100644
> --- a/xdiff-interface.c
> +++ b/xdiff-interface.c
> @@ -26,7 +26,7 @@ static int parse_num(char **cp_p, int *num_p)
>  	return 0;
>  }
>  
> -int parse_hunk_header(char *line, int len,
> +int parse_hunk_header(char *line, size_t len,
>  		      int *ob, int *on,
>  		      int *nb, int *nn)
>  {

This is a correct conversion for the purpose of this series, but the implementation of this function before (and after) this patch is already incorrect. It does not pay any attention to "len", and neither the helper function this function uses, parse_num(), is told how many bytes are remaining on the line to be parsed, so it will happily go beyond "len".

The two things we may want to fix is obviously outside the scope of this series, but they should be fixed (not necessarily by you) after this series once the codebase solidifies, or before this series as preliminary clean-up.

Thanks.