[PATCH] alloc_ref(): allow for trailing NUL

[PATCH] alloc_ref(): allow for trailing NUL

[Johannes Schindelin]

The parameter name "namelen" suggests that you pass the equivalent of
strlen() to the function alloc_ref().  However, this function did not
allocate enough space to put a NUL after the name.

Since struct ref does not have any member to describe the length of the
string, this just does not make sense.

So make space for the NUL.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 remote.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/remote.c b/remote.c
index e7d735b..7531c92 100644
--- a/remote.c
+++ b/remote.c
@@ -453,7 +453,7 @@ int remote_find_tracking(struct remote *remote, struct refspec *refspec)
 
 struct ref *alloc_ref(unsigned namelen)
 {
-	struct ref *ret = xmalloc(sizeof(struct ref) + namelen);
+	struct ref *ret = xmalloc(sizeof(struct ref) + namelen + 1);
 	memset(ret, 0, sizeof(struct ref) + namelen);
 	return ret;
 }
-- 
1.5.3.2.1102.g9487

Re: [PATCH] alloc_ref(): allow for trailing NUL

[Daniel Barkalow]

On Fri, 28 Sep 2007, Johannes Schindelin wrote:

> The parameter name "namelen" suggests that you pass the equivalent of
> strlen() to the function alloc_ref().  However, this function did not
> allocate enough space to put a NUL after the name.
> 
> Since struct ref does not have any member to describe the length of the
> string, this just does not make sense.
> 
> So make space for the NUL.

Good point, but shouldn't you then fix call sites that use strlen(name) + 
1?

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] alloc_ref(): allow for trailing NUL

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> On Fri, 28 Sep 2007, Johannes Schindelin wrote:
>
>> The parameter name "namelen" suggests that you pass the equivalent of
>> strlen() to the function alloc_ref().  However, this function did not
>> allocate enough space to put a NUL after the name.
>> 
>> Since struct ref does not have any member to describe the length of the
>> string, this just does not make sense.
>> 
>> So make space for the NUL.
>
> Good point, but shouldn't you then fix call sites that use strlen(name) + 
> 1?

Good point.

I audited "git grep -A2 -B4 -e alloc_ref next master" output,
and it appears almost everybody knows alloc_ref() wants the
caller to count the terminating NUL.

There however are a few gotchas.

 * There is one overallocation in connect.c, which would not
   hurt but is wasteful;

 * next:transport.c has alloc_ref(strlen(e->name)) which is a
   no-no;

Discarding Johannes's patch, the following would fix it.

---

 connect.c   |    4 ++--
 transport.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/connect.c b/connect.c
index 06d279e..3d5c4ab 100644
--- a/connect.c
+++ b/connect.c
@@ -72,9 +72,9 @@ struct ref **get_remote_heads(int in, struct ref **list,
 			continue;
 		if (nr_match && !path_match(name, nr_match, match))
 			continue;
-		ref = alloc_ref(len - 40);
+		ref = alloc_ref(name_len + 1);
 		hashcpy(ref->old_sha1, old_sha1);
-		memcpy(ref->name, buffer + 41, len - 40);
+		memcpy(ref->name, buffer + 41, name_len + 1);
 		*list = ref;
 		list = &ref->next;
 	}
diff --git a/transport.c b/transport.c
index 4f9cddc..3475cca 100644
--- a/transport.c
+++ b/transport.c
@@ -215,7 +215,7 @@ static struct ref *get_refs_from_bundle(const struct transport *transport)
 		die ("Could not read bundle '%s'.", transport->url);
 	for (i = 0; i < data->header.references.nr; i++) {
 		struct ref_list_entry *e = data->header.references.list + i;
-		struct ref *ref = alloc_ref(strlen(e->name));
+		struct ref *ref = alloc_ref(strlen(e->name) + 1);
 		hashcpy(ref->old_sha1, e->sha1);
 		strcpy(ref->name, e->name);
 		ref->next = result;

Re: [PATCH] alloc_ref(): allow for trailing NUL

[Johannes Schindelin]

Hi,

On Fri, 28 Sep 2007, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > On Fri, 28 Sep 2007, Johannes Schindelin wrote:
> >
> >> The parameter name "namelen" suggests that you pass the equivalent of
> >> strlen() to the function alloc_ref().  However, this function did not
> >> allocate enough space to put a NUL after the name.
> >> 
> >> Since struct ref does not have any member to describe the length of the
> >> string, this just does not make sense.
> >> 
> >> So make space for the NUL.
> >
> > Good point, but shouldn't you then fix call sites that use strlen(name) + 
> > 1?
> 
> Good point.
> 
> I audited "git grep -A2 -B4 -e alloc_ref next master" output,
> and it appears almost everybody knows alloc_ref() wants the
> caller to count the terminating NUL.
> 
> There however are a few gotchas.
> 
>  * There is one overallocation in connect.c, which would not
>    hurt but is wasteful;
> 
>  * next:transport.c has alloc_ref(strlen(e->name)) which is a
>    no-no;
> 
> Discarding Johannes's patch, the following would fix it.

But should the signature of alloc_ref() not be changed, then, to read

	struct ref *alloc_ref(unsigned name_alloc);

Hm?

Further, I am quite sure that the same mistake will happen again, until we 
change the function to get the name length, not the number of bytes to 
allocate.

Ciao,
Dscho

Re: [PATCH] alloc_ref(): allow for trailing NUL

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2898 bytes --]

