{"thread":{"id":"7889","subject":"Re: [PATCH 1/5] Make xstrndup common","startedAt":"2007-04-28T18:53:13Z","lastAt":"2007-04-30T22:21:50Z","messageCount":9,"participants":["Josh Triplett","Junio C Hamano","Daniel Barkalow","Adam Roben","Johannes Schindelin","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"40670","messageId":"46339819.8030007@freedesktop.org","threadId":"7889","inReplyTo":null,"subject":"Re: [PATCH 1/5] Make xstrndup common","fromName":"Josh Triplett","fromEmail":"josh@freedesktop.org","sentAt":"2007-04-28T18:53:13Z","receivedAt":"2007-04-28T18:53:13Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"Daniel Barkalow wrote:\n> It was implemented in commit.c; move it with the other x memory functions.\n[...]\n> +static inline char *xstrndup(const char *str, int len)\n> +{\n> +\tchar *ret = xmalloc(len + 1);\n> +\tmemcpy(ret, str, len);\n> +\tret[len] = '\\0';\n> +\treturn ret;\n> +}\n> +\n\nI don't know if it matters, but this definition of xstrndup, like the version\nin commit.c, doesn't match the definition of strndup.  strndup duplicates a\nstring, copying up to n characters or the length of the string.  This xstrndup\nalways copies n characters, reading past the end of the string if it doesn't\nhave at least n characters.\n\n- Josh Triplett\n\n"},{"id":"40677","messageId":"7vslaki0au.fsf@assigned-by-dhcp.cox.net","threadId":"7889","inReplyTo":"46339819.8030007@freedesktop.org","subject":"Re: [PATCH 1/5] Make xstrndup common","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-28T22:47:21Z","receivedAt":"2007-04-28T22:47:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Triplett <josh@freedesktop.org> writes:\n\n> Daniel Barkalow wrote:\n>> It was implemented in commit.c; move it with the other x memory functions.\n> [...]\n>> +static inline char *xstrndup(const char *str, int len)\n>> +{\n>> +\tchar *ret = xmalloc(len + 1);\n>> +\tmemcpy(ret, str, len);\n>> +\tret[len] = '\\0';\n>> +\treturn ret;\n>> +}\n>> +\n>\n> I don't know if it matters, but this definition of xstrndup, like the version\n> in commit.c, doesn't match the definition of strndup.  strndup duplicates a\n> string, copying up to n characters or the length of the string.  This xstrndup\n> always copies n characters, reading past the end of the string if it doesn't\n> have at least n characters.\n\nVery well caught indeed, thanks.  In the original context in\ncommit.c, this function is always given length computed by\ninspecting the source text by the caller, so the code was\ncorrect, but if we are making it available as a general utility\nroutine, we probably cannot depend on that assumption anymore.\n"},{"id":"40703","messageId":"Pine.LNX.4.64.0704291416370.28708@iabervon.org","threadId":"7889","inReplyTo":"46339819.8030007@freedesktop.org","subject":"Re: [PATCH 1/5] Make xstrndup common","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2007-04-29T18:19:28Z","receivedAt":"2007-04-29T18:19:28Z","isPatch":true,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Sat, 28 Apr 2007, Josh Triplett wrote:\n\n> Daniel Barkalow wrote:\n> > It was implemented in commit.c; move it with the other x memory functions.\n> [...]\n> > +static inline char *xstrndup(const char *str, int len)\n> > +{\n> > +\tchar *ret = xmalloc(len + 1);\n> > +\tmemcpy(ret, str, len);\n> > +\tret[len] = '\\0';\n> > +\treturn ret;\n> > +}\n> > +\n> \n> I don't know if it matters, but this definition of xstrndup, like the version\n> in commit.c, doesn't match the definition of strndup.  strndup duplicates a\n> string, copying up to n characters or the length of the string.  This xstrndup\n> always copies n characters, reading past the end of the string if it doesn't\n> have at least n characters.\n\nGood catch. Replacing the memcpy with strncpy solves this, right? \n(Potentially allocating a bit of extra memory if someone is actually using \nit on too short a string for some reason, of course).\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"40712","messageId":"4635003F.7080408@freedesktop.org","threadId":"7889","inReplyTo":"Pine.LNX.4.64.0704291416370.28708@iabervon.org","subject":"Re: [PATCH 1/5] Make xstrndup common","fromName":"Josh Triplett","fromEmail":"josh@freedesktop.org","sentAt":"2007-04-29T20:29:51Z","receivedAt":"2007-04-29T20:29:51Z","isPatch":true,"sender":{"key":"josh@joshtriplett.org","avatar":"https://avatars.githubusercontent.com/u/162737?v=4"},"body":"Daniel Barkalow wrote:\n> On Sat, 28 Apr 2007, Josh Triplett wrote:\n>> Daniel Barkalow wrote:\n>>> It was implemented in commit.c; move it with the other x memory functions.\n>> [...]\n>>> +static inline char *xstrndup(const char *str, int len)\n>>> +{\n>>> +\tchar *ret = xmalloc(len + 1);\n>>> +\tmemcpy(ret, str, len);\n>>> +\tret[len] = '\\0';\n>>> +\treturn ret;\n>>> +}\n>>> +\n>> I don't know if it matters, but this definition of xstrndup, like the version\n>> in commit.c, doesn't match the definition of strndup.  strndup duplicates a\n>> string, copying up to n characters or the length of the string.  This xstrndup\n>> always copies n characters, reading past the end of the string if it doesn't\n>> have at least n characters.\n> \n> Good catch. Replacing the memcpy with strncpy solves this, right? \n> (Potentially allocating a bit of extra memory if someone is actually using \n> it on too short a string for some reason, of course).\n\nThat would work, but it seems bad to allocate excess memory.  How about just\nusing strlen and setting len to that if shorter, before doing the xmalloc and\nmemcpy?  Yes, that makes two passes over the string, but I don't see any way\naround that.\n\nI just checked the glibc source for strndup, and it does exactly the same\nthing, except that it uses the glibc-specific function strnlen rather than\nusing strlen and figuring out the smaller of the two lengths.  That probably\nincreases efficiency if we have a string longer than, but we can't portably\nuse strnlen, so we'd have to check for it; doesn't seem worth the trouble.\n\n- Josh Triplett\n\n"},{"id":"40714","messageId":"7vy7kadifj.fsf@assigned-by-dhcp.cox.net","threadId":"7889","inReplyTo":"4635003F.7080408@freedesktop.org","subject":"Re: [PATCH 1/5] Make xstrndup common","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-04-29T20:39:12Z","receivedAt":"2007-04-29T20:39:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Josh Triplett <josh@freedesktop.org> writes:\n\n> Daniel Barkalow wrote:\n> ...\n>> Good catch. Replacing the memcpy with strncpy solves this, right? \n>> (Potentially allocating a bit of extra memory if someone is actually using \n>> it on too short a string for some reason, of course).\n>\n> That would work, but it seems bad to allocate excess memory.  How about just\n> using strlen and setting len to that if shorter, before doing the xmalloc and\n> memcpy?  Yes, that makes two passes over the string, but I don't see any way\n> around that.\n\nHand-rolling strnlen() would be needed anyway, because there is\nno guarantee that the incoming string is NUL terminated.  In the\nworst case the string may point at a region of memory filled\nwith non-NUL to the end, which coincides with a page boundary,\nand the next page may be an unmapped one; your strlen() would\nsigbus.\n"},{"id":"40715","messageId":"84F8DDF3-BCF5-4C2A-9C90-38AE139D7178@apple.com","threadId":"7889","inReplyTo":"4635003F.7080408@freedesktop.org","subject":"Re: [PATCH 1/5] Make xstrndup common","fromName":"Adam Roben","fromEmail":"aroben@apple.com","sentAt":"2007-04-29T20:40:19Z","receivedAt":"2007-04-29T20:40:19Z","isPatch":true,"sender":{"key":"aroben@apple.com","avatar":"https://gravatar.com/avatar/9d3697e1de53890adf241331f4b970bdd2b18962b2ff0b8028ebb00e085807f8?d=mp&s=160"},"body":"On Apr 29, 2007, at 1:29 PM, Josh Triplett wrote:\n\n> Daniel Barkalow wrote:\n>> On Sat, 28 Apr 2007, Josh Triplett wrote:\n>>> Daniel Barkalow wrote:\n>>>> It was implemented in commit.c; move it with the other x memory  \n>>>> functions.\n>>> [...]\n>>>> +static inline char *xstrndup(const char *str, int len)\n>>>> +{\n>>>> +\tchar *ret = xmalloc(len + 1);\n>>>> +\tmemcpy(ret, str, len);\n>>>> +\tret[len] = '\\0';\n>>>> +\treturn ret;\n>>>> +}\n>>>> +\n>>> I don't know if it matters, but this definition of xstrndup, like  \n>>> the version\n>>> in commit.c, doesn't match the definition of strndup.  strndup  \n>>> duplicates a\n>>> string, copying up to n characters or the length of the string.   \n>>> This xstrndup\n>>> always copies n characters, reading past the end of the string if  \n>>> it doesn't\n>>> have at least n characters.\n>>\n>> Good catch. Replacing the memcpy with strncpy solves this, right?\n>> (Potentially allocating a bit of extra memory if someone is  \n>> actually using\n>> it on too short a string for some reason, of course).\n>\n> That would work, but it seems bad to allocate excess memory.  How  \n> about just\n> using strlen and setting len to that if shorter, before doing the  \n> xmalloc and\n> memcpy?  Yes, that makes two passes over the string, but I don't  \n> see any way\n> around that.\n\n    An easy way around that is to do the string copy yourself,  \nwalking the string until you either find '\\0' or reach len copied  \ncharacters.\n\n-Adam\n"},{"id":"40744","messageId":"Pine.LNX.4.64.0704301508550.29859@racer.site","threadId":"7889","inReplyTo":"4635003F.7080408@freedesktop.org","subject":"Re: [PATCH 1/5] Make xstrndup common","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-04-30T13:12:50Z","receivedAt":"2007-04-30T13:12:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Sun, 29 Apr 2007, Josh Triplett wrote:\n\n> Daniel Barkalow wrote:\n> > On Sat, 28 Apr 2007, Josh Triplett wrote:\n> >> Daniel Barkalow wrote:\n> >>> It was implemented in commit.c; move it with the other x memory functions.\n> >> [...]\n> >>> +static inline char *xstrndup(const char *str, int len)\n> >>> +{\n> >>> +\tchar *ret = xmalloc(len + 1);\n> >>> +\tmemcpy(ret, str, len);\n> >>> +\tret[len] = '\\0';\n> >>> +\treturn ret;\n> >>> +}\n> >>> +\n> >> I don't know if it matters, but this definition of xstrndup, like the \n> >> version in commit.c, doesn't match the definition of strndup.  \n> >> strndup duplicates a string, copying up to n characters or the length \n> >> of the string.  This xstrndup always copies n characters, reading \n> >> past the end of the string if it doesn't have at least n characters.\n> > \n> > Good catch. Replacing the memcpy with strncpy solves this, right? \n> > (Potentially allocating a bit of extra memory if someone is actually \n> > using it on too short a string for some reason, of course).\n> \n> That would work, but it seems bad to allocate excess memory.  How about \n> just using strlen and setting len to that if shorter, before doing the \n> xmalloc and memcpy?  Yes, that makes two passes over the string, but I \n> don't see any way around that.\n\nUnless I am missing something, I think this should work:\n\nstatic inline char *xstrndup(const char *str, int len)\n{\n\tchar *result = strndup(str, len);\n\tif (result == NULL)\n\t\tdie (\"xstrndup(): out of memory\");\n\treturn result;\n}\n\nHmm?\n\nCiao,\nDscho\n\nP.S.: If you feel real paranoid about it, you might insert\n\n\tif (result == NULL) {\n\t\trelease_pack_memory(len, -1);\n\t\tresult = strndup(str, len);\n\t}\n\nbefore the if (...), but I think that's overkill.\n"},{"id":"40755","messageId":"20070430205050.GC4385@coredump.intra.peff.net","threadId":"7889","inReplyTo":"Pine.LNX.4.64.0704301508550.29859@racer.site","subject":"Re: [PATCH 1/5] Make xstrndup common","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2007-04-30T20:50:50Z","receivedAt":"2007-04-30T20:50:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 30, 2007 at 03:12:50PM +0200, Johannes Schindelin wrote:\n\n> Unless I am missing something, I think this should work:\n> \n> static inline char *xstrndup(const char *str, int len)\n> {\n> \tchar *result = strndup(str, len);\n> \tif (result == NULL)\n> \t\tdie (\"xstrndup(): out of memory\");\n> \treturn result;\n> }\n> \n> Hmm?\n\nI can't speak for the original authors, but I imagine part of the\nimpetus was that strndup is a GNU-ism.\n\n-Peff\n"},{"id":"40761","messageId":"Pine.LNX.4.64.0705010020070.29859@racer.site","threadId":"7889","inReplyTo":"20070430205050.GC4385@coredump.intra.peff.net","subject":"Re: [PATCH 1/5] Make xstrndup common","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-04-30T22:21:50Z","receivedAt":"2007-04-30T22:21:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 30 Apr 2007, Jeff King wrote:\n\n> On Mon, Apr 30, 2007 at 03:12:50PM +0200, Johannes Schindelin wrote:\n> \n> > Unless I am missing something, I think this should work:\n> > \n> > static inline char *xstrndup(const char *str, int len)\n> > {\n> > \tchar *result = strndup(str, len);\n> > \tif (result == NULL)\n> > \t\tdie (\"xstrndup(): out of memory\");\n> > \treturn result;\n> > }\n> > \n> > Hmm?\n> \n> I can't speak for the original authors, but I imagine part of the\n> impetus was that strndup is a GNU-ism.\n\nD'oh! I never thought this was a GNU-ism.\n\nCiao,\nDscho\n"}]}