On 2023/08/23 22:22:49 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> gonzalo found a double-free crash in 'got commit' and provided
> a test case to trigger it. This crash occurs in free_commitable()
> via got_worktree_commit() when it frees ct->path a second time,
> after append_ct_diff() has already freed the same variable.
> 
> #0  0x0000071bf80c213a in free_commitable (ct=0x71ed57d75a0) at \
> 	/home/stsp/src/got/got/../lib/worktree.c:5382
> 5382            free(ct->path);
> 
> This was introduced when I first added the call to append_ct_diff()
> for display of to-be-committed diffs in a temporary file.
> If an error occurs while generating the diff we free a commitable
> which has already been added to the past list.
> First patch below fixes this issue.
> 
> The next patch fixes the reason for the error which was raised
> while generating the diff. This error is a spurious "out of date"
> message which happens because the file index entry for the file
> to be committed is bad. The sequence to trigger this bad state is:
> 
>   got add file
>   rm file
>   got rm -f file
> 
> Gonzalo's test case uses a longer but equivalent sequence:
> 
>   got add file
>   mv file file2
>   got add file2
>   got rm -f file
> 
> 'file' is now in missing status !, after it was in locally added status A.
> When deleting the file we assume that a missing status implies a versioned
> file was removed from disk and mark the file index entry for removal in
> the commit operation. However the entry for 'file' has no base blob and
> no base commit ID since it was never comitted. Instead we should remove
> the file index entry when the file gets removed, effectively reverting
> the local addition of the file (there is no way to detect the former A
> status explicitly, but we can check for the presence of a base-blob).
> 
> This means the following two sequences are now equivalent, which I
> think is fine:
> 
> 1)
>   got add file
>   rm file
>   got rm -f file
> 
> 2)
>   got add file
>   rm file
>   got revert file
> 
> gonzalo provided a test case and these two fixes make it pass.
> 
> Ok?

sorry for the delay.  ok op@

