Re: [PATCH] alloc_ref(): allow for trailing NUL
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Sep 28, 2007, 12:01 UTC
- Message-ID
- <Pine.LNX.4.64.0709281259050.28395@racer.site>
- In-Reply-To
- <7vhclfqisq.fsf@gitster.siamese.dyndns.org>
Hi,
On Fri, 28 Sep 2007, Junio C Hamano wrote:
Show 31 quoted lines
> 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