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

Daniel Barkalow [off-list ref] writes:

On Fri, 28 Sep 2007, Johannes Schindelin wrote:

quoted

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;

Hi,

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

Daniel Barkalow [off-list ref] writes:

quoted

On Fri, 28 Sep 2007, Johannes Schindelin wrote:

quoted

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

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

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

On Fri, 28 Sep 2007, Johannes Schindelin wrote:

Hi,

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

quoted

Daniel Barkalow [off-list ref] writes:

quoted

On Fri, 28 Sep 2007, Johannes Schindelin wrote:

quoted

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*

Hi,

On Fri, 28 Sep 2007, Daniel Barkalow wrote:

On Fri, 28 Sep 2007, Johannes Schindelin wrote:

quoted

Hi,

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

quoted

Daniel Barkalow [off-list ref] writes:

quoted

On Fri, 28 Sep 2007, Johannes Schindelin wrote:

quoted

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