On Fri, Sep 28, 2007 at 12:01:28PM +0000, Johannes Schindelin wrote:
> But should the signature of alloc_ref() not be changed, then, to read
> 
> 	struct ref *alloc_ref(unsigned name_alloc);
> 
> Hm?
> 
> Further, I am quite sure that the same mistake will happen again, until we 
> change the function to get the name length, not the number of bytes to 
> allocate.

<janitor cap on>

  While being at it, I noticed that the following code pattern is used
in many places in git:

  struct something *ptr = xmalloc(sizeof(struct something) + some_string_len + 1);
  memset(ptr, 0, sizeof(struct something));
  memcpy(ptr->name, somestring, some_string_len + 1);

  With name being a flex array.

  Maybe we could deal with all of these, and make a generic construction
that would take care of allocating the proper space

  I believe we could have function doing that, that would factorize this
code, and use a nice api à la xmemdupz for this.

  It would be something like:

	void *xflexdupz(size_t offset, void *src, size_t len)
	{
		char *p = xmalloc(offset + len + 1);
		memset(p, 0, offset);
		memcpy(p + offset, src, len);
		p[offset + len] = '\0';
		return p;
	}

  Then alloc_ref could be a wrapper around:
  xflexdupz(offsetof(struct ref, name), ..., ...).

  Of course right now alloc_ref doesn't perform any kind of copy, but a
