From: Stefan Sperling Subject: catch errors from unlink(2) To: gameoftrees@openbsd.org Date: Sat, 15 Oct 2022 19:18:29 +0200 This patch adds some missing unlink(2) error checks. In cases where we are unlinking a file while already unwinding from an error condition, we cannot return unlink errors, so those unlink calls are left as is. There is also code in buf.c that ignores errors, but this file was inherited from OpenCVS and should probably be fixed there first, if needed. An important fix in here is where we open a new temp file with mkstemp() and then fail to unlink it. This error is not reported up, which is bad. No caller is deleting these files manually, and if there is e.g. an I/O failure which prevents deletion of the file and we still manage to write data to that file, the file might be left behind on disk even though we want it deleted. This is a big diff, my local branch splits it into a couple of commits: check for unlink(2) errors with the == -1 idiom, rather than != 0 check for unlink(2) errors in got_opentempfd() check for unlink(2) errors and don't try to unlink an already renamed file introduce got_path_move_file(); based on code from lib/patch.c handle unlink(2) errors for mergepath, tmppath, and apath in apply_patch() ok? diff refs/heads/main refs/heads/unlink-errors commit - 9cda65e55e3f6d565e83377712e1b81177639cda commit + 9536370f6b2ee32526d8d5c398247f0d1c2210ca blob - a223de14baf9eab1c747904cbc10cbaf30003d8c blob + 7cb114b362ef4f4a6d3f385fb37066d5a97691bd --- include/got_path.h +++ include/got_path.h @@ -132,3 +132,10 @@ const struct got_error *got_path_create_file(const cha /* Create a new file at a specified path, with optional content. */ const struct got_error *got_path_create_file(const char *, const char *); + +/* + * Attempt to move an existing file to a new path, creating missing parent + * directories at the destination path if necessary. + * (Cross-mount-point moves are not yet implemented.) + */ +const struct got_error *got_path_move_file(const char *, const char *); blob - 4c4e3bbe901a58b588c927fbd65dd7620b43e919 blob + 3c1f5eebe793a2f895a8529b13cdad48e3f9ccb0 --- lib/opentemp.c +++ lib/opentemp.c @@ -34,8 +34,12 @@ got_opentempfd(void) return -1; fd = mkstemp(name); - if (fd != -1) - unlink(name); + if (fd != -1) { + if (unlink(name) == -1) { + close(fd); + return -1; + } + } return fd; } blob - f8e3aaf634fc474c5546bddf69cefd58eb36f969 blob + 14024d2a98a433e4feb490708b08e72e42f96f75 --- lib/patch.c +++ lib/patch.c @@ -789,11 +789,10 @@ apply_patch(int *overlapcnt, struct got_worktree *work int do_merge = 0, file_renamed = 0; char *oldlabel = NULL, *newlabel = NULL, *anclabel = NULL; char *oldpath = NULL, *newpath = NULL; - char *tmppath = NULL, *template = NULL, *parent = NULL; + char *tmppath = NULL, *template = NULL; char *apath = NULL, *mergepath = NULL; FILE *oldfile = NULL, *tmpfile = NULL, *afile = NULL, *mergefile = NULL; int outfd; - const char *outpath; mode_t mode = GOT_DEFAULT_FILE_MODE; *overlapcnt = 0; @@ -842,7 +841,6 @@ apply_patch(int *overlapcnt, struct got_worktree *work err = got_opentemp_named(&tmppath, &tmpfile, template); if (err) goto done; - outpath = tmppath; outfd = fileno(tmpfile); err = patch_file(p, afile != NULL ? afile : oldfile, tmpfile); if (err) @@ -900,7 +898,6 @@ apply_patch(int *overlapcnt, struct got_worktree *work err = got_opentemp_named(&mergepath, &mergefile, template); if (err) goto done; - outpath = mergepath; outfd = fileno(mergefile); err = got_merge_diff3(overlapcnt, outfd, tmpfile, afile, @@ -924,24 +921,18 @@ apply_patch(int *overlapcnt, struct got_worktree *work goto done; } - if (rename(outpath, newpath) == -1) { - if (errno != ENOENT) { - err = got_error_from_errno3("rename", outpath, - newpath); + if (mergepath) { + err = got_path_move_file(mergepath, newpath); + if (err) goto done; - } - - err = got_path_dirname(&parent, newpath); - if (err != NULL) + free(mergepath); + mergepath = NULL; + } else { + err = got_path_move_file(tmppath, newpath); + if (err) goto done; - err = got_path_mkdir(parent); - if (err != NULL) - goto done; - if (rename(outpath, newpath) == -1) { - err = got_error_from_errno3("rename", outpath, - newpath); - goto done; - } + free(tmppath); + tmppath = NULL; } if (file_renamed) { @@ -966,11 +957,10 @@ done: err = report_progress(pa, old, new, GOT_STATUS_MODIFY, NULL); done: - free(parent); free(template); - if (tmppath != NULL) - unlink(tmppath); + if (tmppath != NULL && unlink(tmppath) == -1 && err == NULL) + err = got_error_from_errno("unlink"); if (tmpfile != NULL && fclose(tmpfile) == EOF && err == NULL) err = got_error_from_errno("fclose"); free(tmppath); @@ -979,14 +969,14 @@ done: if (oldfile != NULL && fclose(oldfile) == EOF && err == NULL) err = got_error_from_errno("fclose"); - if (apath != NULL) - unlink(apath); + if (apath != NULL && unlink(apath) == -1 && err == NULL) + err = got_error_from_errno("unlink"); if (afile != NULL && fclose(afile) == EOF && err == NULL) err = got_error_from_errno("fclose"); free(apath); - if (mergepath != NULL) - unlink(mergepath); + if (mergepath != NULL && unlink(mergepath) == -1 && err == NULL) + err = got_error_from_errno("unlink"); if (mergefile != NULL && fclose(mergefile) == EOF && err == NULL) err = got_error_from_errno("fclose"); free(mergepath); blob - e3d7799acfdd15ffc55d19126b7c7e27a696bee6 blob + 890ae033826305afdd0fc0401d8ff424ea07eaf6 --- lib/path.c +++ lib/path.c @@ -537,3 +537,24 @@ done: err = got_error_from_errno("close"); return err; } + +const struct got_error * +got_path_move_file(const char *oldpath, const char *newpath) +{ + const struct got_error *err; + + if (rename(oldpath, newpath) != -1) + return NULL; + + if (errno != ENOENT) + return got_error_from_errno3("rename", oldpath, newpath); + + err = make_parent_dirs(newpath); + if (err) + return err; + + if (rename(oldpath, newpath) == -1) + return got_error_from_errno3("rename", oldpath, newpath); + + return NULL; +} blob - ec8a575ccccc0191c9140e4914ca6dd642338b9c blob + d6f94939b844a5e06b4b92355a08680544dba996 --- lib/reference.c +++ lib/reference.c @@ -1320,7 +1320,7 @@ done: free(path_refs); free(path); if (tmppath) { - if (unlink(tmppath) != 0 && err == NULL) + if (unlink(tmppath) == -1 && err == NULL) err = got_error_from_errno2("unlink", tmppath); free(tmppath); } @@ -1454,6 +1454,8 @@ delete_packed_ref(struct got_reference *delref, struct packed_refs_path); goto done; } + free(tmppath); + tmppath = NULL; } done: if (delref->lf == NULL && lf) @@ -1462,11 +1464,10 @@ done: if (fclose(f) == EOF && err == NULL) err = got_error_from_errno("fclose"); } - if (tmpf) { - unlink(tmppath); - if (fclose(tmpf) == EOF && err == NULL) - err = got_error_from_errno("fclose"); - } + if (tmppath && unlink(tmppath) == -1 && err == NULL) + err = got_error_from_errno2("unlink", tmppath); + if (tmpf && fclose(tmpf) == EOF && err == NULL) + err = got_error_from_errno("fclose"); free(tmppath); free(packed_refs_path); free(line); @@ -1501,7 +1502,7 @@ delete_loose_ref(struct got_reference *ref, struct got /* XXX: check if old content matches our expectations? */ - if (unlink(path) != 0) + if (unlink(path) == -1) err = got_error_from_errno2("unlink", path); done: if (ref->lf == NULL && lf) blob - 30e991da06f030404cbce64de63293d385ff1ac8 blob + d0e5af7c33c6ca8a4ba2e9dc621a494ed26dda57 --- lib/worktree.c +++ lib/worktree.c @@ -4135,12 +4135,12 @@ schedule_for_deletion(void *arg, unsigned char status, size_t root_len; if (dirfd != -1) { - if (unlinkat(dirfd, de_name, 0) != 0) { + if (unlinkat(dirfd, de_name, 0) == -1) { err = got_error_from_errno2("unlinkat", ondisk_path); goto done; } - } else if (unlink(ondisk_path) != 0) { + } else if (unlink(ondisk_path) == -1) { err = got_error_from_errno2("unlink", ondisk_path); goto done; }