[PATCH] fetch--tool: fix uninitialized buffer when reading from stdin

[PATCH] fetch--tool: fix uninitialized buffer when reading from stdin

[Johannes Schindelin]

Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
 builtin-fetch--tool.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index 48de08d..e5bb560 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -14,6 +14,10 @@ static char *get_stdin(void)
 		data = xrealloc(data, offset + CHUNK_SIZE);
 		read = xread(0, data + offset, CHUNK_SIZE);
 	}
+	if (read > 0 && data[read - 1] == '\n')
+		data[read - 1] = '\0';
+	else
+		data[read] = '\0';
 	return data;
 }
 
-- 
1.5.0.1.2424.g39563

Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin

[Linus Torvalds]

On Mon, 26 Feb 2007, Johannes Schindelin wrote:
> 
> Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> ---
>  builtin-fetch--tool.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
> index 48de08d..e5bb560 100644
> --- a/builtin-fetch--tool.c
> +++ b/builtin-fetch--tool.c
> @@ -14,6 +14,10 @@ static char *get_stdin(void)
>  		data = xrealloc(data, offset + CHUNK_SIZE);
>  		read = xread(0, data + offset, CHUNK_SIZE);
>  	}
> +	if (read > 0 && data[read - 1] == '\n')
> +		data[read - 1] = '\0';
> +	else
> +		data[read] = '\0';
>  	return data;
>  }

This is horrible crap.

First off, "read" here may be -1.

Secondly, "data[read]", even if read is positive, is in the *middle* of a 
buffer.

It should probably be something like

	if (read > 0)
		offset += read;

	/* Why do this? Because Dscho did.. */
	if (offset && data[offset-1] == '\n')
		offset--;

	data[offset] = 0;

which at least seems to be potentially sane.

		Linus

Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin

[Johannes Schindelin]

Hi,

On Mon, 26 Feb 2007, Linus Torvalds wrote:

> > +	if (read > 0 && data[read - 1] == '\n')
> > +		data[read - 1] = '\0';
> > +	else
> > +		data[read] = '\0';
> 
> First off, "read" here may be -1.

Ah yes. I somehow had the erroneous impression that xread() die()s on 
error like xmalloc()...

> 	/* Why do this? Because Dscho did.. */
> 	if (offset && data[offset-1] == '\n')
> 		offset--;

Well yes, it probably does not change anything except the output, which 
you see only when debugging.

It's not like I care deeply, because this part of the code will hopefully 
soon go away, when git-fetch is a proper builtin.

However, I compile with -DXMALLOC_POISON=1 and _that_ did not play well, 
whereas the read fails rather rarely on stdin...

But I agree your patch is saner.

Ciao,
Dscho

Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin

[Linus Torvalds]

On Mon, 26 Feb 2007, Johannes Schindelin wrote:
> 
> But I agree your patch is saner.

Well, it's not the "saner" I'm worried about, I was more worried about  
the fact that your patch added random '\0' characters in the middle of the 
stdin input if it was ever bigger than CHUNK_SIZE. I suspect you only 
tested with small input to stdin.

			Linus

Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin

[Johannes Schindelin]

Hi,

On Mon, 26 Feb 2007, Linus Torvalds wrote:

> On Mon, 26 Feb 2007, Johannes Schindelin wrote:
> > 
> > But I agree your patch is saner.
> 
> Well, it's not the "saner" I'm worried about, I was more worried about 
> the fact that your patch added random '\0' characters in the middle of 
> the stdin input if it was ever bigger than CHUNK_SIZE. I suspect you 
> only tested with small input to stdin.

Ooops. I did not even realize that after reading your reply.

Ciao,
Dscho

Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin

[Johannes Schindelin]

Hi,

On Mon, 26 Feb 2007, Linus Torvalds wrote:

> I suspect you only tested with small input to stdin.

Addendum: yes, I only tested with a reflist which was smaller than one 
megabyte ;-)

Ciao,
Dscho

Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin

[Linus Torvalds]

On Mon, 26 Feb 2007, Johannes Schindelin wrote:

> Hi,
> 
> On Mon, 26 Feb 2007, Linus Torvalds wrote:
> 
> > I suspect you only tested with small input to stdin.
> 
> Addendum: yes, I only tested with a reflist which was smaller than one 
> megabyte ;-)

Well, that CHUNK_SIZE is just silly. I don't see why you'd have a 
chunk-size of a megabyte to begin with, IO doesn't really get any more 
efficient that way. And yeah, in this case it would easily hide the bug, 
because in practice nobody would ever test with that much input data.

