threads / patch / 48519

patch, 5 partssnprintf truncation fixes

Subject: [PATCH 0/5] snprintf truncation fixes

## tl;dr

13 messages between May 19, 2018 and May 21, 2018. Diffs are folded; open one to read it.

replies: 12people: 5as markdown or json

Jeff King· May 19, 2018, 01:54 UTC · lore

I happened today to be looking at a piece of code that used a bare snprintf() without checking for truncation, and I got annoyed enough to root out the last few cases in our codebase. After this series, looking over the results of:

  git grep '[^vxn]snprintf' '*.c' :^compat
is pretty pleasant.

This series also gets rid of some uses of PATH_MAX, which is another pet peeve of mine. :)

  [1/5]: http: use strbufs instead of fixed buffers
  [2/5]: log_write_email_headers: use strbufs
  [3/5]: query_fsmonitor: use xsnprintf for formatting integers
  [4/5]: shorten_unambiguous_ref: use xsnprintf
  [5/5]: fmt_with_err: add a comment that truncation is OK
 fsmonitor.c |  4 ++--
 http.c      | 66 +++++++++++++++++++++++++++++------------------------
 http.h      |  4 ++--
 log-tree.c  | 16 ++++++++-----
 refs.c      |  4 ++--
 usage.c     |  1 +
 6 files changed, 53 insertions(+), 42 deletions(-)
-Peff
Jeff King· May 19, 2018, 01:56 UTC · re: Jeff King · lore

[PATCH 1/5] http: use strbufs instead of fixed buffers

We keep the names of incoming packs and objects in fixed PATH_MAX-size buffers, and snprintf() into them. This is unlikely to end up with truncated filenames, but it is possible (especially on systems where PATH_MAX is shorter than actual paths can be). Let's switch to using strbufs, which makes the question go away entirely.

Signed-off-by: Jeff King <peff@peff.net>
---
The diff is a little noisy due to the s/tmpfile/&.buf/ everywhere. The
interesting lines to look at are initialization and release, which I
think I got right.
 http.c | 66 ++++++++++++++++++++++++++++++++--------------------------
 http.h |  4 ++--
 2 files changed, 38 insertions(+), 32 deletions(-)
Show changes to 2 files +38 −32

http.c, http.h

diff --git a/http.c b/http.c
index fed13b2169..260bd10f95 100644
--- a/http.c
+++ b/http.c
@@ -2082,6 +2082,7 @@ void release_http_pack_request(struct http_pack_request *preq)
 		preq->packfile = NULL;
 	}
 	preq->slot = NULL;
+	strbuf_release(&preq->tmpfile);
 	free(preq->url);
 	free(preq);
 }
@@ -2104,19 +2105,19 @@ int finish_http_pack_request(struct http_pack_request *preq)
 		lst = &((*lst)->next);
 	*lst = (*lst)->next;
 
-	if (!strip_suffix(preq->tmpfile, ".pack.temp", &len))
+	if (!strip_suffix(preq->tmpfile.buf, ".pack.temp", &len))
 		die("BUG: pack tmpfile does not end in .pack.temp?");
-	tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile);
+	tmp_idx = xstrfmt("%.*s.idx.temp", (int)len, preq->tmpfile.buf);
 
 	argv_array_push(&ip.args, "index-pack");
 	argv_array_pushl(&ip.args, "-o", tmp_idx, NULL);
