{"thread":{"id":"10054","subject":"[PATCH] alloc_ref(): allow for trailing NUL","startedAt":"2007-09-28T02:57:17Z","lastAt":"2007-09-28T18:08:54Z","messageCount":9,"participants":["Johannes Schindelin","Daniel Barkalow","Junio C Hamano","Pierre Habouzit"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"54223","messageId":"Pine.LNX.4.64.0709280356550.28395@racer.site","threadId":"10054","inReplyTo":null,"subject":"[PATCH] alloc_ref(): allow for trailing NUL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-28T02:57:17Z","receivedAt":"2007-09-28T02:57:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThe parameter name \"namelen\" suggests that you pass the equivalent of\nstrlen() to the function alloc_ref(). However, this function did not\nallocate enough space to put a NUL after the name.\n\nSince struct ref does not have any member to describe the length of the\nstring, this just does not make sense.\n\nSo make space for the NUL.\n\nSigned-off-by: Johannes Schindelin \n---\n remote.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex e7d735b..7531c92 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -453,7 +453,7 @@ int remote_find_tracking(struct remote *remote, struct refspec *refspec)\n \n struct ref *alloc_ref(unsigned namelen)\n {\n-\tstruct ref *ret = xmalloc(sizeof(struct ref) + namelen);\n+\tstruct ref *ret = xmalloc(sizeof(struct ref) + namelen + 1);\n \tmemset(ret, 0, sizeof(struct ref) + namelen);\n \treturn ret;\n }\n-- \n1.5.3.2.1102.g9487\n"},{"id":"54231","messageId":"Pine.LNX.4.64.0709280046241.5926@iabervon.org","threadId":"10054","inReplyTo":"Pine.LNX.4.64.0709280356550.28395@racer.site","subject":"Re: [PATCH] alloc_ref(): allow for trailing NUL","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-09-28T05:03:46Z","receivedAt":"2007-09-28T05:03:46Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 28 Sep 2007, Johannes Schindelin wrote:\n\n> The parameter name \"namelen\" suggests that you pass the equivalent of\n> strlen() to the function alloc_ref(). However, this function did not\n> allocate enough space to put a NUL after the name.\n> \n> Since struct ref does not have any member to describe the length of the\n> string, this just does not make sense.\n> \n> So make space for the NUL.\n\nGood point, but shouldn't you then fix call sites that use strlen(name) + \n1?\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"54247","messageId":"7vhclfqisq.fsf@gitster.siamese.dyndns.org","threadId":"10054","inReplyTo":"Pine.LNX.4.64.0709280046241.5926@iabervon.org","subject":"Re: [PATCH] alloc_ref(): allow for trailing NUL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-28T08:46:13Z","receivedAt":"2007-09-28T08:46:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow writes:\n\n> On Fri, 28 Sep 2007, Johannes Schindelin wrote:\n>\n>> The parameter name \"namelen\" suggests that you pass the equivalent of\n>> strlen() to the function alloc_ref(). However, this function did not\n>> allocate enough space to put a NUL after the name.\n>> \n>> Since struct ref does not have any member to describe the length of the\n>> string, this just does not make sense.\n>> \n>> So make space for the NUL.\n>\n> Good point, but shouldn't you then fix call sites that use strlen(name) + \n> 1?\n\nGood point.\n\nI audited \"git grep -A2 -B4 -e alloc_ref next master\" output,\nand it appears almost everybody knows alloc_ref() wants the\ncaller to count the terminating NUL.\n\nThere however are a few gotchas.\n\n * There is one overallocation in connect.c, which would not\n hurt but is wasteful;\n\n * next:transport.c has alloc_ref(strlen(e->name)) which is a\n no-no;\n\nDiscarding Johannes's patch, the following would fix it.\n\n---\n\n connect.c | 4 ++--\n transport.c | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/connect.c b/connect.c\nindex 06d279e..3d5c4ab 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -72,9 +72,9 @@ struct ref **get_remote_heads(int in, struct ref **list,\n \t\t\tcontinue;\n \t\tif (nr_match && !path_match(name, nr_match, match))\n \t\t\tcontinue;\n-\t\tref = alloc_ref(len - 40);\n+\t\tref = alloc_ref(name_len + 1);\n \t\thashcpy(ref->old_sha1, old_sha1);\n-\t\tmemcpy(ref->name, buffer + 41, len - 40);\n+\t\tmemcpy(ref->name, buffer + 41, name_len + 1);\n \t\t*list = ref;\n \t\tlist = &ref->next;\n \t}\ndiff --git a/transport.c b/transport.c\nindex 4f9cddc..3475cca 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -215,7 +215,7 @@ static struct ref *get_refs_from_bundle(const struct transport *transport)\n \t\tdie (\"Could not read bundle '%s'.\", transport->url);\n \tfor (i = 0; i < data->header.references.nr; i++) {\n \t\tstruct ref_list_entry *e = data->header.references.list + i;\n-\t\tstruct ref *ref = alloc_ref(strlen(e->name));\n+\t\tstruct ref *ref = alloc_ref(strlen(e->name) + 1);\n \t\thashcpy(ref->old_sha1, e->sha1);\n \t\tstrcpy(ref->name, e->name);\n \t\tref->next = result;\n"},{"id":"54256","messageId":"Pine.LNX.4.64.0709281259050.28395@racer.site","threadId":"10054","inReplyTo":"7vhclfqisq.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] alloc_ref(): allow for trailing NUL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-28T12:01:28Z","receivedAt":"2007-09-28T12:01:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 28 Sep 2007, Junio C Hamano wrote:\n\n> Daniel Barkalow writes:\n> \n> > On Fri, 28 Sep 2007, Johannes Schindelin wrote:\n> >\n> >> The parameter name \"namelen\" suggests that you pass the equivalent of\n> >> strlen() to the function alloc_ref(). However, this function did not\n> >> allocate enough space to put a NUL after the name.\n> >> \n> >> Since struct ref does not have any member to describe the length of the\n> >> string, this just does not make sense.\n> >> \n> >> So make space for the NUL.\n> >\n> > Good point, but shouldn't you then fix call sites that use strlen(name) + \n> > 1?\n> \n> Good point.\n> \n> I audited \"git grep -A2 -B4 -e alloc_ref next master\" output,\n> and it appears almost everybody knows alloc_ref() wants the\n> caller to count the terminating NUL.\n> \n> There however are a few gotchas.\n> \n> * There is one overallocation in connect.c, which would not\n> hurt but is wasteful;\n> \n> * next:transport.c has alloc_ref(strlen(e->name)) which is a\n> no-no;\n> \n> Discarding Johannes's patch, the following would fix it.\n\nBut should the signature of alloc_ref() not be changed, then, to read\n\n\tstruct ref *alloc_ref(unsigned name_alloc);\n\nHm?\n\nFurther, I am quite sure that the same mistake will happen again, until we \nchange the function to get the name length, not the number of bytes to \nallocate.\n\nCiao,\nDscho\n"},{"id":"54260","messageId":"20070928124102.GA21309@artemis.corp","threadId":"10054","inReplyTo":"Pine.LNX.4.64.0709281259050.28395@racer.site","subject":"Re: [PATCH] alloc_ref(): allow for trailing NUL","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-28T12:41:02Z","receivedAt":"2007-09-28T12:41:02Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Fri, Sep 28, 2007 at 12:01:28PM +0000, Johannes Schindelin wrote:\n> But should the signature of alloc_ref() not be changed, then, to read\n> \n> \tstruct ref *alloc_ref(unsigned name_alloc);\n> \n> Hm?\n> \n> Further, I am quite sure that the same mistake will happen again, until we \n> change the function to get the name length, not the number of bytes to \n> allocate.\n\n\n\n While being at it, I noticed that the following code pattern is used\nin many places in git:\n\n struct something *ptr = xmalloc(sizeof(struct something) + some_string_len + 1);\n memset(ptr, 0, sizeof(struct something));\n memcpy(ptr->name, somestring, some_string_len + 1);\n\n With name being a flex array.\n\n Maybe we could deal with all of these, and make a generic construction\nthat would take care of allocating the proper space\n\n I believe we could have function doing that, that would factorize this\ncode, and use a nice api à la xmemdupz for this.\n\n It would be something like:\n\n\tvoid *xflexdupz(size_t offset, void *src, size_t len)\n\t{\n\t\tchar *p = xmalloc(offset + len + 1);\n\t\tmemset(p, 0, offset);\n\t\tmemcpy(p + offset, src, len);\n\t\tp[offset + len] = '\\0';\n\t\treturn p;\n\t}\n\n Then alloc_ref could be a wrapper around:\n xflexdupz(offsetof(struct ref, name), ..., ...).\n\n Of course right now alloc_ref doesn't perform any kind of copy, but a\ngrep -A1 will convince you that it's not a problem:\n\n\t$ grep -A1 alloc_ref **/*.[hc]\n\tbuiltin-fetch.c:\t\t\trm = alloc_ref(strlen(ref_name) + 1);\n\tbuiltin-fetch.c-\t\t\tstrcpy(rm->name, ref_name);\n\tbuiltin-fetch.c:\t\t\trm->peer_ref = alloc_ref(strlen(ref_name) + 1);\n\tbuiltin-fetch.c-\t\t\tstrcpy(rm->peer_ref->name, ref_name);\n\t--\n\tconnect.c:\t\tref = alloc_ref(len - 40);\n\tconnect.c-\t\thashcpy(ref->old_sha1, old_sha1);\n\t--\n\tremote.c:struct ref *alloc_ref(unsigned namelen)\n\tremote.c-{\n\t--\n\tremote.c:\t\tref = alloc_ref(20);\n\tremote.c-\t\tstrcpy(ref->name, \"(delete)\");\n\t--\n\tremote.c:\tref = alloc_ref(len);\n\tremote.c-\tmemcpy(ref->name, name, len);\n\t--\n\tremote.c:\tret = alloc_ref(len);\n\tremote.c-\tmemcpy(ret->name, name, len);\n\t--\n\tremote.c:\t\t\tcpy->peer_ref = alloc_ref(local_prefix_len +\n\tremote.c-\t\t\t\t\t\t strlen(match) + 1);\n\t--\n\tremote.c:\t\tret = alloc_ref(strlen(name) + 1);\n\tremote.c-\t\tstrcpy(ret->name, name);\n\t--\n\tremote.c:\t\tret = alloc_ref(strlen(name) + 6);\n\tremote.c-\t\tsprintf(ret->name, \"refs/%s\", name);\n\t--\n\tremote.c:\tret = alloc_ref(strlen(name) + 12);\n\tremote.c-\tsprintf(ret->name, \"refs/heads/%s\", name);\n\t--\n\tremote.h:struct ref *alloc_ref(unsigned namelen);\n\tremote.h-\n\t--\n\ttransport.c:\t\tstruct ref *ref = alloc_ref(strlen(e->name));\n\ttransport.c-\t\thashcpy(ref->old_sha1, e->sha1);\n\n\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"54262","messageId":"20070928131348.GB21309@artemis.corp","threadId":"10054","inReplyTo":"20070928124102.GA21309@artemis.corp","subject":"Re: [PATCH] alloc_ref(): allow for trailing NUL","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2007-09-28T13:13:48Z","receivedAt":"2007-09-28T13:13:48Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Fri, Sep 28, 2007 at 12:41:02PM +0000, Pierre Habouzit wrote:\n> \tvoid *xflexdupz(size_t offset, void *src, size_t len)\n> \t{\n> \t\tchar *p = xmalloc(offset + len + 1);\n> \t\tmemset(p, 0, offset);\n> \t\tmemcpy(p + offset, src, len);\n> \t\tp[offset + len] = '\\0';\n> \t\treturn p;\n> \t}\n> \n> Then alloc_ref could be a wrapper around:\n> xflexdupz(offsetof(struct ref, name), ..., ...).\n> \n> Of course right now alloc_ref doesn't perform any kind of copy, but a\n> grep -A1 will convince you that it's not a problem:\n\n [...]\n> \tremote.c:\t\tret = alloc_ref(strlen(name) + 6);\n> \tremote.c-\t\tsprintf(ret->name, \"refs/%s\", name);\n\n okay forget about me, my proposal doesn't work, too bad :)\n\n\n\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"54276","messageId":"Pine.LNX.4.64.0709281138270.5926@iabervon.org","threadId":"10054","inReplyTo":"Pine.LNX.4.64.0709281259050.28395@racer.site","subject":"Re: [PATCH] alloc_ref(): allow for trailing NUL","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-09-28T15:44:21Z","receivedAt":"2007-09-28T15:44:21Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Fri, 28 Sep 2007, Johannes Schindelin wrote:\n\n> Hi,\n> \n> On Fri, 28 Sep 2007, Junio C Hamano wrote:\n> \n> > Daniel Barkalow writes:\n> > \n> > > On Fri, 28 Sep 2007, Johannes Schindelin wrote:\n> > >\n> > >> The parameter name \"namelen\" suggests that you pass the equivalent of\n> > >> strlen() to the function alloc_ref(). However, this function did not\n> > >> allocate enough space to put a NUL after the name.\n> > >> \n> > >> Since struct ref does not have any member to describe the length of the\n> > >> string, this just does not make sense.\n> > >> \n> > >> So make space for the NUL.\n> > >\n> > > Good point, but shouldn't you then fix call sites that use strlen(name) + \n> > > 1?\n> > \n> > Good point.\n> > \n> > I audited \"git grep -A2 -B4 -e alloc_ref next master\" output,\n> > and it appears almost everybody knows alloc_ref() wants the\n> > caller to count the terminating NUL.\n> > \n> > There however are a few gotchas.\n> > \n> > * There is one overallocation in connect.c, which would not\n> > hurt but is wasteful;\n> > \n> > * next:transport.c has alloc_ref(strlen(e->name)) which is a\n> > no-no;\n> > \n> > Discarding Johannes's patch, the following would fix it.\n> \n> But should the signature of alloc_ref() not be changed, then, to read\n> \n> \tstruct ref *alloc_ref(unsigned name_alloc);\n> \n> Hm?\n> \n> Further, I am quite sure that the same mistake will happen again, until we \n> change the function to get the name length, not the number of bytes to \n> allocate.\n\nI agree. But leaving the majority of cases using the old convention is \njust confusing.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"54277","messageId":"Pine.LNX.4.64.0709281711010.28395@racer.site","threadId":"10054","inReplyTo":"Pine.LNX.4.64.0709281138270.5926@iabervon.org","subject":"Re: [PATCH] alloc_ref(): allow for trailing NUL","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-09-28T16:11:47Z","receivedAt":"2007-09-28T16:11:47Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 28 Sep 2007, Daniel Barkalow wrote:\n\n> On Fri, 28 Sep 2007, Johannes Schindelin wrote:\n> \n> > Hi,\n> > \n> > On Fri, 28 Sep 2007, Junio C Hamano wrote:\n> > \n> > > Daniel Barkalow writes:\n> > > \n> > > > On Fri, 28 Sep 2007, Johannes Schindelin wrote:\n> > > >\n> > > >> The parameter name \"namelen\" suggests that you pass the equivalent of\n> > > >> strlen() to the function alloc_ref(). However, this function did not\n> > > >> allocate enough space to put a NUL after the name.\n> > > >> \n> > > >> Since struct ref does not have any member to describe the length of the\n> > > >> string, this just does not make sense.\n> > > >> \n> > > >> So make space for the NUL.\n> > > >\n> > > > Good point, but shouldn't you then fix call sites that use strlen(name) + \n> > > > 1?\n> > > \n> > > Good point.\n> > > \n> > > I audited \"git grep -A2 -B4 -e alloc_ref next master\" output,\n> > > and it appears almost everybody knows alloc_ref() wants the\n> > > caller to count the terminating NUL.\n> > > \n> > > There however are a few gotchas.\n> > > \n> > > * There is one overallocation in connect.c, which would not\n> > > hurt but is wasteful;\n> > > \n> > > * next:transport.c has alloc_ref(strlen(e->name)) which is a\n> > > no-no;\n> > > \n> > > Discarding Johannes's patch, the following would fix it.\n> > \n> > But should the signature of alloc_ref() not be changed, then, to read\n> > \n> > \tstruct ref *alloc_ref(unsigned name_alloc);\n> > \n> > Hm?\n> > \n> > Further, I am quite sure that the same mistake will happen again, until we \n> > change the function to get the name length, not the number of bytes to \n> > allocate.\n> \n> I agree. But leaving the majority of cases using the old convention is \n> just confusing.\n\nYeah, sorry, that patch was only half-cooked.\n\nIf people agree with me, I'll redo the patch (fixing all calling sites, \ntoo).\n\nCiao,\nDscho\n"},{"id":"54284","messageId":"7v641upsqx.fsf@gitster.siamese.dyndns.org","threadId":"10054","inReplyTo":"Pine.LNX.4.64.0709281711010.28395@racer.site","subject":"Re: [PATCH] alloc_ref(): allow for trailing NUL","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-09-28T18:08:54Z","receivedAt":"2007-09-28T18:08:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Fri, 28 Sep 2007, Daniel Barkalow wrote:\n>\n>> On Fri, 28 Sep 2007, Johannes Schindelin wrote:\n>> \n>> > Further, I am quite sure that the same mistake will happen again, until we \n>> > change the function to get the name length, not the number of bytes to \n>> > allocate.\n>> \n>> I agree. But leaving the majority of cases using the old convention is \n>> just confusing.\n>\n> Yeah, sorry, that patch was only half-cooked.\n>\n> If people agree with me, I'll redo the patch (fixing all calling sites, \n> too).\n\nI think that is probably a better solution.\n"}]}