Stefan Sperling:

> Sure. OK by me. Using fchmod is clearly less racy. There might be similar
> code elsewhere in the code base that could be fixed like this?

This converts all remaining instances of chmod() into fchmod().

Where the program logic allows, fchmod() moves into place next after
file creation in case of fixed permissions, and next after a stat()
if that's the source of the permissions.


diff 0843a4ce462d9a691a3b5f38a465b129b69fdd5f /home/naddy/got
blob - a1f3f64950abf6a28988d7499f93bc440c52d438
file + lib/object_create.c
--- lib/object_create.c
+++ lib/object_create.c
@@ -78,6 +78,11 @@ create_object_file(struct got_object_id *id, FILE *con
 			goto done;
 	}
 
+	if (fchmod(fileno(tmpfile), GOT_DEFAULT_FILE_MODE) != 0) {
+		err = got_error_from_errno2("fchmod", tmppath);
+		goto done;
+	}
+
 	err = got_deflate_to_file(&tmplen, content, tmpfile);
 	if (err)
 		goto done;
@@ -92,11 +97,6 @@ create_object_file(struct got_object_id *id, FILE *con
 	}
 	free(tmppath);
 	tmppath = NULL;
-
-	if (chmod(objpath, GOT_DEFAULT_FILE_MODE) != 0) {
-		err = got_error_from_errno2("chmod", objpath);
-		goto done;
-	}
 done:
 	free(objpath);
 	if (tmppath) {
blob - 2b33b7d2fe30a50299fc2d669978a76c66ce98c0
file + lib/reference.c
--- lib/reference.c
+++ lib/reference.c
@@ -1140,17 +1140,17 @@ got_ref_write(struct got_reference *ref, struct got_re
 		sb.st_mode = GOT_DEFAULT_FILE_MODE;
 	}
 
+	if (fchmod(fileno(f), sb.st_mode) != 0) {
+		err = got_error_from_errno2("fchmod", tmppath);
+		goto done;
+	}
+
 	if (rename(tmppath, path) != 0) {
 		err = got_error_from_errno3("rename", tmppath, path);
 		goto done;
 	}
 	free(tmppath);
 	tmppath = NULL;
-
-	if (chmod(path, sb.st_mode) != 0) {
-		err = got_error_from_errno2("chmod", path);
-		goto done;
-	}
 done:
 	if (ref->lf == NULL && lf)
 		unlock_err = got_lockfile_unlock(lf);
@@ -1276,23 +1276,22 @@ delete_packed_ref(struct got_reference *delref, struct
 			goto done;
 		}
 
-		if (stat(packed_refs_path, &sb) != 0) {
+		if (fstat(fileno(f), &sb) != 0) {
 			if (errno != ENOENT) {
-				err = got_error_from_errno2("stat",
+				err = got_error_from_errno2("fstat",
 				    packed_refs_path);
 				goto done;
 			}
 			sb.st_mode = GOT_DEFAULT_FILE_MODE;
 		}
 
-		if (rename(tmppath, packed_refs_path) != 0) {
-			err = got_error_from_errno3("rename", tmppath,
-			    packed_refs_path);
+		if (fchmod(fileno(tmpf), sb.st_mode) != 0) {
+			err = got_error_from_errno2("fchmod", tmppath);
 			goto done;
 		}
 
-		if (chmod(packed_refs_path, sb.st_mode) != 0) {
-			err = got_error_from_errno2("chmod",
+		if (rename(tmppath, packed_refs_path) != 0) {
+			err = got_error_from_errno3("rename", tmppath,
 			    packed_refs_path);
 			goto done;
 		}
blob - 54a1c38fbbcfb2ede54f20d464e12db1a15f0447
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -968,6 +968,11 @@ install_symlink_conflict(const char *deriv_target,
 	if (err)
 		goto done;
 
+	if (fchmod(fileno(f), GOT_DEFAULT_FILE_MODE) == -1) {
+		err = got_error_from_errno2("fchmod", path);
+		goto done;
+	}
+
 	if (fprintf(f, "%s %s\n%s\n%s%s%s%s%s\n%s\n%s\n",
 	    GOT_DIFF_CONFLICT_MARKER_BEGIN, label_deriv,
 	    deriv_target ? deriv_target : "(symlink was deleted)",
@@ -989,10 +994,6 @@ install_symlink_conflict(const char *deriv_target,
 		err = got_error_from_errno3("rename", path, ondisk_path);
 		goto done;
 	}
-	if (chmod(ondisk_path, GOT_DEFAULT_FILE_MODE) == -1) {
-		err = got_error_from_errno2("chmod", ondisk_path);
-		goto done;
-	}
 done:
 	if (f != NULL && fclose(f) == EOF && err == NULL)
 		err = got_error_from_errno2("fclose", path);
@@ -1511,6 +1512,12 @@ install_blob(struct got_worktree *worktree, const char
 			return got_error_from_errno2("open", ondisk_path);
 	}
 
+	if (fchmod(fd, get_ondisk_perms(te_mode & S_IXUSR, st_mode)) == -1) {
+		err = got_error_from_errno2("fchmod",
+		    update ? tmppath : ondisk_path);
+		goto done;
+	}
+
 	if (progress_cb) {
 		if (restoring_missing_file)
 			err = (*progress_cb)(progress_arg, GOT_STATUS_MISSING,
@@ -1559,12 +1566,6 @@ install_blob(struct got_worktree *worktree, const char
 		}
 	}
 
-	if (chmod(ondisk_path,
-	    get_ondisk_perms(te_mode & S_IXUSR, st_mode)) == -1) {
-		err = got_error_from_errno2("chmod", ondisk_path);
-		goto done;
-	}
-
 done:
 	if (fd != -1 && close(fd) != 0 && err == NULL)
 		err = got_error_from_errno("close");
@@ -4332,8 +4333,8 @@ create_patched_content(char **path_outfile, int revers
 			goto done;
 
 		if (!S_ISLNK(sb2.st_mode)) {
-			if (chmod(*path_outfile, sb2.st_mode) == -1) {
-				err = got_error_from_errno2("chmod", path2);
+			if (fchmod(fileno(outfile), sb2.st_mode) == -1) {
+				err = got_error_from_errno2("fchmod", path2);
 				goto done;
 			}
 		}
-- 
Christian "naddy" Weisgerber                          naddy@mips.inka.de