-	argv_array_push(&ip.args, preq->tmpfile);
+	argv_array_push(&ip.args, preq->tmpfile.buf);
 	ip.git_cmd = 1;
 	ip.no_stdin = 1;
 	ip.no_stdout = 1;
 
 	if (run_command(&ip)) {
-		unlink(preq->tmpfile);
+		unlink(preq->tmpfile.buf);
 		unlink(tmp_idx);
 		free(tmp_idx);
 		return -1;
@@ -2124,7 +2125,7 @@ int finish_http_pack_request(struct http_pack_request *preq)
 
 	unlink(sha1_pack_index_name(p->sha1));
 
-	if (finalize_object_file(preq->tmpfile, sha1_pack_name(p->sha1))
+	if (finalize_object_file(preq->tmpfile.buf, sha1_pack_name(p->sha1))
 	 || finalize_object_file(tmp_idx, sha1_pack_index_name(p->sha1))) {
 		free(tmp_idx);
 		return -1;
@@ -2143,6 +2144,7 @@ struct http_pack_request *new_http_pack_request(
 	struct http_pack_request *preq;
 
 	preq = xcalloc(1, sizeof(*preq));
+	strbuf_init(&preq->tmpfile, 0);
 	preq->target = target;
 
 	end_url_with_slash(&buf, base_url);
@@ -2150,12 +2152,11 @@ struct http_pack_request *new_http_pack_request(
 		sha1_to_hex(target->sha1));
 	preq->url = strbuf_detach(&buf, NULL);
 
-	snprintf(preq->tmpfile, sizeof(preq->tmpfile), "%s.temp",
-		sha1_pack_name(target->sha1));
-	preq->packfile = fopen(preq->tmpfile, "a");
+	strbuf_addf(&preq->tmpfile, "%s.temp", sha1_pack_name(target->sha1));
+	preq->packfile = fopen(preq->tmpfile.buf, "a");
 	if (!preq->packfile) {
 		error("Unable to open local file %s for pack",
-		      preq->tmpfile);
+		      preq->tmpfile.buf);
 		goto abort;
 	}
 
@@ -2182,6 +2183,7 @@ struct http_pack_request *new_http_pack_request(
 	return preq;
 
 abort:
+	strbuf_release(&preq->tmpfile);
 	free(preq->url);
 	free(preq);
 	return NULL;
@@ -2232,7 +2234,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 {
 	char *hex = sha1_to_hex(sha1);
 	struct strbuf filename = STRBUF_INIT;
-	char prevfile[PATH_MAX];
+	struct strbuf prevfile = STRBUF_INIT;
 	int prevlocal;
 	char prev_buf[PREV_BUF_SIZE];
 	ssize_t prev_read = 0;
@@ -2240,40 +2242,41 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	struct http_object_request *freq;
 
 	freq = xcalloc(1, sizeof(*freq));
+	strbuf_init(&freq->tmpfile, 0);
 	hashcpy(freq->sha1, sha1);
 	freq->localfile = -1;
 
 	sha1_file_name(the_repository, &filename, sha1);
-	snprintf(freq->tmpfile, sizeof(freq->tmpfile),
-		 "%s.temp", filename.buf);
+	strbuf_addf(&freq->tmpfile, "%s.temp", filename.buf);
 
-	snprintf(prevfile, sizeof(prevfile), "%s.prev", filename.buf);
-	unlink_or_warn(prevfile);
-	rename(freq->tmpfile, prevfile);
-	unlink_or_warn(freq->tmpfile);
+	strbuf_addf(&prevfile, "%s.prev", filename.buf);
+	unlink_or_warn(prevfile.buf);
+	rename(freq->tmpfile.buf, prevfile.buf);
+	unlink_or_warn(freq->tmpfile.buf);
 	strbuf_release(&filename);
 
 	if (freq->localfile != -1)
 		error("fd leakage in start: %d", freq->localfile);
-	freq->localfile = open(freq->tmpfile,
+	freq->localfile = open(freq->tmpfile.buf,
 			       O_WRONLY | O_CREAT | O_EXCL, 0666);
 	/*
 	 * This could have failed due to the "lazy directory creation";
 	 * try to mkdir the last path component.
 	 */
 	if (freq->localfile < 0 && errno == ENOENT) {
-		char *dir = strrchr(freq->tmpfile, '/');
+		char *dir = strrchr(freq->tmpfile.buf, '/');
 		if (dir) {
 			*dir = 0;
-			mkdir(freq->tmpfile, 0777);
+			mkdir(freq->tmpfile.buf, 0777);
 			*dir = '/';
 		}
-		freq->localfile = open(freq->tmpfile,
+		freq->localfile = open(freq->tmpfile.buf,
 				       O_WRONLY | O_CREAT | O_EXCL, 0666);
 	}
 
 	if (freq->localfile < 0) {
-		error_errno("Couldn't create temporary file %s", freq->tmpfile);
+		error_errno("Couldn't create temporary file %s",
+			    freq->tmpfile.buf);
 		goto abort;
 	}
 
@@ -2287,7 +2290,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	 * If a previous temp file is present, process what was already
 	 * fetched.
 	 */
-	prevlocal = open(prevfile, O_RDONLY);
+	prevlocal = open(prevfile.buf, O_RDONLY);
 	if (prevlocal != -1) {
 		do {
 			prev_read = xread(prevlocal, prev_buf, PREV_BUF_SIZE);
@@ -2304,7 +2307,8 @@ struct http_object_request *new_http_object_request(const char *base_url,
 		} while (prev_read > 0);
 		close(prevlocal);
 	}
-	unlink_or_warn(prevfile);
+	unlink_or_warn(prevfile.buf);
+	strbuf_release(&prevfile);
 
 	/*
 	 * Reset inflate/SHA1 if there was an error reading the previous temp
@@ -2319,7 +2323,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 			lseek(freq->localfile, 0, SEEK_SET);
 			if (ftruncate(freq->localfile, 0) < 0) {
 				error_errno("Couldn't truncate temporary file %s",
-					    freq->tmpfile);
+					    freq->tmpfile.buf);
 				goto abort;
 			}
 		}
@@ -2349,6 +2353,7 @@ struct http_object_request *new_http_object_request(const char *base_url,
 	return freq;
 
 abort:
+	strbuf_release(&prevfile);
 	free(freq->url);
 	free(freq);
 	return NULL;
@@ -2376,24 +2381,24 @@ int finish_http_object_request(struct http_object_request *freq)
 	if (freq->http_code == 416) {
 		warning("requested range invalid; we may already have all the data.");
 	} else if (freq->curl_result != CURLE_OK) {
-		if (stat(freq->tmpfile, &st) == 0)
+		if (stat(freq->tmpfile.buf, &st) == 0)
 			if (st.st_size == 0)
-				unlink_or_warn(freq->tmpfile);
+				unlink_or_warn(freq->tmpfile.buf);
 		return -1;
 	}
 
 	git_inflate_end(&freq->stream);
 	git_SHA1_Final(freq->real_sha1, &freq->c);
 	if (freq->zret != Z_STREAM_END) {
-		unlink_or_warn(freq->tmpfile);
+		unlink_or_warn(freq->tmpfile.buf);
 		return -1;
 	}
 	if (hashcmp(freq->sha1, freq->real_sha1)) {
-		unlink_or_warn(freq->tmpfile);
+		unlink_or_warn(freq->tmpfile.buf);
 		return -1;
 	}
 	sha1_file_name(the_repository, &filename, freq->sha1);
-	freq->rename = finalize_object_file(freq->tmpfile, filename.buf);
+	freq->rename = finalize_object_file(freq->tmpfile.buf, filename.buf);
 	strbuf_release(&filename);
 
 	return freq->rename;
@@ -2401,7 +2406,7 @@ int finish_http_object_request(struct http_object_request *freq)
 
 void abort_http_object_request(struct http_object_request *freq)
 {
-	unlink_or_warn(freq->tmpfile);
+	unlink_or_warn(freq->tmpfile.buf);
 
 	release_http_object_request(freq);
 }
@@ -2421,4 +2426,5 @@ void release_http_object_request(struct http_object_request *freq)
 		release_active_slot(freq->slot);
 		freq->slot = NULL;
 	}
+	strbuf_release(&freq->tmpfile);
 }
diff --git a/http.h b/http.h
index 4df4a25e1a..d305ca1dc7 100644
--- a/http.h
+++ b/http.h
@@ -207,7 +207,7 @@ struct http_pack_request {
 	struct packed_git *target;
 	struct packed_git **lst;
 	FILE *packfile;
-	char tmpfile[PATH_MAX];
+	struct strbuf tmpfile;
 	struct active_request_slot *slot;
 };
 
@@ -219,7 +219,7 @@ extern void release_http_pack_request(struct http_pack_request *preq);
 /* Helpers for fetching object */
 struct http_object_request {
 	char *url;
-	char tmpfile[PATH_MAX];
+	struct strbuf tmpfile;
 	int localfile;
 	CURLcode curl_result;
 	char errorstr[CURL_ERROR_SIZE];

-- 
2.17.0.1052.g7d69f75dbf
Stefan Beller· May 21, 2018, 18:11 UTC · re: Jeff King · lore

Re: [PATCH 1/5] http: use strbufs instead of fixed buffers

Hi Jeff,
On Fri, May 18, 2018 at 6:56 PM, Jeff King <peff@peff.net> wrote:
> @@ -2421,4 +2426,5 @@ void release_http_object_request(struct http_object_request *freq)
..
> +       strbuf_release(&freq->tmpfile);
Do we need an equivalent in release_http_pack_request as well?
Jeff King· May 21, 2018, 19:41 UTC · re: Stefan Beller · lore

Re: [PATCH 1/5] http: use strbufs instead of fixed buffers

On Mon, May 21, 2018 at 11:11:51AM -0700, Stefan Beller wrote:
Show 9 quoted lines
> Hi Jeff,
> 
> On Fri, May 18, 2018 at 6:56 PM, Jeff King <peff@peff.net> wrote:
> 
> > @@ -2421,4 +2426,5 @@ void release_http_object_request(struct http_object_request *freq)
> ..
> > +       strbuf_release(&freq->tmpfile);
> 
> Do we need an equivalent in release_http_pack_request as well?
Yes, but isn't there one?
From the original patch:
Show changes to http.c +1 −1
--- a/http.c
+++ b/http.c
@@ -2082,6 +2082,7 @@ void release_http_pack_request(struct http_pack_request *preq)
                preq->packfile = NULL;
        }
        preq->slot = NULL;
+       strbuf_release(&preq->tmpfile);
        free(preq->url);
        free(preq);
 }

Or am I missing something?

-Peff
Stefan Beller· May 21, 2018, 20:57 UTC · re: Jeff King · lore

Re: [PATCH 1/5] http: use strbufs instead of fixed buffers

On Mon, May 21, 2018 at 12:41 PM, Jeff King <peff@peff.net> wrote:
Show 26 quoted lines
> On Mon, May 21, 2018 at 11:11:51AM -0700, Stefan Beller wrote:
>
>> Hi Jeff,
>>
>> On Fri, May 18, 2018 at 6:56 PM, Jeff King <peff@peff.net> wrote:
>>
>> > @@ -2421,4 +2426,5 @@ void release_http_object_request(struct http_object_request *freq)
>> ..
>> > +       strbuf_release(&freq->tmpfile);
>>
>> Do we need an equivalent in release_http_pack_request as well?
>
> Yes, but isn't there one?
>
> From the original patch:
>
> --- a/http.c
> +++ b/http.c
> @@ -2082,6 +2082,7 @@ void release_http_pack_request(struct http_pack_request *preq)
>                 preq->packfile = NULL;
>         }
>         preq->slot = NULL;
> +       strbuf_release(&preq->tmpfile);
>         free(preq->url);
>         free(preq);
>  }

which is at the very top, but I was not looking this far up, but looked for functions order that my editor had.

Clearly I have to practice code reviews, still.
Sorry for the noise.
Stefan
Jeff King· May 19, 2018, 01:57 UTC · re: Jeff King · lore

[PATCH 2/5] log_write_email_headers: use strbufs

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(-)
Show changes to log-tree.c +10 −6
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
Jeff King· May 19, 2018, 01:57 UTC · re: Jeff King · lore

[PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers

These formatted integers should always fit into their 64-byte buffers. Let's use xsnprintf() to add an assertion that this is the case, which makes auditing for other unchecked snprintfs() easier.

Signed-off-by: Jeff King <peff@peff.net>
---
 fsmonitor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to fsmonitor.c +2 −2
diff --git a/fsmonitor.c b/fsmonitor.c
index ed3d1a074d..cc19b27e1d 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -104,8 +104,8 @@ static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *que
 	if (!(argv[0] = core_fsmonitor))
 		return -1;
 
-	snprintf(ver, sizeof(ver), "%d", version);
-	snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update);
+	xsnprintf(ver, sizeof(ver), "%d", version);
+	xsnprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update);
 	argv[1] = ver;
 	argv[2] = date;
 	argv[3] = NULL;

-- 
2.17.0.1052.g7d69f75dbf
René Scharfe· May 19, 2018, 08:27 UTC · re: Jeff King · lore

Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers

Am 19.05.2018 um 03:57 schrieb Jeff King:
> These formatted integers should always fit into their
> 64-byte buffers. Let's use xsnprintf() to add an assertion
> that this is the case, which makes auditing for other
> unchecked snprintfs() easier.
How about this instead?
-- >8 --
Subject: [PATCH] fsmonitor: use internal argv_array of struct child_process

Avoid magic array sizes and indexes by constructing the fsmonitor command line using the embedded argv_array of the child_process. The resulting code is shorter and easier to extend.

Getting rid of the snprintf() calls is a bonus -- even though the buffers were big enough here to avoid truncation -- as it makes auditing the remaining callers easier.

Inspired-by: Jeff King <peff@peff.net>
Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 fsmonitor.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)
Show changes to fsmonitor.c +4 −10
diff --git a/fsmonitor.c b/fsmonitor.c
index ed3d1a074d..665bd2d425 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -97,19 +97,13 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
-	char ver[64];
-	char date[64];
-	const char *argv[4];
 
-	if (!(argv[0] = core_fsmonitor))
+	if (!core_fsmonitor)
 		return -1;
 
-	snprintf(ver, sizeof(ver), "%d", version);
-	snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update);
-	argv[1] = ver;
-	argv[2] = date;
-	argv[3] = NULL;
-	cp.argv = argv;
+	argv_array_push(&cp.args, core_fsmonitor);
+	argv_array_pushf(&cp.args, "%d", version);
+	argv_array_pushf(&cp.args, "%" PRIuMAX, (uintmax_t)last_update);
 	cp.use_shell = 1;
 	cp.dir = get_git_work_tree();
 

-- 
2.17.0
Jeff King· May 20, 2018, 17:08 UTC · re: René Scharfe · lore

Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers

On Sat, May 19, 2018 at 10:27:46AM +0200, René Scharfe wrote:
Show 10 quoted lines
> Am 19.05.2018 um 03:57 schrieb Jeff King:
> > These formatted integers should always fit into their
> > 64-byte buffers. Let's use xsnprintf() to add an assertion
> > that this is the case, which makes auditing for other
> > unchecked snprintfs() easier.
> 
> How about this instead?
> 
> -- >8 --
> Subject: [PATCH] fsmonitor: use internal argv_array of struct child_process

Yeah, I agree that is much nicer (I was so focused on the snprintfs I didn't bother to look at the context for a better solution).

Thanks, getting a review in the form of a complete patch is my favorite kind of review. :)

> Inspired-by: Jeff King <peff@peff.net>
Heh.
-Peff
Junio C Hamano· May 21, 2018, 00:58 UTC · re: René Scharfe · lore

Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers

René Scharfe <l.s.r@web.de> writes:
Show 18 quoted lines
> How about this instead?
>
> -- >8 --
> Subject: [PATCH] fsmonitor: use internal argv_array of struct child_process
>
> Avoid magic array sizes and indexes by constructing the fsmonitor
> command line using the embedded argv_array of the child_process.  The
> resulting code is shorter and easier to extend.
>
> Getting rid of the snprintf() calls is a bonus -- even though the
> buffers were big enough here to avoid truncation -- as it makes auditing
> the remaining callers easier.
> ...
> -	if (!(argv[0] = core_fsmonitor))
> +	if (!core_fsmonitor)
>  		return -1;
>  
> -	snprintf(ver, sizeof(ver), "%d", version);

I really like this change, as this exact line used to take sizeof(version) instead of sizeof(ver), which has been corrected recently.

Show 7 quoted lines
> -	snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update);
> -	argv[1] = ver;
> -	argv[2] = date;
> -	argv[3] = NULL;
> -	cp.argv = argv;
> +	argv_array_push(&cp.args, core_fsmonitor);
> +	argv_array_pushf(&cp.args, "%d", version);

If it were written like this from the beginning, such a bug would never have happened ;-)

