Re: [PATCH 07/18] avoid using fixed PATH_MAX buffers for refs

[Junio C Hamano]

Jeff King  writes:

> On Sun, Apr 16, 2017 at 11:00:25PM -0700, Junio C Hamano wrote:
>
>> > diff --git a/builtin/replace.c b/builtin/replace.c
>> > index f83e7b8fc..065515bab 100644
>> > --- a/builtin/replace.c
>> > +++ b/builtin/replace.c
>> > @@ -93,26 +93,31 @@ typedef int (*each_replace_name_fn)(const char *name, 
>> > const char *ref,
>> >  static int for_each_replace_name(const char **argv, each_replace_name_fn 
>> > fn)
>> [...]
>> 
>> Don't we need to strbuf_release() before leaving this function?
>
> Yes, good catch. Squashable patch is below.
>
> I'm not sure how I missed that one. I double-checked the other hunks in
> the patch and they are all fine.

Heh, I do not know how I missed that one while queuing, and while
advancing the topic to 'next'.  The third time I read over before
merging to 'master' was when I noticed it.

>
> diff --git a/builtin/replace.c b/builtin/replace.c
> index 065515bab..ab17668f4 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -120,6 +120,7 @@ static int for_each_replace_name(const char **argv, 
> each_replace_name_fn fn)
>   if (fn(full_hex, ref.buf, ))
>   had_error = 1;
>   }
> + strbuf_release();
>   return had_error;
>  }
>  

Re: [PATCH 07/18] avoid using fixed PATH_MAX buffers for refs

[Jeff King]

On Sun, Apr 16, 2017 at 11:00:25PM -0700, Junio C Hamano wrote:

> > diff --git a/builtin/replace.c b/builtin/replace.c
> > index f83e7b8fc..065515bab 100644
> > --- a/builtin/replace.c
> > +++ b/builtin/replace.c
> > @@ -93,26 +93,31 @@ typedef int (*each_replace_name_fn)(const char *name, 
> > const char *ref,
> >  static int for_each_replace_name(const char **argv, each_replace_name_fn 
> > fn)
> [...]
> 
> Don't we need to strbuf_release() before leaving this function?

Yes, good catch. Squashable patch is below.

I'm not sure how I missed that one. I double-checked the other hunks in
the patch and they are all fine.

diff --git a/builtin/replace.c b/builtin/replace.c
index 065515bab..ab17668f4 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -120,6 +120,7 @@ static int for_each_replace_name(const char **argv, 
each_replace_name_fn fn)
if (fn(full_hex, ref.buf, ))
had_error = 1;
}
+   strbuf_release();
return had_error;
 }
 

Re: [PATCH 07/18] avoid using fixed PATH_MAX buffers for refs

[Junio C Hamano]

Jeff King  writes:

