From: Stefan Sperling <stsp@stsp.name>
Subject: log message modification check
To: gameoftrees@openbsd.org
Date: Fri, 25 Sep 2020 22:34:05 +0200

On IRC (#gameoftrees on freenode) jrick noted today that our check
for non-modified log messages in edit_logmsg() is slightly wrong.

<M-jrick> this check of initial_content looks odd to me
<M-jrick> it's the same data every iteration
<M-jrick> and it won't handle lines over 1024 bytes long (probably not too likely, but still)

This is a rewritten version which uses getline(3) and compares the entire
log message file without using fixed-sized buffers. Stripping of comments
and leading empty lines is now done separately from this check.

ok?

diff cad0b9e88686cab44e7532dfaaa0b5cdd47beb10 /home/stsp/src/got
blob - fd46852d20b5897c487045e6e0c4d99e0bda0508
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -422,14 +422,16 @@ doneediting:
 
 static const struct got_error *
 edit_logmsg(char **logmsg, const char *editor, const char *logmsg_path,
-    const char *initial_content)
+    const char *initial_content, size_t initial_content_len)
 {
 	const struct got_error *err = NULL;
-	char buf[1024];
+	char *line = NULL;
+	size_t linesize = 0;
+	ssize_t linelen;
 	struct stat st, st2;
-	FILE *fp;
+	FILE *fp = NULL;
 	int content_changed = 0;
-	size_t len;
+	size_t len, logmsg_len;
 
 	*logmsg = NULL;
 
@@ -446,25 +448,62 @@ edit_logmsg(char **logmsg, const char *editor, const c
 		return got_error_msg(GOT_ERR_COMMIT_MSG_EMPTY,
 		    "no changes made to commit message, aborting");
 
-	*logmsg = malloc(st2.st_size + 1);
+	logmsg_len = st2.st_size;
+	*logmsg = malloc(logmsg_len + 1);
 	if (*logmsg == NULL)
 		return got_error_from_errno("malloc");
 	(*logmsg)[0] = '\0';
-	len = 0;
 
 	fp = fopen(logmsg_path, "r");
 	if (fp == NULL) {
 		err = got_error_from_errno("fopen");
 		goto done;
 	}
-	while (fgets(buf, sizeof(buf), fp) != NULL) {
-		if (!content_changed && strcmp(buf, initial_content) != 0)
+
+	if (logmsg_len == initial_content_len) {
+		char *buf;
+		size_t n;
+		buf = malloc(initial_content_len);
+		if (buf == NULL) {
+			err = got_error_from_errno("malloc");
+			goto done;
+		}
+		n = fread(buf, 1, initial_content_len, fp);
+		if (n == 0 && ferror(fp)) {
+			err = got_error_from_errno("fread");
+			free(buf);
+			goto done;
+		}
+		if (n != initial_content_len) {
+			err = got_ferror(fp, GOT_ERR_IO);
+			free(buf);
+			goto done;
+		}
+		if (memcmp(buf, initial_content, initial_content_len) != 0)
 			content_changed = 1;
-		if (buf[0] == '#' || (len == 0 && buf[0] == '\n'))
+		free(buf);
+		if (fseeko(fp, 0L, SEEK_SET) == -1) {
+			err = got_error_from_errno("fseeko");
+			goto done;
+		}
+	} else
+		content_changed = 1;
+
+	len = 0;
+	while ((linelen = getline(&line, &linesize, fp)) != -1) {
+		if ((line[0] == '#' || (len == 0 && line[0] == '\n')))
 			continue; /* remove comments and leading empty lines */
-		len = strlcat(*logmsg, buf, st2.st_size);
+		len = strlcat(*logmsg, line, logmsg_len + 1);
+		if (len >= logmsg_len + 1) {
+			err = got_error(GOT_ERR_NO_SPACE);
+			goto done;
+		}
 	}
-	fclose(fp);
+	free(line);
+	if (ferror(fp)) {
+		err = got_ferror(fp, GOT_ERR_IO);
+		goto done;
+	}
 
 	while (len > 0 && (*logmsg)[len - 1] == '\n') {
 		(*logmsg)[len - 1] = '\0';
@@ -475,6 +514,8 @@ edit_logmsg(char **logmsg, const char *editor, const c
 		err = got_error_msg(GOT_ERR_COMMIT_MSG_EMPTY,
 		    "commit message cannot be empty, aborting");
 done:
+	if (fp && fclose(fp) == EOF && err == NULL)
+		err = got_error_from_errno("fclose");
 	if (err) {
 		free(*logmsg);
 		*logmsg = NULL;
@@ -507,7 +548,8 @@ collect_import_msg(char **logmsg, char **logmsg_path, 
 		goto done;
 	}
 
-	err = edit_logmsg(logmsg, editor, *logmsg_path, initial_content);
+	err = edit_logmsg(logmsg, editor, *logmsg_path, initial_content,
+	     initial_content_len);
 done:
 	if (fd != -1 && close(fd) == -1 && err == NULL)
 		err = got_error_from_errno2("close", *logmsg_path);
@@ -5857,7 +5899,8 @@ get_tag_message(char **tagmsg, char **tagmsg_path, con
 	err = get_editor(&editor);
 	if (err)
 		goto done;
-	err = edit_logmsg(tagmsg, editor, *tagmsg_path, initial_content);
+	err = edit_logmsg(tagmsg, editor, *tagmsg_path, initial_content,
+	    initial_content_len);
 done:
 	free(initial_content);
 	free(template);
@@ -6737,7 +6780,8 @@ collect_commit_logmsg(struct got_pathlist_head *commit
 		    got_commitable_get_path(ct));
 	}
 
-	err = edit_logmsg(logmsg, a->editor, a->logmsg_path, initial_content);
+	err = edit_logmsg(logmsg, a->editor, a->logmsg_path, initial_content,
+	    initial_content_len);
 done:
 	free(initial_content);
 	free(template);
@@ -7971,7 +8015,8 @@ histedit_edit_logmsg(struct got_histedit_list_entry *h
 	if (err)
 		goto done;
 
-	err = edit_logmsg(&hle->logmsg, editor, logmsg_path, logmsg);
+	err = edit_logmsg(&hle->logmsg, editor, logmsg_path, logmsg,
+	    logmsg_len);
 	if (err) {
 		if (err->code != GOT_ERR_COMMIT_MSG_EMPTY)
 			goto done;