threads / patch / 11533

patch, 2 partssideband.c: Use xmalloc() instead of variable-sized arrays.

Subject: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

## tl;dr

11 messages between Jan 8, 2008 and Jan 9, 2008. Diffs are folded; open one to read it.

replies: 10people: 3as markdown or json

Johannes Sixt· Jan 8, 2008, 16:24 UTC · lore
From: Johannes Sixt <johannes.sixt@telecom.at>

How come we got along with this not very portable construct for so long? Probably because the array sizes were computed from the results of strlen() of string constants. Anyway, a follow-up patch will make the lengths really non-constant.

Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
---
 sideband.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)
Show changes to sideband.c +12 −2
diff --git a/sideband.c b/sideband.c
index 756bbc2..513d7b3 100644
--- a/sideband.c
+++ b/sideband.c
@@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 {
 	unsigned pf = strlen(PREFIX);
 	unsigned sf = strlen(SUFFIX);
-	char buf[pf + LARGE_PACKET_MAX + sf + 1];
+	char *buf, *save;
+
+	save = xmalloc(sf);
+	buf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);
 	memcpy(buf, PREFIX, pf);
 	while (1) {
 		int band, len;
@@ -29,6 +32,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 		if (len < 1) {
 			len = sprintf(buf, "%s: protocol error: no band designator\n", me);
 			safe_write(err, buf, len);
+			free(buf);
+			free(save);
 			return SIDEBAND_PROTOCOL_ERROR;
 		}
 		band = buf[pf] & 0xff;
@@ -38,6 +43,8 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 			buf[pf] = ' ';
 			buf[pf+1+len] = '\n';
 			safe_write(err, buf, pf+1+len+1);
+			free(buf);
+			free(save);
 			return SIDEBAND_REMOTE_ERROR;
 		case 2:
 			buf[pf] = ' ';
@@ -59,7 +66,6 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 				 * line data actually contains something.
 				 */
 				if (brk > pf+1 + 1) {
-					char save[sf];
 					memcpy(save, buf + brk, sf);
 					buf[brk + sf - 1] = buf[brk - 1];
 					memcpy(buf + brk - 1, SUFFIX, sf);
@@ -83,9 +89,13 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 				      "%s: protocol error: bad band #%d\n",
 				      me, band);
 			safe_write(err, buf, len);
+			free(buf);
+			free(save);
 			return SIDEBAND_PROTOCOL_ERROR;
 		}
 	}
+	free(buf);
+	free(save);
 	return 0;
 }

-- 
1.5.4.rc2.815.g2f849-dirty
Nicolas Pitre· Jan 8, 2008, 17:01 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

On Tue, 8 Jan 2008, Johannes Sixt wrote:
Show 5 quoted lines
> From: Johannes Sixt <johannes.sixt@telecom.at>
> 
> How come we got along with this not very portable construct for so long?
> Probably because the array sizes were computed from the results of
> strlen() of string constants.

Maybe because it isn't not so unportable anymore? I doubt that compilers that don't know about automatic arrays would be smart enough to notice the variable was actually a constant due to the strlen() of a constant string and just do like if there wasn't any variable for the array size.

> Anyway, a follow-up patch will make the
> lengths really non-constant.
Fair enough.
Nicolas
Johannes Sixt· Jan 9, 2008, 07:34 UTC · re: Nicolas Pitre · lore

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

Nicolas Pitre schrieb:
Show 10 quoted lines
> On Tue, 8 Jan 2008, Johannes Sixt wrote:
>> How come we got along with this not very portable construct for so long?
>> Probably because the array sizes were computed from the results of
>> strlen() of string constants.
> 
> Maybe because it isn't not so unportable anymore?  I doubt that 
> compilers that don't know about automatic arrays would be smart enough 
> to notice the variable was actually a constant due to the strlen() of a 
> constant string and just do like if there wasn't any variable for the 
> array size.
I just tried it with Visual Age 6, and got this:
    CC sideband.o
"sideband.c", line 22.18: 1506-195 (S) Integral constant expression with a
value greater than zero is required.
"sideband.c", line 62.51: 1506-195 (S) Integral constant expression with a
value greater than zero is required.
make: *** [sideband.o] Error 1

But before I got to this point I had to change all 'static inline' in git-compat-util.h to plain 'static'. So this compiler is out of the game anyway.

Having said that, I'd actually prefer to stay with variable-sized arrays if they prove portable enough because we don't need the handful of free()s on function exits. Junio, if you like I can resend patch 2/2 using variable-sized arrays.

-- Hannes
Junio C Hamano· Jan 9, 2008, 07:53 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

Johannes Sixt <j.sixt@viscovery.net> writes:
> Having said that, I'd actually prefer to stay with variable-sized arrays
> if they prove portable enough because we don't need the handful of free()s
> on function exits. Junio, if you like I can resend patch 2/2 using
> variable-sized arrays.

As an old fashoned git myself, and given the fact that the possible prefix and suffix are small number of short constant strings, I actually prefer a simpler-and-more-stupid approach.

 sideband.c |   18 +++++++++++++-----
 1 files changed, 13 insertions(+), 5 deletions(-)
