[PATCH 0/5] snprintf truncation fixes

[PATCH 0/5] snprintf truncation fixes

[Jeff King]

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

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

[Jeff King]

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(-)

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

[PATCH 2/5] log_write_email_headers: use strbufs

[Jeff King]

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

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

[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.

Signed-off-by: Jeff King <peff@peff.net>
---
 fsmonitor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

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

[PATCH 4/5] shorten_unambiguous_ref: use xsnprintf

[Jeff King]

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(-)

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

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

[Jeff King]

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(+)

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

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

[René Scharfe]

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(-)

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

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

[Jeff King]

On Sat, May 19, 2018 at 10:27:46AM +0200, René Scharfe wrote:

> 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

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> 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.

> -	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();

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

[Ben Peart]

On 5/19/2018 4:27 AM, René Scharfe wrote:
> 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();
>   
> 

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

[Stefan Beller]

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?

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

[Jeff King]

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);
 }

Or am I missing something?

-Peff

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

[Stefan Beller]

On Mon, May 21, 2018 at 12:41 PM, Jeff King <peff@peff.net> wrote:
> 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