{"thread":{"id":"8268","subject":"Git string manipulation functions wrong?","startedAt":"2007-05-21T13:11:03Z","lastAt":"2007-05-23T03:22:35Z","messageCount":4,"participants":["Erik Mouw","Petr Baudis","Karl Hasselström","Kyle Moffett"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"42893","messageId":"20070521131103.GN8200@gateway.home","threadId":"8268","inReplyTo":null,"subject":"Git string manipulation functions wrong?","fromName":"Erik Mouw","fromEmail":"mouw@nl.linux.org","sentAt":"2007-05-21T13:11:03Z","receivedAt":"2007-05-21T13:11:03Z","isPatch":false,"sender":{"key":"mouw@nl.linux.org","avatar":null},"body":"Hi,\n\nI got this forwarded from a friend who is subscribed to the Dovecot\nmailing lists (dovecot is a pop3/imap server).\n\n  http://www.dovecot.org/list/dovecot/2007-May/022853.html\n  http://www.dovecot.org/list/dovecot/2007-May/022856.html\n\nThe Dovecot author claims there are \"basic string manipulation errors\"\nin the git code and that's a reason for him not to use git.\n\nI can see his problem with *snprintf() functions in the case where the\namount of output is larger than the buffer size: *snprintf() will\nreturn the number of characters written if there would have been enough\nspace to write them, which will lead to problems with code like \"len +=\nsnprintf(buf, max, bla, ...)\". I don't see his problems with strncpy(),\nthough.\n\n\nErik\n\n-- \nThey're all fools. Don't worry. Darwin may be slow, but he'll\neventually get them. -- Matthew Lammers in alt.sysadmin.recovery\n"},{"id":"42895","messageId":"20070521143616.GG4489@pasky.or.cz","threadId":"8268","inReplyTo":"20070521131103.GN8200@gateway.home","subject":"Re: Git string manipulation functions wrong?","fromName":"Petr Baudis","fromEmail":"pasky@suse.cz","sentAt":"2007-05-21T14:36:16Z","receivedAt":"2007-05-21T14:36:16Z","isPatch":false,"sender":{"key":"pasky@ucw.cz","avatar":"https://avatars.githubusercontent.com/u/18439?v=4"},"body":"On Mon, May 21, 2007 at 03:11:03PM CEST, Erik Mouw wrote:\n> Hi,\n> \n> I got this forwarded from a friend who is subscribed to the Dovecot\n> mailing lists (dovecot is a pop3/imap server).\n> \n>   http://www.dovecot.org/list/dovecot/2007-May/022853.html\n>   http://www.dovecot.org/list/dovecot/2007-May/022856.html\n> \n> The Dovecot author claims there are \"basic string manipulation errors\"\n> in the git code and that's a reason for him not to use git.\n> \n> I can see his problem with *snprintf() functions in the case where the\n> amount of output is larger than the buffer size: *snprintf() will\n> return the number of characters written if there would have been enough\n> space to write them, which will lead to problems with code like \"len +=\n> snprintf(buf, max, bla, ...)\". I don't see his problems with strncpy(),\n> though.\n\nIt's the opposite for me - we don't properly set the NUL byte for smoe\nof our strncpy() calls, but I don't really see his problem with\nsnprintf(), we seem to handle its return value correctly everywhere\n(except diff.c, but there the buffer sizes should be designed in such a\nway that an overflow should be impossible).\n\n-- \n\t\t\t\tPetr \"Pasky the Sleepy\" Baudis\nStuff: http://pasky.or.cz/\nEver try. Ever fail. No matter. // Try again. Fail again. Fail better.\n\t\t-- Samuel Beckett\n"},{"id":"42896","messageId":"20070521145925.GA6474@diana.vm.bytemark.co.uk","threadId":"8268","inReplyTo":"20070521143616.GG4489@pasky.or.cz","subject":"Re: Git string manipulation functions wrong?","fromName":"Karl Hasselström","fromEmail":"kha@treskal.com","sentAt":"2007-05-21T14:59:25Z","receivedAt":"2007-05-21T14:59:25Z","isPatch":false,"sender":{"key":"kha@treskal.com","avatar":"https://gravatar.com/avatar/f0120c734b5279b345075a28521e1ac66acb20c9913ffe9bf6ae97e53f7f3f13?d=mp&s=160"},"body":"On 2007-05-21 16:36:16 +0200, Petr Baudis wrote:\n\n> It's the opposite for me - we don't properly set the NUL byte for\n> smoe of our strncpy() calls, but I don't really see his problem with\n> snprintf(), we seem to handle its return value correctly everywhere\n> (except diff.c, but there the buffer sizes should be designed in\n> such a way that an overflow should be impossible).\n\nI think this kind of detailed case-by-case analysis defeats Timo's\npoint, though: that the C library functions make it too easy to write\nbugs. If it's necessary to do non-trivial bounds checking etc. at\nevery call site, it doesn't really matter if we currently do get them\nall right; at some point, we _are_ going to miss one. Instead of using\nour collective C-fu to get difficult calls right, we should be using\nit to construct string routines that have low enough overhead that\nit's lost in the noise, and are dead simple to use (and, of course,\nthat can be cleanly bypassed in the 1% of cases where it's necessary).\n\n-- \nKarl Hasselström, kha@treskal.com\n      www.treskal.com/kalle\n"},{"id":"43014","messageId":"20070522232235.1cecb880.mrlinuxman@mac.com","threadId":"8268","inReplyTo":"20070521145925.GA6474@diana.vm.bytemark.co.uk","subject":"Re: Git string manipulation functions wrong?","fromName":"Kyle Moffett","fromEmail":"mrlinuxman@mac.com","sentAt":"2007-05-23T03:22:35Z","receivedAt":"2007-05-23T03:22:35Z","isPatch":false,"sender":{"key":"mrlinuxman@mac.com","avatar":null},"body":"On Mon, 21 May 2007 16:59:25 +0200 Karl Hasselström <kha@treskal.com> wrote:\n> On 2007-05-21 16:36:16 +0200, Petr Baudis wrote:\n> > It's the opposite for me - we don't properly set the NUL byte for\n> > smoe of our strncpy() calls, but I don't really see his problem with\n> > snprintf(), we seem to handle its return value correctly everywhere\n> > (except diff.c, but there the buffer sizes should be designed in\n> > such a way that an overflow should be impossible).\n> \n> I think this kind of detailed case-by-case analysis defeats Timo's\n> point, though: that the C library functions make it too easy to write\n> bugs. If it's necessary to do non-trivial bounds checking etc. at\n> every call site, it doesn't really matter if we currently do get them\n> all right; at some point, we _are_ going to miss one. Instead of using\n> our collective C-fu to get difficult calls right, we should be using\n> it to construct string routines that have low enough overhead that\n> it's lost in the noise, and are dead simple to use (and, of course,\n> that can be cleanly bypassed in the 1% of cases where it's necessary).\n\nThat would be mostly true, except for the fact that without snprintf()\nreturning how many bytes _would_ have been written, it's much harder to\nreliably allocate buffers for the result on the first pass.  For\nexample, this is a trivial implementation of an function which returns\na freshly-allocated formatted string:\n\n\tchar *data;\n\tunsigned long len;\n\tlen = snprintf(NULL, 0, some_fmt, arg1, arg2, arg3);\n\tif (!len)\n\t\treturn NULL;\n\tdata = malloc(len+1);\n\tif (!data)\n\t\treturn NULL;\n\tdata[len] = '\\0';\n\tsnprintf(data, len, some_fmt, arg1, arg2, arg3);\n\treturn data;\n\nYou can't do that without a loop if it returns how many bytes were\nactually written (although some braindead platforms do that already).\nHere's a function which handles both use-cases in an optimal way:\n\n\tchar *data = NULL;\n\tunsigned long datalen = 0, len;\n\tdo {\n\t\tlen = snprintf(data, datalen, some_fmt, arg1, arg2, arg3);\n\t\tif (!datalen) {\n\t\t\tdatalen = len ? len : 16;\n\t\t\tdata = malloc(datalen);\n\t\t\tif (!data)\n\t\t\t\treturn NULL;\n\t\t} else if (len >= datalen) {\n\t\t\tvoid *newmem;\n\t\t\tdatalen = (len > datalen)?(len + 1):(datalen +16);\n\t\t\tnewmem = realloc(data, datalen);\n\t\t\tif (!newmem) {\n\t\t\t\tfree(data);\n\t\t\t\treturn NULL\n\t\t\t}\n\t\t}\n\t} while (len >= datalen);\n\tdata[len] = '\\0';\n\treturn data;\n\nHopefully, on a nice modern platform, the first iteration will have len\nequal to the ideal actual required length and so it will hit the first\ncase and carefully allocate exactly enough bytes, then on the second\nloop through it will fill in exactly the required bytes and return\nsuccess.  On one of the abovementioned dain-bramaged systems, this will\nloop until snprintf doesn't use all the space in the buffer,\nincrementing by some fixed value each time (in this implementation,\n16).  It should be obvious that correctly-implemented systems will be\nsignificantly more performant than ones without the useful \"feature\" of\nPOSIX-compliance. :-D\n\nCheers,\nKyle Moffett\n"}]}