> diff --git a/builtin/replace.c b/builtin/replace.c
> index f83e7b8fc..065515bab 100644
> --- a/builtin/replace.c
> +++ b/builtin/replace.c
> @@ -93,26 +93,31 @@ typedef int (*each_replace_name_fn)(const char *name, 
> const char *ref,
>  static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
>  {
>   const char **p, *full_hex;
> - char ref[PATH_MAX];
> + struct strbuf ref = STRBUF_INIT;
> + size_t base_len;
>   int had_error = 0;
>   struct object_id oid;
>  
> + strbuf_addstr(, git_replace_ref_base);
> + base_len = ref.len;
> +
>   for (p = argv; *p; p++) {
>   if (get_oid(*p, )) {
>   error("Failed to resolve '%s' as a valid ref.", *p);
>   had_error = 1;
>   continue;
>   }
> - full_hex = oid_to_hex();
> - snprintf(ref, sizeof(ref), "%s%s", git_replace_ref_base, 
> full_hex);
> - /* read_ref() may reuse the buffer */
> - full_hex = ref + strlen(git_replace_ref_base);
> - if (read_ref(ref, oid.hash)) {
> +
> + strbuf_setlen(, base_len);
> + strbuf_addstr(, oid_to_hex());
> + full_hex = ref.buf + base_len;
> +
> + if (read_ref(ref.buf, oid.hash)) {
>   error("replace ref '%s' not found.", full_hex);
>   had_error = 1;
>   continue;
>   }
> - if (fn(full_hex, ref, ))
> + if (fn(full_hex, ref.buf, ))
>   had_error = 1;
>   }
>   return had_error;

Don't we need to strbuf_release() before leaving this function?

[PATCH 07/18] avoid using fixed PATH_MAX buffers for refs

[Jeff King]

Many functions which handle refs use a PATH_MAX-sized buffer
to do so. This is mostly reasonable as we have to write
loose refs into the filesystem, and at least on Linux the 4K
PATH_MAX is big enough that nobody would care. But:

  1. The static PATH_MAX is not always the filesystem limit.

  2. On other platforms, PATH_MAX may be much smaller.

  3. As we move to alternate ref storage, we won't be bound
 by filesystem limits.

Let's convert these to heap buffers so we don't have to
worry about truncation or size limits.

We may want to eventually constrain ref lengths for sanity
and to prevent malicious names, but we should do so
consistently across all platforms, and in a central place
(like the ref code).

Signed-off-by: Jeff King 
---
 builtin/checkout.c  |  5 ++---
 builtin/ls-remote.c | 10 ++
 builtin/replace.c   | 50 +++---
 builtin/tag.c   | 15 ++-
 4 files changed, 41 insertions(+), 39 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 81f07c3ef..93e8dfc82 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -889,11 +889,10 @@ static int check_tracking_name(struct remote *remote, 
void *cb_data)
 static const char *unique_tracking_name(const char *name, struct object_id 
*oid)
 {
struct tracking_name_data cb_data = { NULL, NULL, NULL, 1 };
-   char src_ref[PATH_MAX];
-   snprintf(src_ref, PATH_MAX, "refs/heads/%s", name);
-   cb_data.src_ref = src_ref;
+   cb_data.src_ref = xstrfmt("refs/heads/%s", name);
cb_data.dst_oid = oid;
for_each_remote(check_tracking_name, _data);
+   free(cb_data.src_ref);
if (cb_data.unique)
return cb_data.dst_ref;
free(cb_data.dst_ref);
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 66cdd45cc..b2d7d5ce6 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -17,17 +17,19 @@ static const char * const ls_remote_usage[] = {
 static int tail_match(const char **pattern, const char *path)
 {
const char *p;
-   char pathbuf[PATH_MAX];
+   char *pathbuf;
 
if (!pattern)
return 1; /* no restriction */
 
-   if (snprintf(pathbuf, sizeof(pathbuf), "/%s", path) > sizeof(pathbuf))
-   return error("insanely long ref %.*s...", 20, path);
+   pathbuf = xstrfmt("/%s", path);
while ((p = *(pattern++)) != NULL) {
-   if (!wildmatch(p, pathbuf, 0, NULL))
+   if (!wildmatch(p, pathbuf, 0, NULL)) {
+   free(pathbuf);
return 1;
+   }
}
+   free(pathbuf);
return 0;
 }
 
diff --git a/builtin/replace.c b/builtin/replace.c
index f83e7b8fc..065515bab 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -93,26 +93,31 @@ typedef int (*each_replace_name_fn)(const char *name, const 
char *ref,
 static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
 {
const char **p, *full_hex;
-   char ref[PATH_MAX];
+   struct strbuf ref = STRBUF_INIT;
+   size_t base_len;
int had_error = 0;
struct object_id oid;
 
+   strbuf_addstr(, git_replace_ref_base);
+   base_len = ref.len;
+
for (p = argv; *p; p++) {
if (get_oid(*p, )) {
error("Failed to resolve '%s' as a valid ref.", *p);
had_error = 1;
continue;
}
-   full_hex = oid_to_hex();
-   snprintf(ref, sizeof(ref), "%s%s", git_replace_ref_base, 
full_hex);
-   /* read_ref() may reuse the buffer */
-   full_hex = ref + strlen(git_replace_ref_base);
-   if (read_ref(ref, oid.hash)) {
+
+   strbuf_setlen(, base_len);
+   strbuf_addstr(, oid_to_hex());
+   full_hex = ref.buf + base_len;
+
+   if (read_ref(ref.buf, oid.hash)) {
error("replace ref '%s' not found.", full_hex);
had_error = 1;
continue;
}
-   if (fn(full_hex, ref, ))
+   if (fn(full_hex, ref.buf, ))
had_error = 1;
}
return had_error;
@@ -129,21 +134,18 @@ static int delete_replace_ref(const char *name, const 
char *ref,
 
 static void check_ref_valid(struct object_id *object,
struct object_id *prev,
-   char *ref,
-   int ref_size,
+   struct strbuf *ref,
int force)
 {
-   if (snprintf(ref, ref_size,
-"%s%s", git_replace_ref_base,
-oid_to_hex(object)) > ref_size - 1)
-   die("replace ref name too long: %.*s...", 50, ref);
-   if (check_refname_format(ref, 0))
-   die("'%s' is not a valid ref name.",