commit 321e4e2c02d53683e001c9033974450a9b3882c5
from: Stefan Sperling <stsp@stsp.name>
date: Tue Sep 17 11:07:58 2024 UTC

safely install configs by using rename(2)

Instead of opening config files in append mode, writing a single line and closing
them again, over and over, use a temporary file which collects file contents and
then rename this file on top of the actual config file stored in /var/run.

This prevents a bug where config content was repeatedly appended to files
in /var/run. Renaming files on top is also safer and more transactional because
other programs reading these files won't see half-way written content.

commit - 1d023088c28e4ddb3fbbb2cba531dcd929a5d276
commit + 321e4e2c02d53683e001c9033974450a9b3882c5
blob - 3be6c3c95114e1039c37328276c082a28857f1e5
blob + 9a3984ccada111db5231435f335be6e00fcedd2b
--- commands.c
+++ commands.c
@@ -3049,8 +3049,19 @@ cmdrc(char rcname[FILENAME_MAX])
 			break;
 		if (line[0] == '#')
 			continue;
-		if (line[0] == '!')
+		if (line[0] == '!') {
+			if (savec) {
+				if (savec->handler == ctlhandler && savec->modh) {
+					saveline[0] = '\0';
+					if (savec->modh == 1)
+						(*savec->handler) (0, NULL, modhvar);
+					else
+						(*savec->handler) (0, NULL, 0);
+				}
+				savec = NULL;
+			}
 			continue;
+		}
 		/*
 		 * Don't ignore indented comments with pound sign, otherwise
 		 * comments won't be saved into daemon/ctl config files.
blob - f5b393c9d71f9dbe31718db7682a8101876ed4cf
blob + 775f6565df69ee34062c4c316b093cd43a0a5af7
--- ctl.c
+++ ctl.c
@@ -92,7 +92,7 @@ void start_dhcpd(char *, char *, char *, char *, char 
 void restart_dhcpd(char *, char *, char *, char *, char *);
 int edit_file(char *, mode_t, char *, char **);
 void ctl_symlink(char *, char *, char *);
-int rule_writeline(char *, mode_t, char *);
+void rule_writeline(int, char *);
 int fill_tmpfile(char **, char *, char **);
 int acq_lock(char *);
 void rls_lock(int);
@@ -669,6 +669,9 @@ ctlhandler(int argc, char **argv, char *modhvar)
 	char **xtest_args = NULL;
 	int rv = 0;
 	int nargs;
+	/* Reused across invocations of this function to accumulate config contents. */
+	static char contentpath[PATH_MAX];
+	static int contentfd = -1;
 
 	memset(&daemons1, 0, sizeof(daemons1));
 
@@ -680,14 +683,14 @@ ctlhandler(int argc, char **argv, char *modhvar)
 		daemons2 = (struct daemons2 *) genget(hname,
 		    (char **)ctl_daemons2, sizeof(struct daemons2));
 		if (daemons2 == NULL || Ambiguous(daemons2)) {
-			printf("%% Internal error - Invalid argument %s\n", argv[1]);
+			printf("%% Internal error - Invalid argument %s\n", hname);
 			return 0;
 		}
 		if (daemons2todaemons(&daemons1, daemons2))
 			goto done;
 		daemons = &daemons1;
 	} else if (Ambiguous(daemons)) {
-		printf("%% Internal error - Ambiguous argument %s\n", argv[1]);
+		printf("%% Internal error - Ambiguous argument %s\n", hname);
 		return 0;
 	}
 
@@ -703,18 +706,47 @@ ctlhandler(int argc, char **argv, char *modhvar)
 		    cli_rtable);
 
 	if (modhvar) {
+		if (argc == 0 && saveline[0] == '\0') {
+			/* install configuration file rules by renaming on top */
+			if (contentfd == -1)
+				goto done;
+			if (chmod(contentpath, daemons->mode) == 1) {
+				printf("%% chmod %o %s: %s\n", daemons->mode, tmpfile,
+				    strerror(errno));
+				goto done;
+			}
+			if (rename(contentpath, tmpfile) == -1) {
+				printf("%% rename %s %s: %s\n", contentpath,
+				    tmpfile, strerror(errno));
+				goto done;
+			}
+			close(contentfd);
+			contentfd = -1;
+			contentpath[0] = '\0';
+			goto done;
+		}
 		/* action specified or indented command specified */
 		if (argc == 2 && isprefix(argv[1], "rules")) {
+			if (contentfd == -1) {
+				snprintf(contentpath, sizeof(contentpath),
+				    "%s.XXXXXXXXXXXXX", tmpfile);
+				contentfd = mkstemp(contentpath);
+				if (contentfd == -1) {
+					printf("%% mkstemp: %s\n", strerror(errno));
+					goto done;
+				}
+			}
 			/* skip 'X rules' line */
+			rv = 1;
 			goto done;
-		}
-		if (isprefix(modhvar, "rules")) {
-			if (!daemons->tmpfile) {
+		} if (isprefix(modhvar, "rules")) {
+			if (contentfd == -1) {
 				printf("%% writeline without tmpfile\n");
 				goto done;
 			}
-			/* write indented line to tmp config file */
-			rule_writeline(tmpfile, daemons->mode, saveline);
+			/* write indented line to tmp content file */
+			rule_writeline(contentfd, saveline);
+			rv = 1;
 			goto done;
 		}
 	}
@@ -819,6 +851,17 @@ ctlhandler(int argc, char **argv, char *modhvar)
 	rv = 1;
 done:
 	free(daemons1.table);
+	if (rv == 0) {
+		/* discard partially written configs */
+		if (contentpath[0]) {
+			unlink(contentpath);
+			contentpath[0] = '\0';
+		}
+		if (contentfd != -1) {
+			close(contentfd);
+			contentfd = -1;
+		}
+	}
 	return rv;
 }
 
@@ -1149,22 +1192,12 @@ edit_file(char *tmpfile, mode_t mode, char *propername
 	return ret;
 }
 
-int
-rule_writeline(char *fname, mode_t mode, char *writeline)
+void
+rule_writeline(int contentfd, char *writeline)
 {
-	FILE *rulefile;
-
-	rulefile = fopen(fname, "a");
-	if (rulefile == NULL) {
-		printf("%% Rule write failed: %s\n", strerror(errno));
-		return(1);
-	}
 	if (writeline[0] == ' ')
 		writeline++;
-	fprintf(rulefile, "%s", writeline);
-	fclose(rulefile);
-	chmod(fname, mode);
-	return(0);
+	dprintf(contentfd, "%s", writeline);
 }
 
 int