> +	argv_array_pushf(&cp.args, "%" PRIuMAX, (uintmax_t)last_update);
>  	cp.use_shell = 1;
>  	cp.dir = get_git_work_tree();
Ben Peart· May 21, 2018, 12:36 UTC · re: René Scharfe · lore

Re: [PATCH 3/5] query_fsmonitor: use xsnprintf for formatting integers

On 5/19/2018 4:27 AM, René Scharfe wrote:
Show 19 quoted lines
> Am 19.05.2018 um 03:57 schrieb Jeff King:
>> These formatted integers should always fit into their
>> 64-byte buffers. Let's use xsnprintf() to add an assertion
>> that this is the case, which makes auditing for other
>> unchecked snprintfs() easier.
> 
> How about this instead?
> 
> -- >8 --
>   
> -	snprintf(ver, sizeof(ver), "%d", version);
> -	snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update);
> -	argv[1] = ver;
> -	argv[2] = date;
> -	argv[3] = NULL;
> -	cp.argv = argv;
> +	argv_array_push(&cp.args, core_fsmonitor);
> +	argv_array_pushf(&cp.args, "%d", version);
> +	argv_array_pushf(&cp.args, "%" PRIuMAX, (uintmax_t)last_update);
Looks good.  Simpler, cleaner, less error prone.
>   	cp.use_shell = 1;
>   	cp.dir = get_git_work_tree();
>   
>
Jeff King· May 19, 2018, 01:58 UTC · re: Jeff King · lore