It might make sense to make the chunk-size smaller just from a testability 
standpoint (not to mention that it's probably currently just wasting 
memory for most users - although at least under Linux, if you never use a 
page, none will be allocated for you, so the OS may hide the wastage).

		Linus

Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Mon, 26 Feb 2007, Johannes Schindelin wrote:
>
>> Hi,
>> 
>> On Mon, 26 Feb 2007, Linus Torvalds wrote:
>> 
>> > I suspect you only tested with small input to stdin.
>> 
>> Addendum: yes, I only tested with a reflist which was smaller than one 
>> megabyte ;-)
>
> Well, that CHUNK_SIZE is just silly. I don't see why you'd have a 
> chunk-size of a megabyte to begin with,...

Me neither (not my code) ;-).  I agree with you that using
smaller limit than what will be deployed often helps
testability.

Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Well, that CHUNK_SIZE is just silly. I don't see why you'd have a 
> chunk-size of a megabyte to begin with, IO doesn't really get any more 
> efficient that way. And yeah, in this case it would easily hide the bug, 
> because in practice nobody would ever test with that much input data.
>
> It might make sense to make the chunk-size smaller just from a testability 
> standpoint (not to mention that it's probably currently just wasting 
> memory for most users - although at least under Linux, if you never use a 
> page, none will be allocated for you, so the OS may hide the wastage).

How about doing this instead?

-- >8 --
[PATCH] fetch--tool: fix uninitialized buffer when reading from stdin

The original code allocates too much space and forgets to NUL
terminate the string.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 builtin-fetch--tool.c |   19 +++++++++++++------
 1 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
index 48de08d..a068f8d 100644
--- a/builtin-fetch--tool.c
+++ b/builtin-fetch--tool.c
@@ -2,17 +2,24 @@
 #include "refs.h"
 #include "commit.h"
 
-#define CHUNK_SIZE (1048576)
+#define CHUNK_SIZE 1024
 
 static char *get_stdin(void)
 {
+	int offset = 0;
 	char *data = xmalloc(CHUNK_SIZE);
-	int offset = 0, read = 0;
-	read = xread(0, data, CHUNK_SIZE);
-	while (read == CHUNK_SIZE) {
-		offset += CHUNK_SIZE;
+
+	while (1) {
+		int cnt = xread(0, data + offset, CHUNK_SIZE);
+		if (cnt < 0)
+			die("error reading standard input: %s",
+			    strerror(errno));
+		if (cnt == 0) {
+			data[offset] = 0;
+			break;
+		}
+		offset += cnt;
 		data = xrealloc(data, offset + CHUNK_SIZE);
-		read = xread(0, data + offset, CHUNK_SIZE);
 	}
 	return data;
 }

Re: [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin

[Johannes Schindelin]

Hi

On Mon, 26 Feb 2007, Junio C Hamano wrote:

> [PATCH] fetch--tool: fix uninitialized buffer when reading from stdin
> 
> The original code allocates too much space and forgets to NUL
> terminate the string.
> 
> Signed-off-by: Junio C Hamano <junkio@cox.net>
> ---
> 
>  builtin-fetch--tool.c |   19 +++++++++++++------
>  1 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/builtin-fetch--tool.c b/builtin-fetch--tool.c
> index 48de08d..a068f8d 100644
> --- a/builtin-fetch--tool.c
> +++ b/builtin-fetch--tool.c
> @@ -2,17 +2,24 @@
>  #include "refs.h"
>  #include "commit.h"
>  
> -#define CHUNK_SIZE (1048576)
> +#define CHUNK_SIZE 1024
>  
>  static char *get_stdin(void)
>  {
> +	int offset = 0;
>  	char *data = xmalloc(CHUNK_SIZE);
> -	int offset = 0, read = 0;
> -	read = xread(0, data, CHUNK_SIZE);
> -	while (read == CHUNK_SIZE) {
> -		offset += CHUNK_SIZE;
> +
> +	while (1) {
> +		int cnt = xread(0, data + offset, CHUNK_SIZE);
> +		if (cnt < 0)
> +			die("error reading standard input: %s",
> +			    strerror(errno));
> +		if (cnt == 0) {
> +			data[offset] = 0;
> +			break;
> +		}
> +		offset += cnt;
>  		data = xrealloc(data, offset + CHUNK_SIZE);
> -		read = xread(0, data + offset, CHUNK_SIZE);
>  	}
>  	return data;
>  }

Me-liked-by: Dscho. And maybe people forget about my silly "fix"...

Ciao,
Dscho