{"thread":{"id":"11533","subject":"[PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.","startedAt":"2008-01-08T16:24:18Z","lastAt":"2008-01-09T19:14:55Z","messageCount":11,"participants":["Johannes Sixt","Nicolas Pitre","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"64748","messageId":"4783A3B2.3060801@viscovery.net","threadId":"11533","inReplyTo":null,"subject":"[PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-08T16:24:18Z","receivedAt":"2008-01-08T16:24:18Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"From: Johannes Sixt \n\nHow come we got along with this not very portable construct for so long?\nProbably because the array sizes were computed from the results of\nstrlen() of string constants. Anyway, a follow-up patch will make the\nlengths really non-constant.\n\nSigned-off-by: Johannes Sixt \n---\n sideband.c | 14 ++++++++++++--\n 1 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/sideband.c b/sideband.c\nindex 756bbc2..513d7b3 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)\n {\n \tunsigned pf = strlen(PREFIX);\n \tunsigned sf = strlen(SUFFIX);\n-\tchar buf[pf + LARGE_PACKET_MAX + sf + 1];\n+\tchar *buf, *save;\n+\n+\tsave = xmalloc(sf);\n+\tbuf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);\n \tmemcpy(buf, PREFIX, pf);\n \twhile (1) {\n \t\tint band, len;\n@@ -29,6 +32,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)\n \t\tif (len < 1) {\n \t\t\tlen = sprintf(buf, \"%s: protocol error: no band designator\\n\", me);\n \t\t\tsafe_write(err, buf, len);\n+\t\t\tfree(buf);\n+\t\t\tfree(save);\n \t\t\treturn SIDEBAND_PROTOCOL_ERROR;\n \t\t}\n \t\tband = buf[pf] & 0xff;\n@@ -38,6 +43,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)\n \t\t\tbuf[pf] = ' ';\n \t\t\tbuf[pf+1+len] = '\\n';\n \t\t\tsafe_write(err, buf, pf+1+len+1);\n+\t\t\tfree(buf);\n+\t\t\tfree(save);\n \t\t\treturn SIDEBAND_REMOTE_ERROR;\n \t\tcase 2:\n \t\t\tbuf[pf] = ' ';\n@@ -59,7 +66,6 @@ int recv_sideband(const char *me, int in_stream, int out, int err)\n \t\t\t\t * line data actually contains something.\n \t\t\t\t */\n \t\t\t\tif (brk > pf+1 + 1) {\n-\t\t\t\t\tchar save[sf];\n \t\t\t\t\tmemcpy(save, buf + brk, sf);\n \t\t\t\t\tbuf[brk + sf - 1] = buf[brk - 1];\n \t\t\t\t\tmemcpy(buf + brk - 1, SUFFIX, sf);\n@@ -83,9 +89,13 @@ int recv_sideband(const char *me, int in_stream, int out, int err)\n \t\t\t\t \"%s: protocol error: bad band #%d\\n\",\n \t\t\t\t me, band);\n \t\t\tsafe_write(err, buf, len);\n+\t\t\tfree(buf);\n+\t\t\tfree(save);\n \t\t\treturn SIDEBAND_PROTOCOL_ERROR;\n \t\t}\n \t}\n+\tfree(buf);\n+\tfree(save);\n \treturn 0;\n }\n\n-- \n1.5.4.rc2.815.g2f849-dirty\n"},{"id":"64752","messageId":"alpine.LFD.1.00.0801081154460.3054@xanadu.home","threadId":"11533","inReplyTo":"4783A3B2.3060801@viscovery.net","subject":"Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-01-08T17:01:07Z","receivedAt":"2008-01-08T17:01:07Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 8 Jan 2008, Johannes Sixt wrote:\n\n> From: Johannes Sixt \n> \n> How come we got along with this not very portable construct for so long?\n> Probably because the array sizes were computed from the results of\n> strlen() of string constants.\n\nMaybe because it isn't not so unportable anymore? I doubt that \ncompilers that don't know about automatic arrays would be smart enough \nto notice the variable was actually a constant due to the strlen() of a \nconstant string and just do like if there wasn't any variable for the \narray size.\n\n> Anyway, a follow-up patch will make the\n> lengths really non-constant.\n\nFair enough.\n\n\nNicolas\n"},{"id":"64758","messageId":"7v4pdotdtl.fsf@gitster.siamese.dyndns.org","threadId":"11533","inReplyTo":"4783A3B2.3060801@viscovery.net","subject":"Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-08T17:41:26Z","receivedAt":"2008-01-08T17:41:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> From: Johannes Sixt \n>\n> How come we got along with this not very portable construct for so long?\n> Probably because the array sizes were computed from the results of\n> strlen() of string constants. Anyway, a follow-up patch will make the\n> lengths really non-constant.\n>\n> Signed-off-by: Johannes Sixt \n> ---\n> sideband.c | 14 ++++++++++++--\n> 1 files changed, 12 insertions(+), 2 deletions(-)\n>\n> diff --git a/sideband.c b/sideband.c\n> index 756bbc2..513d7b3 100644\n> --- a/sideband.c\n> +++ b/sideband.c\n> @@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)\n> {\n> \tunsigned pf = strlen(PREFIX);\n> \tunsigned sf = strlen(SUFFIX);\n> -\tchar buf[pf + LARGE_PACKET_MAX + sf + 1];\n> +\tchar *buf, *save;\n> +\n> +\tsave = xmalloc(sf);\n> +\tbuf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);\n\nI have to wonder if the malloc() overhead is small enough\ncompared to the network bandwidth to make a two malloc-free\npairs per packet a non-issue...\n"},{"id":"64767","messageId":"alpine.LFD.1.00.0801081444360.3054@xanadu.home","threadId":"11533","inReplyTo":"7v4pdotdtl.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-01-08T19:59:20Z","receivedAt":"2008-01-08T19:59:20Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 8 Jan 2008, Junio C Hamano wrote:\n\n> Johannes Sixt writes:\n> \n> > From: Johannes Sixt \n> >\n> > How come we got along with this not very portable construct for so long?\n> > Probably because the array sizes were computed from the results of\n> > strlen() of string constants. Anyway, a follow-up patch will make the\n> > lengths really non-constant.\n> >\n> > Signed-off-by: Johannes Sixt \n> > ---\n> > sideband.c | 14 ++++++++++++--\n> > 1 files changed, 12 insertions(+), 2 deletions(-)\n> >\n> > diff --git a/sideband.c b/sideband.c\n> > index 756bbc2..513d7b3 100644\n> > --- a/sideband.c\n> > +++ b/sideband.c\n> > @@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)\n> > {\n> > \tunsigned pf = strlen(PREFIX);\n> > \tunsigned sf = strlen(SUFFIX);\n> > -\tchar buf[pf + LARGE_PACKET_MAX + sf + 1];\n> > +\tchar *buf, *save;\n> > +\n> > +\tsave = xmalloc(sf);\n> > +\tbuf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);\n> \n> I have to wonder if the malloc() overhead is small enough\n> compared to the network bandwidth to make a two malloc-free\n> pairs per packet a non-issue...\n\nEeek. Overhead might be insignificant, but it still doesn't feel right.\n\nWhat about using alloca() instead? This is not like if we are doing \nfunky things with the allocated memory anyway.\n\n\nNicolas\n"},{"id":"64771","messageId":"7v7iikrs86.fsf@gitster.siamese.dyndns.org","threadId":"11533","inReplyTo":"alpine.LFD.1.00.0801081444360.3054@xanadu.home","subject":"Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-08T20:13:13Z","receivedAt":"2008-01-08T20:13:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> On Tue, 8 Jan 2008, Junio C Hamano wrote:\n>\n>> Johannes Sixt writes:\n>> \n>> > From: Johannes Sixt \n>> >\n>> > How come we got along with this not very portable construct for so long?\n>> > Probably because the array sizes were computed from the results of\n>> > strlen() of string constants. Anyway, a follow-up patch will make the\n>> > lengths really non-constant.\n>> >\n>> > Signed-off-by: Johannes Sixt \n>> > ---\n>> > sideband.c | 14 ++++++++++++--\n>> > 1 files changed, 12 insertions(+), 2 deletions(-)\n>> >\n>> > diff --git a/sideband.c b/sideband.c\n>> > index 756bbc2..513d7b3 100644\n>> > --- a/sideband.c\n>> > +++ b/sideband.c\n>> > @@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)\n>> > {\n>> > \tunsigned pf = strlen(PREFIX);\n>> > \tunsigned sf = strlen(SUFFIX);\n>> > -\tchar buf[pf + LARGE_PACKET_MAX + sf + 1];\n>> > +\tchar *buf, *save;\n>> > +\n>> > +\tsave = xmalloc(sf);\n>> > +\tbuf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);\n>> \n>> I have to wonder if the malloc() overhead is small enough\n>> compared to the network bandwidth to make a two malloc-free\n>> pairs per packet a non-issue...\n>\n> Eeek. Overhead might be insignificant, but it still doesn't feel right.\n>\n> What about using alloca() instead? This is not like if we are doing \n> funky things with the allocated memory anyway.\n\nThat's double Eek as I recall AIX is not dead.\n\nHow about using a constant large enough slop? It is not like\nPREFIX and SUFFIX are different vastly between calls.\n"},{"id":"64779","messageId":"alpine.LFD.1.00.0801081526230.3054@xanadu.home","threadId":"11533","inReplyTo":"7v7iikrs86.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-01-08T20:44:49Z","receivedAt":"2008-01-08T20:44:49Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 8 Jan 2008, Junio C Hamano wrote:\n\n> How about using a constant large enough slop? It is not like\n> PREFIX and SUFFIX are different vastly between calls.\n\nThe size of the buffer is certainly bounded since the prefix length is \nconstant, the buffer size is constant, and both possible suffixes are \nalso constants.\n\n\nNicolas\n"},{"id":"64818","messageId":"4784772A.1060704@viscovery.net","threadId":"11533","inReplyTo":"7v4pdotdtl.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-09T07:26:34Z","receivedAt":"2008-01-09T07:26:34Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Sixt writes:\n>> @@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)\n>> {\n>> \tunsigned pf = strlen(PREFIX);\n>> \tunsigned sf = strlen(SUFFIX);\n>> -\tchar buf[pf + LARGE_PACKET_MAX + sf + 1];\n>> +\tchar *buf, *save;\n>> +\n>> +\tsave = xmalloc(sf);\n>> +\tbuf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);\n> \n> I have to wonder if the malloc() overhead is small enough\n> compared to the network bandwidth to make a two malloc-free\n> pairs per packet a non-issue...\n\nrecv_sideband() is called _once_per_connection_ and not for each packet.\nHence, these two mallocs should not concern us.\n\n-- Hannes\n"},{"id":"64819","messageId":"4784791F.6090904@viscovery.net","threadId":"11533","inReplyTo":"alpine.LFD.1.00.0801081154460.3054@xanadu.home","subject":"Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-09T07:34:55Z","receivedAt":"2008-01-09T07:34:55Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Nicolas Pitre schrieb:\n> On Tue, 8 Jan 2008, Johannes Sixt wrote:\n>> How come we got along with this not very portable construct for so long?\n>> Probably because the array sizes were computed from the results of\n>> strlen() of string constants.\n> \n> Maybe because it isn't not so unportable anymore? I doubt that \n> compilers that don't know about automatic arrays would be smart enough \n> to notice the variable was actually a constant due to the strlen() of a \n> constant string and just do like if there wasn't any variable for the \n> array size.\n\nI just tried it with Visual Age 6, and got this:\n\n CC sideband.o\n\"sideband.c\", line 22.18: 1506-195 (S) Integral constant expression with a\nvalue greater than zero is required.\n\"sideband.c\", line 62.51: 1506-195 (S) Integral constant expression with a\nvalue greater than zero is required.\nmake: *** [sideband.o] Error 1\n\nBut before I got to this point I had to change all 'static inline' in\ngit-compat-util.h to plain 'static'. So this compiler is out of the game\nanyway.\n\nHaving said that, I'd actually prefer to stay with variable-sized arrays\nif they prove portable enough because we don't need the handful of free()s\non function exits. Junio, if you like I can resend patch 2/2 using\nvariable-sized arrays.\n\n-- Hannes\n"},{"id":"64820","messageId":"7vk5mjmo4f.fsf@gitster.siamese.dyndns.org","threadId":"11533","inReplyTo":"4784791F.6090904@viscovery.net","subject":"Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-09T07:53:04Z","receivedAt":"2008-01-09T07:53:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Having said that, I'd actually prefer to stay with variable-sized arrays\n> if they prove portable enough because we don't need the handful of free()s\n> on function exits. Junio, if you like I can resend patch 2/2 using\n> variable-sized arrays.\n\nAs an old fashoned git myself, and given the fact that the\npossible prefix and suffix are small number of short constant\nstrings, I actually prefer a simpler-and-more-stupid approach.\n\n sideband.c | 18 +++++++++++++-----\n 1 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/sideband.c b/sideband.c\nindex 756bbc2..24f7f12 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -13,14 +13,22 @@\n */\n \n #define PREFIX \"remote:\"\n-#define SUFFIX \"\\033[K\" /* change to \" \" if ANSI sequences don't work */\n \n int recv_sideband(const char *me, int in_stream, int out, int err)\n {\n \tunsigned pf = strlen(PREFIX);\n-\tunsigned sf = strlen(SUFFIX);\n-\tchar buf[pf + LARGE_PACKET_MAX + sf + 1];\n+\tunsigned sf;\n+\tchar buf[LARGE_PACKET_MAX + 100]; /* for marker slop */\n+\tchar *suffix, *term;\n+\n \tmemcpy(buf, PREFIX, pf);\n+\tterm = getenv(\"TERM\");\n+\tif (term && strcmp(term, \"dumb\"))\n+\t\tsuffix = \"\\033[K\";\n+\telse\n+\t\tsuffix = \" \";\n+\tsf = strlen(suffix);\n+\n \twhile (1) {\n \t\tint band, len;\n \t\tlen = packet_read_line(in_stream, buf + pf, LARGE_PACKET_MAX);\n@@ -59,10 +67,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)\n \t\t\t\t * line data actually contains something.\n \t\t\t\t */\n \t\t\t\tif (brk > pf+1 + 1) {\n-\t\t\t\t\tchar save[sf];\n+\t\t\t\t\tchar save[100]; /* enough slop */\n \t\t\t\t\tmemcpy(save, buf + brk, sf);\n \t\t\t\t\tbuf[brk + sf - 1] = buf[brk - 1];\n-\t\t\t\t\tmemcpy(buf + brk - 1, SUFFIX, sf);\n+\t\t\t\t\tmemcpy(buf + brk - 1, suffix, sf);\n \t\t\t\t\tsafe_write(err, buf, brk + sf);\n \t\t\t\t\tmemcpy(buf + brk, save, sf);\n \t\t\t\t} else\n"},{"id":"64822","messageId":"478480A0.5030907@viscovery.net","threadId":"11533","inReplyTo":"7vk5mjmo4f.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2008-01-09T08:06:56Z","receivedAt":"2008-01-09T08:06:56Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Sixt writes:\n> \n>> Having said that, I'd actually prefer to stay with variable-sized arrays\n>> if they prove portable enough because we don't need the handful of free()s\n>> on function exits. Junio, if you like I can resend patch 2/2 using\n>> variable-sized arrays.\n> \n> As an old fashoned git myself, and given the fact that the\n> possible prefix and suffix are small number of short constant\n> strings, I actually prefer a simpler-and-more-stupid approach.\n> \n> sideband.c | 18 +++++++++++++-----\n> 1 files changed, 13 insertions(+), 5 deletions(-)\n\nThanks, your version works well here, too, as a replacement of my two patches.\n\n-- Hannes\n"},{"id":"64863","messageId":"alpine.LFD.1.00.0801091412000.3054@xanadu.home","threadId":"11533","inReplyTo":"7vk5mjmo4f.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2008-01-09T19:14:55Z","receivedAt":"2008-01-09T19:14:55Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 8 Jan 2008, Junio C Hamano wrote:\n\n> As an old fashoned git myself, and given the fact that the\n> possible prefix and suffix are small number of short constant\n> strings, I actually prefer a simpler-and-more-stupid approach.\n\nOK. However I think the following on top of your patch would look \ncleaner:\n\ndiff --git a/sideband.c b/sideband.c\nindex 91e462c..b677781 100644\n--- a/sideband.c\n+++ b/sideband.c\n@@ -14,19 +14,24 @@\n \n #define PREFIX \"remote:\"\n \n+#define ANSI_SUFFIX \"\\033[K\"\n+#define DUMB_SUFFIX \" \"\n+\n+#define FIX_SIZE 10 /* large enough for any of the above */\n+\n int recv_sideband(const char *me, int in_stream, int out, int err)\n {\n \tunsigned pf = strlen(PREFIX);\n \tunsigned sf;\n-\tchar buf[LARGE_PACKET_MAX + 100]; /* for marker slop */\n+\tchar buf[LARGE_PACKET_MAX + 2*FIX_SIZE];\n \tchar *suffix, *term;\n \n \tmemcpy(buf, PREFIX, pf);\n \tterm = getenv(\"TERM\");\n \tif (term && strcmp(term, \"dumb\"))\n-\t\tsuffix = \"\\033[K\";\n+\t\tsuffix = ANSI_SUFFIX;\n \telse\n-\t\tsuffix = \" \";\n+\t\tsuffix = DUMB_SUFFIX;\n \tsf = strlen(suffix);\n \n \twhile (1) {\n@@ -67,7 +72,7 @@ int recv_sideband(const char *me, int in_stream, int out, int err)\n \t\t\t\t * line data actually contains something.\n \t\t\t\t */\n \t\t\t\tif (brk > pf+1 + 1) {\n-\t\t\t\t\tchar save[100]; /* enough slop */\n+\t\t\t\t\tchar save[FIX_SIZE];\n \t\t\t\t\tmemcpy(save, buf + brk, sf);\n \t\t\t\t\tbuf[brk + sf - 1] = buf[brk - 1];\n \t\t\t\t\tmemcpy(buf + brk - 1, suffix, sf);\n"}]}