[PATCH 4/5] shorten_unambiguous_ref: use xsnprintf

We convert the ref_rev_parse_rules array into scanf formats on the fly, and use snprintf() to write into each string. We should have enough memory to hold everything because of the earlier total_len computation. Let's use xsnprintf() to give runtime confirmation that this is the case, and to make it easy for people auditing the code to know there's no truncation bug.

Signed-off-by: Jeff King <peff@peff.net>
---
 refs.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to refs.c +2 −2
diff --git a/refs.c b/refs.c
index 64aadd14c9..73c446628f 100644
--- a/refs.c
+++ b/refs.c
@@ -1147,8 +1147,8 @@ char *shorten_unambiguous_ref(const char *refname, int strict)
 		for (i = 0; i < nr_rules; i++) {
 			assert(offset < total_len);
 			scanf_fmts[i] = (char *)&scanf_fmts[nr_rules] + offset;
-			offset += snprintf(scanf_fmts[i], total_len - offset,
-					   ref_rev_parse_rules[i], 2, "%s") + 1;
+			offset += xsnprintf(scanf_fmts[i], total_len - offset,
+					    ref_rev_parse_rules[i], 2, "%s") + 1;
 		}
 	}
 

-- 
2.17.0.1052.g7d69f75dbf
Jeff King· May 19, 2018, 01:58 UTC · re: Jeff King · lore

[PATCH 5/5] fmt_with_err: add a comment that truncation is OK

Functions like die_errno() use fmt_with_err() to combine the caller-provided format with the strerror() string. We use a fixed stack buffer because we're already handling an error and don't have any way to report another one. Our buffer should generally be big enough to fit this, but if it's not, truncation is our best option. Let's add a comment to that effect, so that anybody auditing the code for truncation bugs knows that this is fine.

Signed-off-by: Jeff King <peff@peff.net>
---
 usage.c | 1 +
 1 file changed, 1 insertion(+)
Show changes to usage.c +1 −0
diff --git a/usage.c b/usage.c
index cdd534c9df..b3c78931ad 100644
--- a/usage.c
+++ b/usage.c
@@ -148,6 +148,7 @@ static const char *fmt_with_err(char *buf, int n, const char *fmt)
 		}
 	}
 	str_error[j] = 0;
+	/* Truncation is acceptable here */
 	snprintf(buf, n, "%s: %s", fmt, str_error);
 	return buf;
 }

-- 
2.17.0.1052.g7d69f75dbf

← back to recent threads