[PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf

[PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf

[Elia Pinto]

With the commits f2f02675 and 5096d490 we have been converted in some files the call
from snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls
to snprintf with xsnprintf under the following conditions:

- The call to snprintf does not control the outcome of the command
  or the presence of truncation errors.
- A call to snprintf can generate a fatal error, directly or indirectly.

The other few remaining cases in which a call to snprintf can generate a soft error
have not been changed.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 builtin/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 443ff91..c65abaa 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1552,7 +1552,7 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 	code = start_command(&proc);
 	if (code)
 		return code;
-	n = snprintf(buf, sizeof(buf), "%s %s\n",
+	n = xsnprintf(buf, sizeof(buf), "%s %s\n",
 		     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
 	sigchain_push(SIGPIPE, SIG_IGN);
 	write_in_full(proc.in, buf, n);
-- 
2.9.0.rc1.265.geb5d750

[PATCH 02/10] builtin/index-pack.c: convert trivial snprintf calls to xsnprintf

[Elia Pinto]

With the commits f2f02675 and 5096d490 we have been converted in some files the call
from snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls
to snprintf with xsnprintf under the following conditions:

- The call to snprintf does not control the outcome of the command
  or the presence of truncation errors.
- A call to snprintf can generate a fatal error, directly or indirectly.

The other few remaining cases in which a call to snprintf can generate a soft error
have not been changed.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 builtin/index-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index e8c71fc..c032fe7 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1443,7 +1443,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
 		printf("%s\n", sha1_to_hex(sha1));
 	} else {
 		char buf[48];
-		int len = snprintf(buf, sizeof(buf), "%s\t%s\n",
+		int len = xsnprintf(buf, sizeof(buf), "%s\t%s\n",
 				   report, sha1_to_hex(sha1));
 		write_or_die(1, buf, len);
 
-- 
2.9.0.rc1.265.geb5d750

Re: [PATCH 02/10] builtin/index-pack.c: convert trivial snprintf calls to xsnprintf

[Jeff King]

On Fri, Jun 03, 2016 at 07:47:16AM +0000, Elia Pinto wrote:

> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index e8c71fc..c032fe7 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1443,7 +1443,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>  		printf("%s\n", sha1_to_hex(sha1));
>  	} else {
>  		char buf[48];
> -		int len = snprintf(buf, sizeof(buf), "%s\t%s\n",
> +		int len = xsnprintf(buf, sizeof(buf), "%s\t%s\n",
>  				   report, sha1_to_hex(sha1));
>  		write_or_die(1, buf, len);

So it's pretty unclear here whether that 48 is big enough (it is, if you
read the whole function, because "report" is always a 4-char string).
Yuck. At least there should be a comment explaining why 48 is big
enough.

-Peff

Re: [PATCH 02/10] builtin/index-pack.c: convert trivial snprintf calls to xsnprintf

[Ramsay Jones]

On 03/06/16 09:53, Jeff King wrote:
> On Fri, Jun 03, 2016 at 07:47:16AM +0000, Elia Pinto wrote:
> 
>> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
>> index e8c71fc..c032fe7 100644
>> --- a/builtin/index-pack.c
>> +++ b/builtin/index-pack.c
>> @@ -1443,7 +1443,7 @@ static void final(const char *final_pack_name, const char *curr_pack_name,
>>  		printf("%s\n", sha1_to_hex(sha1));
>>  	} else {
>>  		char buf[48];
>> -		int len = snprintf(buf, sizeof(buf), "%s\t%s\n",
>> +		int len = xsnprintf(buf, sizeof(buf), "%s\t%s\n",
>>  				   report, sha1_to_hex(sha1));
>>  		write_or_die(1, buf, len);
> 
> So it's pretty unclear here whether that 48 is big enough (it is, if you
> read the whole function, because "report" is always a 4-char string).
> Yuck. At least there should be a comment explaining why 48 is big
> enough.

Agreed, again I would use something like:

		char buf[GIT_SHA1_HEXSZ + 7]; /* 40 (sha1) + 4 (report) + 3 (\t\n\0) */

(and yes yuck - is report ever likely to increase? "bitmap" perhaps?)

ATB,
Ramsay Jones

Re: [PATCH 02/10] builtin/index-pack.c: convert trivial snprintf calls to xsnprintf

[Jeff King]

On Fri, Jun 03, 2016 at 04:32:41PM +0100, Ramsay Jones wrote:

> >>  		char buf[48];
> >> -		int len = snprintf(buf, sizeof(buf), "%s\t%s\n",
> >> +		int len = xsnprintf(buf, sizeof(buf), "%s\t%s\n",
> >>  				   report, sha1_to_hex(sha1));
> >>  		write_or_die(1, buf, len);
> > 
> > So it's pretty unclear here whether that 48 is big enough (it is, if you
> > read the whole function, because "report" is always a 4-char string).
> > Yuck. At least there should be a comment explaining why 48 is big
> > enough.
> 
> Agreed, again I would use something like:
> 
> 		char buf[GIT_SHA1_HEXSZ + 7]; /* 40 (sha1) + 4 (report) + 3 (\t\n\0) */

Yes, that's much better, I think.

> (and yes yuck - is report ever likely to increase? "bitmap" perhaps?)

It shouldn't. It's easy to think that's a filetype, but it really is
just "did you tell me --keep". TBH, I am not really sure that switching
it accomplishes anything.

-Peff

[PATCH 03/10] builtin/tag.c: convert trivial snprintf calls to xsnprintf

[Elia Pinto]

With the commits f2f02675 and 5096d490 we have been converted in some files the call
from snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls
to snprintf with xsnprintf under the following conditions:

- The call to snprintf does not control the outcome of the command
  or the presence of truncation errors.
- A call to snprintf can generate a fatal error, directly or indirectly.

The other few remaining cases in which a call to snprintf can generate a soft error
have not been changed.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 builtin/tag.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 50e4ae5..0345ca3 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -225,7 +225,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 	if (type <= OBJ_NONE)
 	    die(_("bad object type."));
 
-	header_len = snprintf(header_buf, sizeof(header_buf),
+	header_len = xsnprintf(header_buf, sizeof(header_buf),
 			  "object %s\n"
 			  "type %s\n"
 			  "tag %s\n"
-- 
2.9.0.rc1.265.geb5d750

Re: [PATCH 03/10] builtin/tag.c: convert trivial snprintf calls to xsnprintf

[Jeff King]

On Fri, Jun 03, 2016 at 07:47:17AM +0000, Elia Pinto wrote:

>  builtin/tag.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 50e4ae5..0345ca3 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -225,7 +225,7 @@ static void create_tag(const unsigned char *object, const char *tag,
>  	if (type <= OBJ_NONE)
>  	    die(_("bad object type."));
>  
> -	header_len = snprintf(header_buf, sizeof(header_buf),
> +	header_len = xsnprintf(header_buf, sizeof(header_buf),
>  			  "object %s\n"
>  			  "type %s\n"
>  			  "tag %s\n"

This is another of my "type 2" cases. I'd argue it should be using a
heap buffer to handle tag and tagger names of arbitrary size.

-Peff

Re: [PATCH 03/10] builtin/tag.c: convert trivial snprintf calls to xsnprintf

[Ramsay Jones]

On 03/06/16 09:52, Jeff King wrote:
> On Fri, Jun 03, 2016 at 07:47:17AM +0000, Elia Pinto wrote:
> 
>>  builtin/tag.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/builtin/tag.c b/builtin/tag.c
>> index 50e4ae5..0345ca3 100644
>> --- a/builtin/tag.c
>> +++ b/builtin/tag.c
>> @@ -225,7 +225,7 @@ static void create_tag(const unsigned char *object, const char *tag,
>>  	if (type <= OBJ_NONE)
>>  	    die(_("bad object type."));
>>  
>> -	header_len = snprintf(header_buf, sizeof(header_buf),
>> +	header_len = xsnprintf(header_buf, sizeof(header_buf),
>>  			  "object %s\n"
>>  			  "type %s\n"
>>  			  "tag %s\n"
> 
> This is another of my "type 2" cases. I'd argue it should be using a
> heap buffer to handle tag and tagger names of arbitrary size.

Yep. As it stands, the code following this hunk:

        if (header_len > sizeof(header_buf) - 1)
                die(_("tag header too big."));

is now dead code, and the new error message is not as useful.

ATB,
Ramsay Jones

[PATCH 04/10] combine-diff.c: convert trivial snprintf calls to xsnprintf

[Elia Pinto]

With the commits f2f02675 and 5096d490 we have been converted in some files the call
from snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls
to snprintf with xsnprintf under the following conditions:

- The call to snprintf does not control the outcome of the command
  or the presence of truncation errors.
- A call to snprintf can generate a fatal error, directly or indirectly.

The other few remaining cases in which a call to snprintf can generate a soft error
have not been changed.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 combine-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/combine-diff.c b/combine-diff.c
index 8f2313d..a91d9b3 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -293,7 +293,7 @@ static char *grab_blob(const struct object_id *oid, unsigned int mode,
 
 	if (S_ISGITLINK(mode)) {
 		blob = xmalloc(100);
-		*size = snprintf(blob, 100,
+		*size = xsnprintf(blob, 100,
 				 "Subproject commit %s\n", oid_to_hex(oid));
 	} else if (is_null_oid(oid)) {
 		/* deleted blob */
-- 
2.9.0.rc1.265.geb5d750

Re: [PATCH 04/10] combine-diff.c: convert trivial snprintf calls to xsnprintf

[Jeff King]

On Fri, Jun 03, 2016 at 07:47:18AM +0000, Elia Pinto wrote:

> diff --git a/combine-diff.c b/combine-diff.c
> index 8f2313d..a91d9b3 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -293,7 +293,7 @@ static char *grab_blob(const struct object_id *oid, unsigned int mode,
>  
>  	if (S_ISGITLINK(mode)) {
>  		blob = xmalloc(100);
> -		*size = snprintf(blob, 100,
> +		*size = xsnprintf(blob, 100,
>  				 "Subproject commit %s\n", oid_to_hex(oid));

This one seems like a no-brainer to convert to xstrmft(), since we're
already using a heap buffer, and then we can get rid of that meaningless
"100" magic number.

-Peff

[PATCH 05/10] compat/inet_ntop.c: convert trivial snprintf calls to xsnprintf

[Elia Pinto]

With the commits f2f02675 and 5096d490 we have been converted in some files the call
from snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls
to snprintf with xsnprintf under the following conditions:

- The call to snprintf does not control the outcome of the command
  or the presence of truncation errors.
- A call to snprintf can generate a fatal error, directly or indirectly.

The other few remaining cases in which a call to snprintf can generate a soft error
have not been changed.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 compat/inet_ntop.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/inet_ntop.c b/compat/inet_ntop.c
index 6830726..a4a6546 100644
--- a/compat/inet_ntop.c
+++ b/compat/inet_ntop.c
@@ -139,7 +139,7 @@ inet_ntop6(const u_char *src, char *dst, size_t size)
 			tp += strlen(tp);
 			break;
 		}
-		tp += snprintf(tp, sizeof tmp - (tp - tmp), "%x", words[i]);
+		tp += xsnprintf(tp, sizeof tmp - (tp - tmp), "%x", words[i]);
 	}
 	/* Was it a trailing run of 0x00's? */
 	if (best.base != -1 && (best.base + best.len) ==
-- 
2.9.0.rc1.265.geb5d750

[PATCH 06/10] diff.c: convert trivial snprintf calls to xsnprintf

[Elia Pinto]

With the commits f2f02675 and 5096d490 we have been converted in some files the call
from snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls
to snprintf with xsnprintf under the following conditions:

- The call to snprintf does not control the outcome of the command
  or the presence of truncation errors.
- A call to snprintf can generate a fatal error, directly or indirectly.

The other few remaining cases in which a call to snprintf can generate a soft error
have not been changed.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 diff.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index d3734d3..fb61539 100644
--- a/diff.c
+++ b/diff.c
@@ -4491,7 +4491,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
 		len1 = remove_space(p->one->path, strlen(p->one->path));
 		len2 = remove_space(p->two->path, strlen(p->two->path));
 		if (p->one->mode == 0)
-			len1 = snprintf(buffer, sizeof(buffer),
+			len1 = xsnprintf(buffer, sizeof(buffer),
 					"diff--gita/%.*sb/%.*s"
 					"newfilemode%06o"
 					"---/dev/null"
@@ -4501,7 +4501,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
 					p->two->mode,
 					len2, p->two->path);
 		else if (p->two->mode == 0)
-			len1 = snprintf(buffer, sizeof(buffer),
+			len1 = xsnprintf(buffer, sizeof(buffer),
 					"diff--gita/%.*sb/%.*s"
 					"deletedfilemode%06o"
 					"---a/%.*s"
@@ -4511,7 +4511,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
 					p->one->mode,
 					len1, p->one->path);
 		else
-			len1 = snprintf(buffer, sizeof(buffer),
+			len1 = xsnprintf(buffer, sizeof(buffer),
 					"diff--gita/%.*sb/%.*s"
 					"---a/%.*s"
 					"+++b/%.*s",
-- 
2.9.0.rc1.265.geb5d750

Re: [PATCH 06/10] diff.c: convert trivial snprintf calls to xsnprintf

[Jeff King]

On Fri, Jun 03, 2016 at 07:47:20AM +0000, Elia Pinto wrote:

>  diff.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/diff.c b/diff.c
> index d3734d3..fb61539 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4491,7 +4491,7 @@ static int diff_get_patch_id(struct diff_options *options, unsigned char *sha1)
>  		len1 = remove_space(p->one->path, strlen(p->one->path));
>  		len2 = remove_space(p->two->path, strlen(p->two->path));
>  		if (p->one->mode == 0)
> -			len1 = snprintf(buffer, sizeof(buffer),
> +			len1 = xsnprintf(buffer, sizeof(buffer),
>  					"diff--gita/%.*sb/%.*s"
>  					"newfilemode%06o"
>  					"---/dev/null"

More that assume that PATH_MAX is plenty big for any patch we would
see. I'd argue these should be on the heap, which removes that
restriction, and gets rid of the "PATH_MAX * 4 + 20" magic at the top of
the function.

It looks like the buffer just gets fed into sha1, so I was also tempted
to suggest we simply feed the bits of string in directly. But we are
using the format string here to handle the mode, so you'd still have to
deal with that.

-Peff

[PATCH 07/10] fast-import.c: convert trivial snprintf calls to xsnprintf

[Elia Pinto]

With the commits f2f02675 and 5096d490 we have been converted in some files the call
from snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls
to snprintf with xsnprintf under the following conditions:

- The call to snprintf does not control the outcome of the command
  or the presence of truncation errors.
- A call to snprintf can generate a fatal error, directly or indirectly.

The other few remaining cases in which a call to snprintf can generate a soft error
have not been changed.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 fast-import.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 59630ce..5beb63d 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1240,10 +1240,7 @@ static void stream_blob(uintmax_t len, unsigned char *sha1out, uintmax_t mark)
 	sha1file_checkpoint(pack_file, &checkpoint);
 	offset = checkpoint.offset;
 
-	hdrlen = snprintf((char *)out_buf, out_sz, "blob %" PRIuMAX, len) + 1;
-	if (out_sz <= hdrlen)
-		die("impossibly large object header");
-
+	hdrlen = xsnprintf((char *)out_buf, out_sz, "blob %" PRIuMAX, len) + 1;
 	git_SHA1_Init(&c);
 	git_SHA1_Update(&c, out_buf, hdrlen);
 
-- 
2.9.0.rc1.265.geb5d750

[PATCH 08/10] refs.c: convert trivial snprintf calls to xsnprintf

[Elia Pinto]

With the commits f2f02675 and 5096d490 we have been converted in some files the call
from snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls
to snprintf with xsnprintf under the following conditions:

- The call to snprintf does not control the outcome of the command
  or the presence of truncation errors.
- A call to snprintf can generate a fatal error, directly or indirectly.

The other few remaining cases in which a call to snprintf can generate a soft error
have not been changed.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 refs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/refs.c b/refs.c
index 87dc82f..c797ea0 100644
--- a/refs.c
+++ b/refs.c
@@ -912,7 +912,7 @@ 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,
+			offset += xsnprintf(scanf_fmts[i], total_len - offset,
 					   ref_rev_parse_rules[i], 2, "%s") + 1;
 		}
 	}
-- 
2.9.0.rc1.265.geb5d750

[PATCH 09/10] transport-helper.c: convert trivial snprintf calls to xsnprintf

[Elia Pinto]

With the commits f2f02675 and 5096d490 we have been converted in some files the call
from snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls
to snprintf with xsnprintf under the following conditions:

- The call to snprintf does not control the outcome of the command
  or the presence of truncation errors.
- A call to snprintf can generate a fatal error, directly or indirectly.

The other few remaining cases in which a call to snprintf can generate a soft error
have not been changed.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 transport-helper.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/transport-helper.c b/transport-helper.c
index bd666b2..18e9f44 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -317,9 +317,7 @@ static void standard_options(struct transport *t)
 
 	set_helper_option(t, "progress", t->progress ? "true" : "false");
 
-	n = snprintf(buf, sizeof(buf), "%d", v + 1);
-	if (n >= sizeof(buf))
-		die("impossibly large verbosity value");
+	n = xsnprintf(buf, sizeof(buf), "%d", v + 1);
 	set_helper_option(t, "verbosity", buf);
 
 	switch (t->family) {
-- 
2.9.0.rc1.265.geb5d750

[PATCH 10/10] wrapper.c: convert trivial snprintf calls to xsnprintf

[Elia Pinto]

With the commits f2f02675 and 5096d490 we have been converted in some files the call
from snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls
to snprintf with xsnprintf under the following conditions:

- The call to snprintf does not control the outcome of the command
  or the presence of truncation errors.
- A call to snprintf can generate a fatal error, directly or indirectly.

The other few remaining cases in which a call to snprintf can generate a soft error
have not been changed.

Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
---
 wrapper.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/wrapper.c b/wrapper.c
index 5dc4e15..0d44835 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -433,16 +433,11 @@ int xmkstemp(char *template)
 int git_mkstemp(char *path, size_t len, const char *template)
 {
 	const char *tmp;
-	size_t n;
 
 	tmp = getenv("TMPDIR");
 	if (!tmp)
 		tmp = "/tmp";
-	n = snprintf(path, len, "%s/%s", tmp, template);
-	if (len <= n) {
-		errno = ENAMETOOLONG;
-		return -1;
-	}
+	(void)xsnprintf(path, len, "%s/%s", tmp, template);
 	return mkstemp(path);
 }
 
-- 
2.9.0.rc1.265.geb5d750

Re: [PATCH 10/10] wrapper.c: convert trivial snprintf calls to xsnprintf

[Jeff King]

On Fri, Jun 03, 2016 at 07:47:24AM +0000, Elia Pinto wrote:

> diff --git a/wrapper.c b/wrapper.c
> index 5dc4e15..0d44835 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -433,16 +433,11 @@ int xmkstemp(char *template)
>  int git_mkstemp(char *path, size_t len, const char *template)
>  {
>  	const char *tmp;
> -	size_t n;
>  
>  	tmp = getenv("TMPDIR");
>  	if (!tmp)
>  		tmp = "/tmp";
> -	n = snprintf(path, len, "%s/%s", tmp, template);
> -	if (len <= n) {
> -		errno = ENAMETOOLONG;
> -		return -1;
> -	}
> +	(void)xsnprintf(path, len, "%s/%s", tmp, template);
>  	return mkstemp(path);
>  }

I don't think this one is a good change. git_mkstemp is a library
function, and there are places that indirectly call it (via the gpg
code) that do not die when it fails (and even if all the callers _did_
die, isn't "could not create temporary file ..." a better error message
than "BUG: attempt to snprintf into too-small buffer"?).

-Peff

Re: [PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf

[Jeff King]

On Fri, Jun 03, 2016 at 07:47:15AM +0000, Elia Pinto wrote:

> With the commits f2f02675 and 5096d490 we have been converted in some files the call
> from snprintf/sprintf/strcpy to xsnprintf. This patch converts the remaining calls
> to snprintf with xsnprintf under the following conditions:
> 
> - The call to snprintf does not control the outcome of the command
>   or the presence of truncation errors.
> - A call to snprintf can generate a fatal error, directly or indirectly.
> 
> The other few remaining cases in which a call to snprintf can generate a soft error
> have not been changed.

I see that all 10 of these commits have the same commit message. IMHO,
that is a good sign that they should just be a single commit.

It is a good idea to break your changes up into small commits, but I
think it only makes sense to do so on _logical_ boundaries. For a
cross code-base change like this, it doesn't really matter what file
these are in. They are all the same logical change; they have the same
prerequisites to be a good candidate for the change, and the mechanism
itself is the same in all cases. Somebody reviewing them would apply the
same criteria in all cases.

I haven't looked carefully at each call site in detail yet, but from the
previous rounds of sprintf handling, I'd guess each site falls into one
of two categories:

  1. We've sized the buffer appropriately earlier in the function, so
     this is a "should never truncate" case. Using xsnprintf gives us a
     run-time assurance that there was no programming error.

  2. The original author didn't give much thought to the buffer size and
     figured "probably big enough". These ones might actually cause
     truncation in pathological cases, but we'd prefer to generate a
     fatal error, since that's better than continuing with bogus data.

But again, that's just a guess. There might be other ways of grouping
the changes logically.

For this particular change:

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 443ff91..c65abaa 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1552,7 +1552,7 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
>  	code = start_command(&proc);
>  	if (code)
>  		return code;
> -	n = snprintf(buf, sizeof(buf), "%s %s\n",
> +	n = xsnprintf(buf, sizeof(buf), "%s %s\n",
>  		     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));

I think it's the first type, as earlier we have:

	/* oldsha1 SP newsha1 LF NUL */
	static char buf[2*40 + 3];

So unless that calculation is wrong, this would never truncate. The move
to xsnprintf is an improvement, but I wonder if we would be happier
still with:

  buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));

Then we do not even have to wonder about the size computation. True, it
uses a heap buffer when we do not need to, but I find it hard to care
about grabbing 80 bytes of heap when we are in the midst of exec-ing an
entirely new process.

By the way, there are some other uses of snprintf in the same file, that
I think would fall into the type 2 I mentioned above (they use PATH_MAX,
which I think is shorter than necessary on some systems).

Those ones would also benefit from using higher-level constructs. Like:

diff --git a/builtin/commit.c b/builtin/commit.c
index 443ff91..9336724 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1563,24 +1563,23 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
 
 int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)
 {
-	const char *hook_env[3] =  { NULL };
-	char index[PATH_MAX];
+	struct argv_array hook_env = ARGV_ARRAY_INIT;
 	va_list args;
 	int ret;
 
-	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
-	hook_env[0] = index;
+	argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
 
 	/*
 	 * Let the hook know that no editor will be launched.
 	 */
 	if (!editor_is_used)
-		hook_env[1] = "GIT_EDITOR=:";
+		argv_array_push(&hook_env, "GIT_EDITOR=:");
 
 	va_start(args, name);
 	ret = run_hook_ve(hook_env, name, args);
 	va_end(args);
 
+	argv_array_clear(&hook_env);
 	return ret;
 }
 

-Peff

Re: [PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf

[Jeff King]

On Fri, Jun 03, 2016 at 04:49:18AM -0400, Jeff King wrote:

> I haven't looked carefully at each call site in detail yet, but from the
> previous rounds of sprintf handling, I'd guess each site falls into one
> of two categories:

So that being said, I did just look at several of them. And I think they
_do_ fall into these categories, but I think the solutions don't
necessarily. So what I would do is find solutions for each, and then go
back and group them by situation and solution-type using "git rebase
-i". It may be that you end up with 10 patches, or it may be that you
see some patterns. But I think grouping is helpful for reviewers,
because it communicates the patterns you found while doing the work.

-Peff

Re: [PATCH 01/10] builtin/commit.c: convert trivial snprintf calls to xsnprintf

[Ramsay Jones]

On 03/06/16 09:49, Jeff King wrote:
> On Fri, Jun 03, 2016 at 07:47:15AM +0000, Elia Pinto wrote:
> 
[snip]

> For this particular change:
> 
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index 443ff91..c65abaa 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1552,7 +1552,7 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
>>  	code = start_command(&proc);
>>  	if (code)
>>  		return code;
>> -	n = snprintf(buf, sizeof(buf), "%s %s\n",
>> +	n = xsnprintf(buf, sizeof(buf), "%s %s\n",
>>  		     sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> 
> I think it's the first type, as earlier we have:
> 
> 	/* oldsha1 SP newsha1 LF NUL */
> 	static char buf[2*40 + 3];
> 
> So unless that calculation is wrong, this would never truncate. The move
> to xsnprintf is an improvement, 

I was going to suggest, if we stay with the static buffer, that the size
expression be changed to '2 * GIT_SHA1_HEXSZ + 3'. However, ...

>                                  but I wonder if we would be happier
> still with:
> 
>   buf = xstrfmt("%s %s\n", sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
> 
> Then we do not even have to wonder about the size computation. True, it
> uses a heap buffer when we do not need to, but I find it hard to care
> about grabbing 80 bytes of heap when we are in the midst of exec-ing an
> entirely new process.

... I agree with this also.

> 
> By the way, there are some other uses of snprintf in the same file, that
> I think would fall into the type 2 I mentioned above (they use PATH_MAX,
> which I think is shorter than necessary on some systems).
> 
> Those ones would also benefit from using higher-level constructs. Like:
> 
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 443ff91..9336724 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1563,24 +1563,23 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
>  
>  int run_commit_hook(int editor_is_used, const char *index_file, const char *name, ...)
>  {
> -	const char *hook_env[3] =  { NULL };
> -	char index[PATH_MAX];
> +	struct argv_array hook_env = ARGV_ARRAY_INIT;
>  	va_list args;
>  	int ret;
>  
> -	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> -	hook_env[0] = index;
> +	argv_array_pushf(&hook_env, "GIT_INDEX_FILE=%s", index_file);
>  
>  	/*
>  	 * Let the hook know that no editor will be launched.
>  	 */
>  	if (!editor_is_used)
> -		hook_env[1] = "GIT_EDITOR=:";
> +		argv_array_push(&hook_env, "GIT_EDITOR=:");
>  
>  	va_start(args, name);
>  	ret = run_hook_ve(hook_env, name, args);
>  	va_end(args);
>  
> +	argv_array_clear(&hook_env);
>  	return ret;
>  }

Indeed.

ATB,
Ramsay Jones