threads / patch / 7744

patchgit-fetch--tool and "insanely" long actions

Subject: [RFD PATCH] git-fetch--tool and "insanely" long actions

## tl;dr

4 messages between Apr 20, 2007 and Apr 20, 2007. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

A Large Angry SCM· Apr 20, 2007, 01:05 UTC · lore

This fixes a problem my repository mirroring script has been having since the git-fetch--tool was added to master in the middle of March. However, it is not a proper fix since it causes actual errors from snprintf() to be ignored. A proper fix is complicated by the lack of a consistent indicator that the buffer is too small across snprintf() implementations.

 builtin-fetch--tool.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

Show changes to builtin-fetch--tool.c +1 −1

diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index e9d6764..173dd4f 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -44,7 +44,7 @@ static int update_ref(const char *action,
 		rla = "(reflog update)";
 	len = snprintf(msg, sizeof(msg), "%s: %s", rla, action);
 	if (sizeof(msg) <= len)
-		die("insanely long action");
+		msg[sizeof(msg)-1] = '\0';
 	lock = lock_any_ref_for_update(refname, oldval);
 	if (!lock)
 		return 1;

Julian Phillips· Apr 20, 2007, 01:34 UTC · re: A Large Angry SCM · lore

Re: [RFD PATCH] git-fetch--tool and "insanely" long actions

On Thu, 19 Apr 2007, A Large Angry SCM wrote:

Show 5 quoted lines

> This fixes a problem my repository mirroring script has been having since
> the git-fetch--tool was added to master in the middle of March. However,
> it is not a proper fix since it causes actual errors from snprintf() to be
> ignored. A proper fix is complicated by the lack of a consistent indicator
> that the buffer is too small across snprintf() implementations.

. . .

>       if (sizeof(msg) <= len)
> -             die("insanely long action");
> +             msg[sizeof(msg)-1] = '\0';

Or you could just let the whole thing through?

Show changes to builtin-fetch--tool.c +12 −7

diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index e9d6764..9b5ae9f 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -36,21 +36,26 @@ static int update_ref(const char *action,
 		      unsigned char *oldval)
 {
 	int len;
-	char msg[1024];
+	char buffer[1024];
+	int ret = 0;
+	char *msg = buffer;
 	char *rla = getenv("GIT_REFLOG_ACTION");
 	static struct ref_lock *lock;
 
 	if (!rla)
 		rla = "(reflog update)";
-	len = snprintf(msg, sizeof(msg), "%s: %s", rla, action);
-	if (sizeof(msg) <= len)
-		die("insanely long action");
+	len = strlen(rla) + strlen(action) + 3;
+	if (len > sizeof(buffer))
+		msg = xmalloc(len);
+	snprintf(msg, len, "%s: %s", rla, action);
 	lock = lock_any_ref_for_update(refname, oldval);
 	if (!lock)
-		return 1;
+		ret = 1;
 	if (write_ref_sha1(lock, sha1, msg) < 0)
-		return 1;
-	return 0;
+		ret = 1;
+	if (msg != buffer)
+		free(msg);
+	return ret;
 }
 
 static int update_local_ref(const char *name,

-- 
1.5.1.1

A Large Angry SCM· Apr 20, 2007, 01:53 UTC · re: Julian Phillips · lore

Re: [RFD PATCH] git-fetch--tool and "insanely" long actions

Julian Phillips wrote:

Show 54 quoted lines

> On Thu, 19 Apr 2007, A Large Angry SCM wrote:
> 
>> This fixes a problem my repository mirroring script has been having since
>> the git-fetch--tool was added to master in the middle of March. However,
>> it is not a proper fix since it causes actual errors from snprintf() to be
>> ignored. A proper fix is complicated by the lack of a consistent indicator
>> that the buffer is too small across snprintf() implementations.
> .
> .
> .
>>       if (sizeof(msg) <= len)
>> -             die("insanely long action");
>> +             msg[sizeof(msg)-1] = '\0';
> 
> Or you could just let the whole thing through?
> 
> diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
> index e9d6764..9b5ae9f 100644
> --- a/builtin-fetch--tool.c
> +++ b/builtin-fetch--tool.c
> @@ -36,21 +36,26 @@ static int update_ref(const char *action,
>  		      unsigned char *oldval)
>  {
>  	int len;
> -	char msg[1024];
> +	char buffer[1024];
> +	int ret = 0;
> +	char *msg = buffer;
>  	char *rla = getenv("GIT_REFLOG_ACTION");
>  	static struct ref_lock *lock;
>  
>  	if (!rla)
>  		rla = "(reflog update)";
> -	len = snprintf(msg, sizeof(msg), "%s: %s", rla, action);
> -	if (sizeof(msg) <= len)
> -		die("insanely long action");
> +	len = strlen(rla) + strlen(action) + 3;
> +	if (len > sizeof(buffer))
> +		msg = xmalloc(len);
> +	snprintf(msg, len, "%s: %s", rla, action);
>  	lock = lock_any_ref_for_update(refname, oldval);
>  	if (!lock)
> -		return 1;
> +		ret = 1;
>  	if (write_ref_sha1(lock, sha1, msg) < 0)
> -		return 1;
> -	return 0;
> +		ret = 1;
> +	if (msg != buffer)
> +		free(msg);
> +	return ret;
>  }
>  
>  static int update_local_ref(const char *name,

See the last sentence in my original message. Yours also ignores errors from snprintf().

Julian Phillips· Apr 20, 2007, 07:40 UTC · re: A Large Angry SCM · lore

Re: [RFD PATCH] git-fetch--tool and "insanely" long actions

On Thu, 19 Apr 2007, A Large Angry SCM wrote:

Show 11 quoted lines

> Julian Phillips wrote:
>>  On Thu, 19 Apr 2007, A Large Angry SCM wrote:
>> 
>> >  This fixes a problem my repository mirroring script has been having 
>> >  since
>> >  the git-fetch--tool was added to master in the middle of March. However,
>> >  it is not a proper fix since it causes actual errors from snprintf() to 
>> >  be
>> >  ignored. A proper fix is complicated by the lack of a consistent 
>> >  indicator
>> >  that the buffer is too small across snprintf() implementations.

>
> See the last sentence in my original message. Yours also ignores errors from 
> snprintf().
>

Well your last sentence was about the buffer being too small. The change I made means it won't ever be too small. True that it doesn't check for other errors.

-- 
Julian

  ---
You will be successful in your work.