On Tue, Sep 15, 2015 at 05:55:55PM +0100, Ramsay Jones wrote: > On 15/09/15 16:26, Jeff King wrote: > > The sha1_to_hex and find_unique_abbrev functions always > > write into reusable static buffers. There are a few problems > > with this: > > > > - future calls overwrite our result. This is especially > > annoying with find_unique_abbrev, which does not have a > > ring of buffers, so you cannot even printf() a result > > that has two abbreviated sha1s. > > > > - if you want to put the result into another buffer, we > > often strcpy, which looks suspicious when auditing for > > overflows. > > > > This patch introduces sha1_to_hex_to and find_unique_abbrev_to, > > which write into a user-provided buffer. Of course this is > > just punting on the overflow-auditing, as the buffer > > obviously needs to be GIT_SHA1_HEXSZ + 1 bytes. But it is > > much easier to audit, since that is a well-known size. > > Hmm, I haven't read any other patches yet (including those which use these > new '_to' functions), but I can't help feeling they should be named something > like 'sha1_to_hex_str()' and 'find_unique_abbrev_str()' instead. i.e. I don't get > the '_to' thing - not that I'm any good at naming things ... I meant it as a contrast with their original. sha1_to_hex() formats into an internal buffer and returns it. But sha1_to_hex_to() formats "to" a buffer of your choice. I'm happy to switch the names to something else, but I don't think _str() conveys the difference. If I were starting from scratch, I would probably have just called my variant sha1_to_hex(), and called the original sha1_to_hex_unsafe(). :) -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html