"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Stefan Sperling <stsp@stsp.name>
Subject:
fix double-free and removal of missing locally-added files
To:
gameoftrees@openbsd.org
Date:
Wed, 23 Aug 2023 22:22:49 +0200

Download raw body.

Thread
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?

-----------------------------------------------
 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