# [PATCH] alloc_ref(): allow for trailing NUL 9 messages from 2007-09-28 to 2007-09-28. Participants: Johannes Schindelin, Daniel Barkalow, Junio C Hamano, Pierre Habouzit. Thread: https://gitlist.dev/t/10054 ## Johannes Schindelin, 2007-09-28 02:57 Subject: [PATCH] alloc_ref(): allow for trailing NUL Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0709280356550.28395%40racer.site ``` 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 --- 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 ``` ## Daniel Barkalow, 2007-09-28 05:03 Subject: Re: [PATCH] alloc_ref(): allow for trailing NUL Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0709280046241.5926%40iabervon.org In-Reply-To: ``` 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* ``` ## Junio C Hamano, 2007-09-28 08:46 Subject: Re: [PATCH] alloc_ref(): allow for trailing NUL Message-ID: <7vhclfqisq.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vhclfqisq.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Daniel Barkalow 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; ``` ## Johannes Schindelin, 2007-09-28 12:01 Subject: Re: [PATCH] alloc_ref(): allow for trailing NUL Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0709281259050.28395%40racer.site In-Reply-To: <7vhclfqisq.fsf@gitster.siamese.dyndns.org> ``` Hi, On Fri, 28 Sep 2007, Junio C Hamano wrote: > Daniel Barkalow 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 ``` ## Pierre Habouzit, 2007-09-28 12:41 Subject: Re: [PATCH] alloc_ref(): allow for trailing NUL Message-ID: <20070928124102.GA21309@artemis.corp> URL: https://gitlist.dev/e/20070928124102.GA21309%40artemis.corp In-Reply-To: ``` 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. 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); -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org ``` ## Pierre Habouzit, 2007-09-28 13:13 Subject: Re: [PATCH] alloc_ref(): allow for trailing NUL Message-ID: <20070928131348.GB21309@artemis.corp> URL: https://gitlist.dev/e/20070928131348.GB21309%40artemis.corp In-Reply-To: <20070928124102.GA21309@artemis.corp> ``` 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 ``` ## Daniel Barkalow, 2007-09-28 15:44 Subject: Re: [PATCH] alloc_ref(): allow for trailing NUL Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0709281138270.5926%40iabervon.org In-Reply-To: ``` On Fri, 28 Sep 2007, Johannes Schindelin wrote: > Hi, > > On Fri, 28 Sep 2007, Junio C Hamano wrote: > > > Daniel Barkalow 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* ``` ## Johannes Schindelin, 2007-09-28 16:11 Subject: Re: [PATCH] alloc_ref(): allow for trailing NUL Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0709281711010.28395%40racer.site In-Reply-To: ``` 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 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 ``` ## Junio C Hamano, 2007-09-28 18:08 Subject: Re: [PATCH] alloc_ref(): allow for trailing NUL Message-ID: <7v641upsqx.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v641upsqx.fsf%40gitster.siamese.dyndns.org In-Reply-To: ``` Johannes Schindelin 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. ```