From: Stefan Sperling Subject: Re: log message modification check To: gameoftrees@openbsd.org Date: Fri, 25 Sep 2020 23:27:54 +0200 On Fri, Sep 25, 2020 at 10:34:05PM +0200, Stefan Sperling wrote: > On IRC (#gameoftrees on freenode) jrick noted today that our check > for non-modified log messages in edit_logmsg() is slightly wrong. > > this check of initial_content looks odd to me > it's the same data every iteration > 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? This is an improved patch after more feedback on IRC: i would think that passing both the prev, and re-read contents through the comment filter, and then comparing that would be better 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; - int content_changed = 0; - size_t len; + FILE *fp = NULL; + size_t len, logmsg_len; + char *initial_content_stripped = NULL, *buf = NULL, *s; *logmsg = NULL; @@ -446,35 +448,83 @@ 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); + /* + * Set up a stripped version of the initial content without comments + * and blank lines. We need this in order to check if the message + * has in fact been edited. + */ + initial_content_stripped = malloc(initial_content_len + 1); + if (initial_content_stripped == NULL) + return got_error_from_errno("calloc"); + initial_content_stripped[0] = '\0'; + + buf = strdup(initial_content); + if (buf == NULL) { + err = got_error_from_errno("strdup"); + goto done; + } + s = buf; + len = 0; + while ((line = strsep(&s, "\n")) != NULL) { + if ((line[0] == '#' || (len == 0 && line[0] == '\n'))) + continue; /* remove comments and leading empty lines */ + len = strlcat(initial_content_stripped, line, + initial_content_len + 1); + if (len >= initial_content_len + 1) { + err = got_error(GOT_ERR_NO_SPACE); + goto done; + } + } + while (len > 0 && initial_content_stripped[len - 1] == '\n') { + initial_content_stripped[len - 1] = '\0'; + len--; + } + + 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) - content_changed = 1; - if (buf[0] == '#' || (len == 0 && buf[0] == '\n')) + + 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'; len--; } - if (len == 0 || !content_changed) + if (len == 0) { err = got_error_msg(GOT_ERR_COMMIT_MSG_EMPTY, "commit message cannot be empty, aborting"); + goto done; + } + if (strcmp(*logmsg, initial_content_stripped) == 0) + err = got_error_msg(GOT_ERR_COMMIT_MSG_EMPTY, + "no changes made to commit message, aborting"); done: + free(initial_content_stripped); + free(buf); + if (fp && fclose(fp) == EOF && err == NULL) + err = got_error_from_errno("fclose"); if (err) { free(*logmsg); *logmsg = NULL; @@ -507,7 +557,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 +5908,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 +6789,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 +8024,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;