From a3b636e21574e6cff1494e0a84644e06201ddb8d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 22 Aug 2018 20:45:44 -0400 Subject: trailer: use size_t for iterating trailer list We store the length of the trailers list in a size_t. So on a 64-bit system with a 32-bit int, in the unlikely case that we manage to actually allocate a list with 2^31 entries, we'd loop forever trying to iterate over it (our "int" would wrap to negative before exceeding info->trailer_nr). This probably doesn't matter in practice. Each entry is at least a pointer plus a non-empty string, so even without malloc overhead or the memory to hold the original string we're parsing from, you'd need to allocate tens of gigabytes. But it's easy enough to do it right. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'sequencer.c') diff --git a/sequencer.c b/sequencer.c index 4034c0461b..b0613c06bb 100644 --- a/sequencer.c +++ b/sequencer.c @@ -225,7 +225,7 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob, int ignore_footer) { struct trailer_info info; - int i; + size_t i; int found_sob = 0, found_sob_last = 0; trailer_info_get(&info, sb->buf); -- cgit 1.2.3-korg From 00a21f5cbda35eb5d355b453849b625eeca7eac4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 22 Aug 2018 20:46:23 -0400 Subject: trailer: pass process_trailer_opts to trailer_info_get() Most of the trailer code has an "opts" struct which is filled in by the caller. We don't pass it down to trailer_info_get(), which does the initial parsing, because there hasn't yet been a need to do so. Let's start passing it down in preparation for adding new options. Note that there's a single caller which doesn't otherwise have such an options struct. Since it's just one caller (that we'd have to modify anyway), let's not bother with any special treatment like accepting a NULL options struct, and just have it allocate one with the defaults. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- sequencer.c | 3 ++- trailer.c | 7 ++++--- trailer.h | 3 ++- 3 files changed, 8 insertions(+), 5 deletions(-) (limited to 'sequencer.c') diff --git a/sequencer.c b/sequencer.c index b0613c06bb..849208eb40 100644 --- a/sequencer.c +++ b/sequencer.c @@ -224,11 +224,12 @@ static const char *get_todo_path(const struct replay_opts *opts) static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob, int ignore_footer) { + struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; struct trailer_info info; size_t i; int found_sob = 0, found_sob_last = 0; - trailer_info_get(&info, sb->buf); + trailer_info_get(&info, sb->buf, &opts); if (info.trailer_start == info.trailer_end) return 0; diff --git a/trailer.c b/trailer.c index 40eef8880e..e769c5b72c 100644 --- a/trailer.c +++ b/trailer.c @@ -950,7 +950,7 @@ static size_t process_input_file(FILE *outfile, struct strbuf val = STRBUF_INIT; size_t i; - trailer_info_get(&info, str); + trailer_info_get(&info, str, opts); /* Print lines before the trailers as is */ if (!opts->only_trailers) @@ -1067,7 +1067,8 @@ void process_trailers(const char *file, strbuf_release(&sb); } -void trailer_info_get(struct trailer_info *info, const char *str) +void trailer_info_get(struct trailer_info *info, const char *str, + const struct process_trailer_options *opts) { int patch_start, trailer_end, trailer_start; struct strbuf **trailer_lines, **ptr; @@ -1159,7 +1160,7 @@ void format_trailers_from_commit(struct strbuf *out, const char *msg, { struct trailer_info info; - trailer_info_get(&info, msg); + trailer_info_get(&info, msg, opts); format_trailer_info(out, &info, opts); trailer_info_release(&info); } diff --git a/trailer.h b/trailer.h index 6d7f8c2a52..82a62b33bb 100644 --- a/trailer.h +++ b/trailer.h @@ -77,7 +77,8 @@ void process_trailers(const char *file, const struct process_trailer_options *opts, struct list_head *new_trailer_head); -void trailer_info_get(struct trailer_info *info, const char *str); +void trailer_info_get(struct trailer_info *info, const char *str, + const struct process_trailer_options *opts); void trailer_info_release(struct trailer_info *info); -- cgit 1.2.3-korg From ffce7f590fabee6f2314ffd891f1fd3629222839 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 22 Aug 2018 20:50:37 -0400 Subject: sequencer: ignore "---" divider when parsing trailers When the sequencer code appends a signoff or cherry-pick origin, it uses the default trailer-parsing options, which treat "---" as the end of the commit message. As a result, it may be fooled by a commit message that contains that string and fail to find the existing trailer block. Even more confusing, the actual append code does not know about "---", and always appends to the end of the string. This can lead to bizarre results. E.g., appending a signoff to a commit message like this: subject body --- these dashes confuse the parser! Signed-off-by: A results in output with a final block like: Signed-off-by: A Signed-off-by: A The parser thinks the final line of the message is "body", and ignores everything else, claiming there are no trailers. So we output an extra newline separator (wrong) and add a duplicate signoff (also wrong). Since we know we are feeding a pure commit message, we can simply tell the parser to ignore the "---" divider. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- sequencer.c | 2 ++ t/t7501-commit.sh | 16 ++++++++++++++++ 2 files changed, 18 insertions(+) (limited to 'sequencer.c') diff --git a/sequencer.c b/sequencer.c index 849208eb40..51ef7245b1 100644 --- a/sequencer.c +++ b/sequencer.c @@ -229,6 +229,8 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob, size_t i; int found_sob = 0, found_sob_last = 0; + opts.no_divider = 1; + trailer_info_get(&info, sb->buf, &opts); if (info.trailer_start == info.trailer_end) diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh index 9dbbd01fc0..025d65b517 100755 --- a/t/t7501-commit.sh +++ b/t/t7501-commit.sh @@ -517,6 +517,22 @@ Myfooter: x" && test_cmp expected actual ' +test_expect_success 'signoff not confused by ---' ' + cat >expected <<-EOF && + subject + + body + --- + these dashes confuse the parser! + + Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> + EOF + # should be a noop, since we already signed + git commit --allow-empty --signoff -F expected && + git log -1 --pretty=format:%B >actual && + test_cmp expected actual +' + test_expect_success 'multiple -m' ' >negative && -- cgit 1.2.3-korg From 66e83d9b41f7438cb167b9bb54093ebbf0532437 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 22 Aug 2018 20:50:51 -0400 Subject: append_signoff: use size_t for string offsets The append_signoff() function takes an "int" to specify the number of bytes to ignore. Most callers just pass 0, and the remainder use ignore_non_trailer() to skip over cruft. That function also returns an int, and uses them internally. On systems where size_t is larger than an int (i.e., most 64-bit systems), dealing with a ridiculously large commit message could end up overflowing an int, producing surprising results (e.g., returning a negative offset, which would cause us to look outside the original string). Let's consistently use size_t for these offsets through this whole stack. As a bonus, this makes the meaning of "ignore_footer" as an offset (and not a boolean) more clear. But while we're here, let's also document the interface. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- commit.c | 6 +++--- commit.h | 2 +- sequencer.c | 4 ++-- sequencer.h | 9 ++++++++- 4 files changed, 14 insertions(+), 7 deletions(-) (limited to 'sequencer.c') diff --git a/commit.c b/commit.c index 0030e79940..873ef0d5b1 100644 --- a/commit.c +++ b/commit.c @@ -1687,10 +1687,10 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len * Returns the number of bytes from the tail to ignore, to be fed as * the second parameter to append_signoff(). */ -int ignore_non_trailer(const char *buf, size_t len) +size_t ignore_non_trailer(const char *buf, size_t len) { - int boc = 0; - int bol = 0; + size_t boc = 0; + size_t bol = 0; int in_old_conflicts_block = 0; size_t cutoff = wt_status_locate_end(buf, len); diff --git a/commit.h b/commit.h index c3af512f8b..1f7b97cebd 100644 --- a/commit.h +++ b/commit.h @@ -301,7 +301,7 @@ extern const char *find_commit_header(const char *msg, const char *key, size_t *out_len); /* Find the end of the log message, the right place for a new trailer. */ -extern int ignore_non_trailer(const char *buf, size_t len); +extern size_t ignore_non_trailer(const char *buf, size_t len); typedef int (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra, void *cb_data); diff --git a/sequencer.c b/sequencer.c index 51ef7245b1..f24d6c3597 100644 --- a/sequencer.c +++ b/sequencer.c @@ -222,7 +222,7 @@ static const char *get_todo_path(const struct replay_opts *opts) * Returns 3 when sob exists within conforming footer as last entry */ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob, - int ignore_footer) + size_t ignore_footer) { struct process_trailer_options opts = PROCESS_TRAILER_OPTIONS_INIT; struct trailer_info info; @@ -3660,7 +3660,7 @@ int sequencer_pick_revisions(struct replay_opts *opts) return res; } -void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag) +void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag) { unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP; struct strbuf sob = STRBUF_INIT; diff --git a/sequencer.h b/sequencer.h index c5787c6b56..06ff5ff065 100644 --- a/sequencer.h +++ b/sequencer.h @@ -85,7 +85,14 @@ int rearrange_squash(void); extern const char sign_off_header[]; -void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag); +/* + * Append a signoff to the commit message in "msgbuf". The ignore_footer + * parameter specifies the number of bytes at the end of msgbuf that should + * not be considered at all. I.e., they are not checked for existing trailers, + * and the new signoff will be spliced into the buffer before those bytes. + */ +void append_signoff(struct strbuf *msgbuf, size_t ignore_footer, unsigned flag); + void append_conflicts_hint(struct strbuf *msgbuf); int message_is_empty(const struct strbuf *sb, enum commit_msg_cleanup_mode cleanup_mode); -- cgit 1.2.3-korg