grep -A1 will convince you that it's not a problem:

	$ grep -A1 alloc_ref **/*.[hc]
	builtin-fetch.c:			rm = alloc_ref(strlen(ref_name) + 1);
	builtin-fetch.c-			strcpy(rm->name, ref_name);
	builtin-fetch.c:			rm->peer_ref = alloc_ref(strlen(ref_name) + 1);
	builtin-fetch.c-			strcpy(rm->peer_ref->name, ref_name);
	--
	connect.c:		ref = alloc_ref(len - 40);
	connect.c-		hashcpy(ref->old_sha1, old_sha1);
	--
	remote.c:struct ref *alloc_ref(unsigned namelen)
	remote.c-{
	--
	remote.c:		ref = alloc_ref(20);
	remote.c-		strcpy(ref->name, "(delete)");
	--
	remote.c:	ref = alloc_ref(len);
	remote.c-	memcpy(ref->name, name, len);
	--
	remote.c:	ret = alloc_ref(len);
	remote.c-	memcpy(ret->name, name, len);
	--
	remote.c:			cpy->peer_ref = alloc_ref(local_prefix_len +
	remote.c-						  strlen(match) + 1);
	--
	remote.c:		ret = alloc_ref(strlen(name) + 1);
	remote.c-		strcpy(ret->name, name);
	--
	remote.c:		ret = alloc_ref(strlen(name) + 6);
	remote.c-		sprintf(ret->name, "refs/%s", name);
	--
	remote.c:	ret = alloc_ref(strlen(name) + 12);
	remote.c-	sprintf(ret->name, "refs/heads/%s", name);
	--
	remote.h:struct ref *alloc_ref(unsigned namelen);
	remote.h-
	--
	transport.c:		struct ref *ref = alloc_ref(strlen(e->name));
	transport.c-		hashcpy(ref->old_sha1, e->sha1);

</janitor>

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] alloc_ref(): allow for trailing NUL

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 883 bytes --]

On Fri, Sep 28, 2007 at 12:41:02PM +0000, Pierre Habouzit wrote:
> 	void *xflexdupz(size_t offset, void *src, size_t len)
> 	{
> 		char *p = xmalloc(offset + len + 1);
> 		memset(p, 0, offset);
> 		memcpy(p + offset, src, len);
> 		p[offset + len] = '\0';
> 		return p;
> 	}
> 
>   Then alloc_ref could be a wrapper around:
>   xflexdupz(offsetof(struct ref, name), ..., ...).
> 
>   Of course right now alloc_ref doesn't perform any kind of copy, but a
> grep -A1 will convince you that it's not a problem:

  [...]
> 	remote.c:		ret = alloc_ref(strlen(name) + 6);
> 	remote.c-		sprintf(ret->name, "refs/%s", name);

  okay forget about me, my proposal doesn't work, too bad :)



-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] alloc_ref(): allow for trailing NUL

[Daniel Barkalow]

On Fri, 28 Sep 2007, Johannes Schindelin wrote:

> Hi,
> 
> On Fri, 28 Sep 2007, Junio C Hamano wrote:
> 
> > Daniel Barkalow <barkalow@iabervon.org> writes:
> > 
> > > On Fri, 28 Sep 2007, Johannes Schindelin wrote:
> > >
> > >> The parameter name "namelen" suggests that you pass the equivalent of
> > >> strlen() to the function alloc_ref().  However, this function did not
> > >> allocate enough space to put a NUL after the name.
> > >> 
> > >> Since struct ref does not have any member to describe the length of the
> > >> string, this just does not make sense.
> > >> 
> > >> So make space for the NUL.
> > >
> > > Good point, but shouldn't you then fix call sites that use strlen(name) + 
> > > 1?
> > 
> > Good point.
> > 
> > I audited "git grep -A2 -B4 -e alloc_ref next master" output,
> > and it appears almost everybody knows alloc_ref() wants the
> > caller to count the terminating NUL.
> > 
> > There however are a few gotchas.
> > 
> >  * There is one overallocation in connect.c, which would not
> >    hurt but is wasteful;
> > 
> >  * next:transport.c has alloc_ref(strlen(e->name)) which is a
> >    no-no;
> > 
> > Discarding Johannes's patch, the following would fix it.
> 
> But should the signature of alloc_ref() not be changed, then, to read
> 
> 	struct ref *alloc_ref(unsigned name_alloc);
> 
> Hm?
> 
> Further, I am quite sure that the same mistake will happen again, until we 
> change the function to get the name length, not the number of bytes to 
> allocate.

I agree. But leaving the majority of cases using the old convention is 
just confusing.

	-Daniel
*This .sig left intentionally blank*

Re: [PATCH] alloc_ref(): allow for trailing NUL

[Johannes Schindelin]

Hi,

On Fri, 28 Sep 2007, Daniel Barkalow wrote:

> On Fri, 28 Sep 2007, Johannes Schindelin wrote:
> 
> > Hi,
> > 
> > On Fri, 28 Sep 2007, Junio C Hamano wrote:
> > 
> > > Daniel Barkalow <barkalow@iabervon.org> writes:
> > > 
> > > > On Fri, 28 Sep 2007, Johannes Schindelin wrote:
> > > >
> > > >> The parameter name "namelen" suggests that you pass the equivalent of
> > > >> strlen() to the function alloc_ref().  However, this function did not
> > > >> allocate enough space to put a NUL after the name.
> > > >> 
> > > >> Since struct ref does not have any member to describe the length of the
> > > >> string, this just does not make sense.
> > > >> 
> > > >> So make space for the NUL.
> > > >
> > > > Good point, but shouldn't you then fix call sites that use strlen(name) + 
> > > > 1?
> > > 
> > > Good point.
> > > 
> > > I audited "git grep -A2 -B4 -e alloc_ref next master" output,
> > > and it appears almost everybody knows alloc_ref() wants the
> > > caller to count the terminating NUL.
> > > 
> > > There however are a few gotchas.
> > > 
> > >  * There is one overallocation in connect.c, which would not
> > >    hurt but is wasteful;
> > > 
> > >  * next:transport.c has alloc_ref(strlen(e->name)) which is a
> > >    no-no;
> > > 
> > > Discarding Johannes's patch, the following would fix it.
> > 
> > But should the signature of alloc_ref() not be changed, then, to read
> > 
> > 	struct ref *alloc_ref(unsigned name_alloc);
> > 
> > Hm?
> > 
> > Further, I am quite sure that the same mistake will happen again, until we 
> > change the function to get the name length, not the number of bytes to 
> > allocate.
> 
> I agree. But leaving the majority of cases using the old convention is 
> just confusing.

Yeah, sorry, that patch was only half-cooked.

If people agree with me, I'll redo the patch (fixing all calling sites, 
too).

Ciao,
Dscho

Re: [PATCH] alloc_ref(): allow for trailing NUL

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Fri, 28 Sep 2007, Daniel Barkalow wrote:
>
>> On Fri, 28 Sep 2007, Johannes Schindelin wrote:
>> 
>> > Further, I am quite sure that the same mistake will happen again, until we 
>> > change the function to get the name length, not the number of bytes to 
>> > allocate.
>> 
>> I agree. But leaving the majority of cases using the old convention is 
>> just confusing.
>
> Yeah, sorry, that patch was only half-cooked.
>
> If people agree with me, I'll redo the patch (fixing all calling sites, 
> too).

I think that is probably a better solution.