From: Omar Polo Subject: Re: fix double-free and removal of missing locally-added files To: Stefan Sperling Cc: gameoftrees@openbsd.org Date: Fri, 25 Aug 2023 13:03:21 +0200 On 2023/08/23 22:22:49 +0200, Stefan Sperling 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