From: Stefan Sperling <stsp@stsp.name>
Subject: Re: handle NULL argument to got_diffreg_result_free
To: James Cook <falsifian@falsifian.org>
Cc: gameoftrees@openbsd.org
Date: Mon, 18 Sep 2023 09:03:31 +0200

On Mon, Sep 18, 2023 at 01:46:57AM +0000, James Cook wrote:
> 1. Commit a large file (mine was >1G).
> 2. Change the file.
> 3. got add the file
> 4. See "Segmentation fault (core dumped)"
> 
> This patch fixes the segfault, and also deletes two functions which are no
> longer used since commit cb7c1b68 "remove unused function
> got_diff_blob_prepared_file()".
> 
> As far as I can tell, what was happening was:
> - get_modified_file_content_status calls got_diff_files
>   (lib/worktree.c:1576).
> - got_diff_files returns an error, so get_modified_file_content_status
>   jumps to "done".
> - get_modified_file_content_status tries to call
>   got_diffreg_result_free(diffreg_result) but diffreg_result is NULL.
> - got_diffreg_result_free assumes the pointer passed to it can be
>   dereferenced. This patch changes that.

Yes, thanks. This is a good fix.

> I don't understand why "got add" needs to do a diff, but I
> don't have time to dig into that now. As far as I know, it should just
> ignore the file since it's already added.

In order to arrive at a decision about what to do with a file we have to
determine the file's current status in the first place, which is why
'got add' ends up here.

The diff happens because your file is in M (modified) status and we run
diff in order to see if M may be upgraded to C (conflict).
This case is not intended for 'got add' in particular but we just call
the generic get_file_status() function from anywhere. We could add a flag
that indicates the operation being run to perhaps short-cut some checks.
But I cannot tell if that is worth the extra complexity.

> diff /home/falsifian/co/got
> commit - ef6234452a17eb17687612d9bd417ded3bf6802d
> path + /home/falsifian/co/got
> blob - 68471e1487ff2ce6b1a6a73c6b425eb0a580341f
> file + lib/diffreg.c
> --- lib/diffreg.c
> +++ lib/diffreg.c
> @@ -327,6 +327,9 @@ got_diffreg_result_free(struct got_diffreg_result *dif
>  {
>  	const struct got_error *err;

Add a blank line here, please.
With that, ok stsp.

> +	if (diffreg_result == NULL)
> +		return NULL;
> +
>  	diff_result_free(diffreg_result->result);
>  	diff_data_free(&diffreg_result->left);
>  	diff_data_free(&diffreg_result->right);
> @@ -335,21 +338,3 @@ got_diffreg_result_free(struct got_diffreg_result *dif
>  	free(diffreg_result);
>  	return err;
>  }
> -
> -const struct got_error *
> -got_diffreg_result_free_left(struct got_diffreg_result *diffreg_result)
> -{
> -	diff_data_free(&diffreg_result->left);
> -	memset(&diffreg_result->left, 0, sizeof(diffreg_result->left));
> -	return got_diffreg_close(diffreg_result->map1, diffreg_result->size1,
> -	    NULL, 0);
> -}
> -
> -const struct got_error *
> -got_diffreg_result_free_right(struct got_diffreg_result *diffreg_result)
> -{
> -	diff_data_free(&diffreg_result->right);
> -	memset(&diffreg_result->right, 0, sizeof(diffreg_result->right));
> -	return got_diffreg_close(NULL, 0, diffreg_result->map2,
> -	    diffreg_result->size2);
> -}
> blob - 8aae9cbb204baa3c88860e324853c5f81cc36b56
> file + lib/got_lib_diff.h
> --- lib/got_lib_diff.h
> +++ lib/got_lib_diff.h
> @@ -48,10 +48,6 @@ const struct got_error *got_diffreg_output(struct got_
>      struct got_diffreg_result *, int, int, const char *, const char *,
>      enum got_diff_output_format, int, FILE *);
>  const struct got_error *got_diffreg_result_free(struct got_diffreg_result *);
> -const struct got_error *got_diffreg_result_free_left(
> -    struct got_diffreg_result *);
> -const struct got_error *got_diffreg_result_free_right(
> -    struct got_diffreg_result *);
>  const struct got_error *got_diffreg_close(char *, size_t, char *, size_t);
>  const struct got_error *got_merge_diff3(int *, int, FILE *, FILE *, FILE *,
> 
>