git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: git@vger.kernel.org
Subject: [PATCH 2/5] log_write_email_headers: use strbufs
Date: Fri, 18 May 2018 18:57:44 -0700	[thread overview]
Message-ID: <20180519015743.GB32492@sigill.intra.peff.net> (raw)
In-Reply-To: <20180519015444.GA12080@sigill.intra.peff.net>

When we write a MIME attachment, we write the mime headers
into fixed-size buffers. These are likely to be big enough
in practice, but technically the input could be arbitrarily
large (e.g., if the caller provided a lot of content in the
extra_headers string), in which case we'd quietly truncate
it and generate bogus output. Let's convert these buffers to
strbufs.

The memory ownership here is a bit funny. The original fixed
buffers were static, and we merely pass out pointers to them
to be used by the caller (and in one case, we even just
stuff our value into the opt->diffopt.stat_sep value).
Ideally we'd actually pass back heap buffers, and the caller
would be responsible for freeing them.

This patch punts on that cleanup for now, and instead just
marks the strbufs as static. That means we keep ownership in
this function, making it not a complete leak. This also
takes us one step closer to fixing it in the long term
(since we can eventually use strbuf_detach() to hand
ownership to the caller, once it's ready).

Signed-off-by: Jeff King <peff@peff.net>
---
The rest of that cleanup is a possible #leftoverbits.

 log-tree.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index d1c0bedf24..1173fdb057 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -386,11 +386,15 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 		graph_show_oneline(opt->graph);
 	}
 	if (opt->mime_boundary) {
-		static char subject_buffer[1024];
-		static char buffer[1024];
+		static struct strbuf subject_buffer = STRBUF_INIT;
+		static struct strbuf buffer = STRBUF_INIT;
 		struct strbuf filename =  STRBUF_INIT;
 		*need_8bit_cte_p = -1; /* NEVER */
-		snprintf(subject_buffer, sizeof(subject_buffer) - 1,
+
+		strbuf_reset(&subject_buffer);
+		strbuf_reset(&buffer);
+
+		strbuf_addf(&subject_buffer,
 			 "%s"
 			 "MIME-Version: 1.0\n"
 			 "Content-Type: multipart/mixed;"
@@ -405,13 +409,13 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			 extra_headers ? extra_headers : "",
 			 mime_boundary_leader, opt->mime_boundary,
 			 mime_boundary_leader, opt->mime_boundary);
-		extra_headers = subject_buffer;
+		extra_headers = subject_buffer.buf;
 
 		if (opt->numbered_files)
 			strbuf_addf(&filename, "%d", opt->nr);
 		else
 			fmt_output_commit(&filename, commit, opt);
-		snprintf(buffer, sizeof(buffer) - 1,
+		strbuf_addf(&buffer,
 			 "\n--%s%s\n"
 			 "Content-Type: text/x-patch;"
 			 " name=\"%s\"\n"
@@ -422,7 +426,7 @@ void log_write_email_headers(struct rev_info *opt, struct commit *commit,
 			 filename.buf,
 			 opt->no_inline ? "attachment" : "inline",
 			 filename.buf);
-		opt->diffopt.stat_sep = buffer;
+		opt->diffopt.stat_sep = buffer.buf;
 		strbuf_release(&filename);
 	}
 	*extra_headers_p = extra_headers;
-- 
2.17.0.1052.g7d69f75dbf


  parent reply	other threads:[~2018-05-19  1:57 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-19  1:54 [PATCH 0/5] snprintf truncation fixes Jeff King
2018-05-19  1:56 ` [PATCH 1/5] http: use strbufs instead of fixed buffers Jeff King
2018-05-21 18:11   ` Stefan Beller
2018-05-21 19:41     ` Jeff King
2018-05-21 20:57       ` Stefan Beller
2018-05-19  1:57 ` Jeff King [this message]
2018-05-19  1:57 ` [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers Jeff King
2018-05-19  8:27   ` René Scharfe
2018-05-20 17:08     ` Jeff King
2018-05-21  0:58     ` Junio C Hamano
2018-05-21 12:36     ` Ben Peart
2018-05-19  1:58 ` [PATCH 4/5] shorten_unambiguous_ref: use xsnprintf Jeff King
2018-05-19  1:58 ` [PATCH 5/5] fmt_with_err: add a comment that truncation is OK Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180519015743.GB32492@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).