> -----------------------------------------------
>  prevent a double-free in got_worktree_commit
>  
>  If creating the /tmp display diff for a commitable item failed we
>  would free the commitable item while it was already on the path list.
>  Later on when the path list was freed in got_worktree_commit() a
>  double-free would be detected and the program would be aborted.
>  
>  Found by gonzalo@
>  
> diff 808264b2482b61d7bab9f3d7b0b4a9703a3942cd 36cb26ac04e804d2ad0087c13aa868fc7bb18943
> commit - 808264b2482b61d7bab9f3d7b0b4a9703a3942cd
> commit + 36cb26ac04e804d2ad0087c13aa868fc7bb18943
> blob - 2cdafab5a89da7e31a55bc887af42b14226637d8
> blob + bfdd5fc5f4361a2ce5c55a856b02624a840888f5
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -5765,17 +5765,16 @@ collect_commitables(void *arg, unsigned char status,
>  		err = got_error_from_errno("strdup");
>  		goto done;
>  	}
> -	err = got_pathlist_insert(&new, a->commitable_paths, ct->path, ct);
> -	if (err)
> -		goto done;
>  
> -	if (a->diff_outfile && ct && new != NULL) {
> +	if (a->diff_outfile) {
>  		err = append_ct_diff(ct, &a->diff_header_shown,
>  		    a->diff_outfile, a->f1, a->f2, dirfd, de_name,
>  		    a->have_staged_files, a->repo, a->worktree);
>  		if (err)
>  			goto done;
>  	}
> +
> +	err = got_pathlist_insert(&new, a->commitable_paths, ct->path, ct);
>  done:
>  	if (ct && (err || new == NULL))
>  		free_commitable(ct);
> 
> -----------------------------------------------
>  fix deletion of missing locally-added files
>  
>  If a locally added file in A status gets deleted from disk it will
>  move into missing ! status. If the user then decides to delete the
>  file we must remove the file index entry immediately rather than
>  flagging the file for removal during the next commit operation.
>  The file was never committed and lacks a base-blob and base-commit,
>  so it cannot be removed during the next commit. As a result the
>  commit operation was reporting a bogus "out of date" error.
>  
>  Found while diagnosing a related bug reported by gonzalo@
>  
> diff 36cb26ac04e804d2ad0087c13aa868fc7bb18943 2d115df40ac99f40f33e55b5dee279e3ec0ebe67
> commit - 36cb26ac04e804d2ad0087c13aa868fc7bb18943
> commit + 2d115df40ac99f40f33e55b5dee279e3ec0ebe67
> blob - bfdd5fc5f4361a2ce5c55a856b02624a840888f5
> blob + a60bbbeee54da16d7504c7439574f7825bf1625a
> --- lib/worktree.c
> +++ lib/worktree.c
> @@ -4593,7 +4593,10 @@ schedule_for_deletion(void *arg, unsigned char status,
>  		    strlen(ondisk_path), root_len) != 0);
>  	}
>  
> -	got_fileindex_entry_mark_deleted_from_disk(ie);
> +	if (got_fileindex_entry_has_blob(ie))
> +		got_fileindex_entry_mark_deleted_from_disk(ie);
> +	else
> +		got_fileindex_entry_remove(a->fileindex, ie);
>  done:
>  	free(ondisk_path);
>  	if (err)
> 
> -----------------------------------------------
>  add a test case which removes a missing locally-added file
>  
>  Based on a patch by gonzalo@
>  
> diff 2d115df40ac99f40f33e55b5dee279e3ec0ebe67 d55658adcc965468e258c8663c87003bdcd30a4e
> commit - 2d115df40ac99f40f33e55b5dee279e3ec0ebe67
> commit + d55658adcc965468e258c8663c87003bdcd30a4e
> blob - 230e87510d8cfc3bfd45d3bbfc058b91d0a05e6e
> blob + 31567d2fc90dcccd8d23c81bc042414e05d514b1
> --- regress/cmdline/add.sh
> +++ regress/cmdline/add.sh
> @@ -196,6 +196,78 @@ test_add_directory() {
>  	test_done "$testroot" "$ret"
>  }
>  
> +test_add_force_delete_commit() {
> +	local testroot=`test_init add_force_delete_commit`
> +
> +	got checkout $testroot/repo $testroot/wt > /dev/null
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	echo new > $testroot/wt/new
> +
> +	echo -n > $testroot/stdout.expected
> +	(cd $testroot/wt && got add new > $testroot/stdout 2> $testroot/stderr)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got add command failed unexpectedly" >&2
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	(cd $testroot/wt && mv new new2 > $testroot/stdout 2> $testroot/stderr)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "rename the file failed unexpectedly" >&2
> +		ls beta2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +
> +	(cd $testroot/wt && got add new2 > $testroot/stdout 2> $testroot/stderr)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got add  command failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	# File 'new' was once in A status (locally added) but is now
> +	# in "!" (missing) status since it was never committed.
> +	# Removing it effectively reverts the local addition.
> +	(cd $testroot/wt && got remove -f new > $testroot/stdout \
> +		2> $testroot/stderr)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got remove -f command failed unexpectedly" >&2
> +		test_done "$testroot" "1"
> +		return 1
> +	fi
> +
> +	(cd $testroot/wt && got status > $testroot/stdout)
> +
> +	echo 'A  new2' > $testroot/stdout.expected
> +	cmp -s $testroot/stdout.expected $testroot/stdout
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		diff -u $testroot/stdout.expected $testroot/stdout
> +		test_done "$testroot" "$ret"
> +		return 1
> +	fi
> +
> +	(cd $testroot/wt && got commit -m "add force remove commit" \
> +		> $testroot/stdout 2> $testroot/stderr)
> +	ret=$?
> +	if [ $ret -ne 0 ]; then
> +		echo "got commit command failed unexpectedly" >&2
> +	fi
> +	test_done "$testroot" "$ret"
> +}
> +
>  test_add_directory() {
>  	local testroot=`test_init add_directory`
>  
> @@ -419,6 +491,7 @@ run_test test_add_directory
>  run_test test_add_multiple
>  run_test test_add_file_in_new_subdir
>  run_test test_add_deleted
> +run_test test_add_force_delete_commit
>  run_test test_add_directory
>  run_test test_add_clashes_with_submodule
>  run_test test_add_symlink