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