Show changes to sideband.c +13 −5
diff --git a/sideband.c b/sideband.c
index 756bbc2..24f7f12 100644
--- a/sideband.c
+++ b/sideband.c
@@ -13,14 +13,22 @@
  */
 
 #define PREFIX "remote:"
-#define SUFFIX "\033[K"  /* change to "        " if ANSI sequences don't work */
 
 int recv_sideband(const char *me, int in_stream, int out, int err)
 {
 	unsigned pf = strlen(PREFIX);
-	unsigned sf = strlen(SUFFIX);
-	char buf[pf + LARGE_PACKET_MAX + sf + 1];
+	unsigned sf;
+	char buf[LARGE_PACKET_MAX + 100]; /* for marker slop */
+	char *suffix, *term;
+
 	memcpy(buf, PREFIX, pf);
+	term = getenv("TERM");
+	if (term && strcmp(term, "dumb"))
+		suffix = "\033[K";
+	else
+		suffix = "        ";
+	sf = strlen(suffix);
+
 	while (1) {
 		int band, len;
 		len = packet_read_line(in_stream, buf + pf, LARGE_PACKET_MAX);
@@ -59,10 +67,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 				 * line data actually contains something.
 				 */
 				if (brk > pf+1 + 1) {
-					char save[sf];
+					char save[100]; /* enough slop */
 					memcpy(save, buf + brk, sf);
 					buf[brk + sf - 1] = buf[brk - 1];
-					memcpy(buf + brk - 1, SUFFIX, sf);
+					memcpy(buf + brk - 1, suffix, sf);
 					safe_write(err, buf, brk + sf);
 					memcpy(buf + brk, save, sf);
 				} else
Johannes Sixt· Jan 9, 2008, 08:06 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

Junio C Hamano schrieb:
Show 13 quoted lines
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
>> Having said that, I'd actually prefer to stay with variable-sized arrays
>> if they prove portable enough because we don't need the handful of free()s
>> on function exits. Junio, if you like I can resend patch 2/2 using
>> variable-sized arrays.
> 
> As an old fashoned git myself, and given the fact that the
> possible prefix and suffix are small number of short constant
> strings, I actually prefer a simpler-and-more-stupid approach.
> 
>  sideband.c |   18 +++++++++++++-----
>  1 files changed, 13 insertions(+), 5 deletions(-)
Thanks, your version works well here, too, as a replacement of my two patches.
-- Hannes
Nicolas Pitre· Jan 9, 2008, 19:14 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

On Tue, 8 Jan 2008, Junio C Hamano wrote:
> As an old fashoned git myself, and given the fact that the
> possible prefix and suffix are small number of short constant
> strings, I actually prefer a simpler-and-more-stupid approach.

OK. However I think the following on top of your patch would look cleaner:

Show changes to sideband.c +9 −4
diff --git a/sideband.c b/sideband.c
index 91e462c..b677781 100644
--- a/sideband.c
+++ b/sideband.c
@@ -14,19 +14,24 @@
 
 #define PREFIX "remote:"
 
+#define ANSI_SUFFIX "\033[K"
+#define DUMB_SUFFIX "        "
+
+#define FIX_SIZE 10  /* large enough for any of the above */
+
 int recv_sideband(const char *me, int in_stream, int out, int err)
 {
 	unsigned pf = strlen(PREFIX);
 	unsigned sf;
-	char buf[LARGE_PACKET_MAX + 100]; /* for marker slop */
+	char buf[LARGE_PACKET_MAX + 2*FIX_SIZE];
 	char *suffix, *term;
 
 	memcpy(buf, PREFIX, pf);
 	term = getenv("TERM");
 	if (term && strcmp(term, "dumb"))
-		suffix = "\033[K";
+		suffix = ANSI_SUFFIX;
 	else
-		suffix = "        ";
+		suffix = DUMB_SUFFIX;
 	sf = strlen(suffix);
 
 	while (1) {
@@ -67,7 +72,7 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
 				 * line data actually contains something.
 				 */
 				if (brk > pf+1 + 1) {
-					char save[100]; /* enough slop */
+					char save[FIX_SIZE];
 					memcpy(save, buf + brk, sf);
 					buf[brk + sf - 1] = buf[brk - 1];
 					memcpy(buf + brk - 1, suffix, sf);
Junio C Hamano· Jan 8, 2008, 17:41 UTC · re: Johannes Sixt · lore

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 25 quoted lines
> From: Johannes Sixt <johannes.sixt@telecom.at>
>
> How come we got along with this not very portable construct for so long?
> Probably because the array sizes were computed from the results of
> strlen() of string constants. Anyway, a follow-up patch will make the
> lengths really non-constant.
>
> Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> ---
>  sideband.c |   14 ++++++++++++--
>  1 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/sideband.c b/sideband.c
> index 756bbc2..513d7b3 100644
> --- a/sideband.c
> +++ b/sideband.c
> @@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
>  {
>  	unsigned pf = strlen(PREFIX);
>  	unsigned sf = strlen(SUFFIX);
> -	char buf[pf + LARGE_PACKET_MAX + sf + 1];
> +	char *buf, *save;
> +
> +	save = xmalloc(sf);
> +	buf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);

I have to wonder if the malloc() overhead is small enough compared to the network bandwidth to make a two malloc-free pairs per packet a non-issue...

Nicolas Pitre· Jan 8, 2008, 19:59 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

On Tue, 8 Jan 2008, Junio C Hamano wrote:
Show 31 quoted lines
> Johannes Sixt <j.sixt@viscovery.net> writes:
> 
> > From: Johannes Sixt <johannes.sixt@telecom.at>
> >
> > How come we got along with this not very portable construct for so long?
> > Probably because the array sizes were computed from the results of
> > strlen() of string constants. Anyway, a follow-up patch will make the
> > lengths really non-constant.
> >
> > Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
> > ---
> >  sideband.c |   14 ++++++++++++--
> >  1 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/sideband.c b/sideband.c
> > index 756bbc2..513d7b3 100644
> > --- a/sideband.c
> > +++ b/sideband.c
> > @@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
> >  {
> >  	unsigned pf = strlen(PREFIX);
> >  	unsigned sf = strlen(SUFFIX);
> > -	char buf[pf + LARGE_PACKET_MAX + sf + 1];
> > +	char *buf, *save;
> > +
> > +	save = xmalloc(sf);
> > +	buf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);
> 
> I have to wonder if the malloc() overhead is small enough
> compared to the network bandwidth to make a two malloc-free
> pairs per packet a non-issue...
Eeek.  Overhead might be insignificant, but it still doesn't feel right.

What about using alloca() instead? This is not like if we are doing funky things with the allocated memory anyway.

Nicolas
Junio C Hamano· Jan 8, 2008, 20:13 UTC · re: Nicolas Pitre · lore

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

Nicolas Pitre <nico@cam.org> writes:
Show 38 quoted lines
> On Tue, 8 Jan 2008, Junio C Hamano wrote:
>
>> Johannes Sixt <j.sixt@viscovery.net> writes:
>> 
>> > From: Johannes Sixt <johannes.sixt@telecom.at>
>> >
>> > How come we got along with this not very portable construct for so long?
>> > Probably because the array sizes were computed from the results of
>> > strlen() of string constants. Anyway, a follow-up patch will make the
>> > lengths really non-constant.
>> >
>> > Signed-off-by: Johannes Sixt <johannes.sixt@telecom.at>
>> > ---
>> >  sideband.c |   14 ++++++++++++--
>> >  1 files changed, 12 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/sideband.c b/sideband.c
>> > index 756bbc2..513d7b3 100644
>> > --- a/sideband.c
>> > +++ b/sideband.c
>> > @@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
>> >  {
>> >  	unsigned pf = strlen(PREFIX);
>> >  	unsigned sf = strlen(SUFFIX);
>> > -	char buf[pf + LARGE_PACKET_MAX + sf + 1];
>> > +	char *buf, *save;
>> > +
>> > +	save = xmalloc(sf);
>> > +	buf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);
>> 
>> I have to wonder if the malloc() overhead is small enough
>> compared to the network bandwidth to make a two malloc-free
>> pairs per packet a non-issue...
>
> Eeek.  Overhead might be insignificant, but it still doesn't feel right.
>
> What about using alloca() instead?  This is not like if we are doing 
> funky things with the allocated memory anyway.
That's double Eek as I recall AIX is not dead.

How about using a constant large enough slop? It is not like PREFIX and SUFFIX are different vastly between calls.

Nicolas Pitre· Jan 8, 2008, 20:44 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

On Tue, 8 Jan 2008, Junio C Hamano wrote:
> How about using a constant large enough slop?  It is not like
> PREFIX and SUFFIX are different vastly between calls.

The size of the buffer is certainly bounded since the prefix length is constant, the buffer size is constant, and both possible suffixes are also constants.

Nicolas
Johannes Sixt· Jan 9, 2008, 07:26 UTC · re: Junio C Hamano · lore

Re: [PATCH 1/2] sideband.c: Use xmalloc() instead of variable-sized arrays.

Junio C Hamano schrieb:
Show 14 quoted lines
> Johannes Sixt <j.sixt@viscovery.net> writes:
>> @@ -19,7 +19,10 @@ int recv_sideband(const char *me, int in_stream, int out, int err)
>>  {
>>  	unsigned pf = strlen(PREFIX);
>>  	unsigned sf = strlen(SUFFIX);
>> -	char buf[pf + LARGE_PACKET_MAX + sf + 1];
>> +	char *buf, *save;
>> +
>> +	save = xmalloc(sf);
>> +	buf = xmalloc(pf + LARGE_PACKET_MAX + sf + 1);
> 
> I have to wonder if the malloc() overhead is small enough
> compared to the network bandwidth to make a two malloc-free
> pairs per packet a non-issue...

recv_sideband() is called _once_per_connection_ and not for each packet. Hence, these two mallocs should not concern us.

-- Hannes

← back to recent threads