[PATCH] btrfs-progs: don't overrun "answer" array in cmds-chunk.c

[PATCH] btrfs-progs: don't overrun "answer" array in cmds-chunk.c

[Eric Sandeen]

If an array is 5 chars in size:

       char answer[5];

and we write the 6th char (counting from 0)...

       answer[5] = '\0';

we get problems:

cmds-chunk.c: In function 'ask_user.clone.0':
cmds-chunk.c:1343: warning: array subscript is above array bounds

Fix it...

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---


diff --git a/cmds-chunk.c b/cmds-chunk.c
index 03314de..7c3257c 100644
--- a/cmds-chunk.c
+++ b/cmds-chunk.c
@@ -1340,7 +1340,7 @@ again:
 		}
 		i++;
 	}
-	answer[5] = '\0';
+	answer[4] = '\0';
 	__fpurge(stdin);
 
 	if (strlen(answer) == 0) {

Re: [PATCH] btrfs-progs: don't overrun "answer" array in cmds-chunk.c

[Miao Xie]

On 	mon, 05 Aug 2013 21:52:57 -0500, Eric Sandeen wrote:
> If an array is 5 chars in size:
> 
>        char answer[5];
> 
> and we write the 6th char (counting from 0)...
> 
>        answer[5] = '\0';
> 
> we get problems:
> 
> cmds-chunk.c: In function 'ask_user.clone.0':
> cmds-chunk.c:1343: warning: array subscript is above array bounds
> 
> Fix it...
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Thanks to fix this problem.

Reviewed-by: Miao Xie <miaox@cn.fujitsu.com>

> 
> 
> diff --git a/cmds-chunk.c b/cmds-chunk.c
> index 03314de..7c3257c 100644
> --- a/cmds-chunk.c
> +++ b/cmds-chunk.c
> @@ -1340,7 +1340,7 @@ again:
>  		}
>  		i++;
>  	}
> -	answer[5] = '\0';
> +	answer[4] = '\0';
>  	__fpurge(stdin);
>  
>  	if (strlen(answer) == 0) {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH] btrfs-progs: don't overrun "answer" array in cmds-chunk.c

[Zach Brown]

On Mon, Aug 05, 2013 at 09:52:57PM -0500, Eric Sandeen wrote:
> If an array is 5 chars in size:
> 
>        char answer[5];
> 
> and we write the 6th char (counting from 0)...
> 
>        answer[5] = '\0';

*high fives*

> -	answer[5] = '\0';
> +	answer[4] = '\0';

I went to see which way of avoiding another magical raw constant would
be best and did a bit of a double take.

If you're in here, want to reimplement this thing in a few lines of
scanf(%s) and strcasecmp()?  I can give it a go if you don't want to.

- z

Re: [PATCH] btrfs-progs: don't overrun "answer" array in cmds-chunk.c

[Eric Sandeen]

On 8/5/13 10:57 PM, Zach Brown wrote:
> On Mon, Aug 05, 2013 at 09:52:57PM -0500, Eric Sandeen wrote:
>> If an array is 5 chars in size:
>>
>>        char answer[5];
>>
>> and we write the 6th char (counting from 0)...
>>
>>        answer[5] = '\0';
> 
> *high fives*
> 
>> -	answer[5] = '\0';
>> +	answer[4] = '\0';
> 
> I went to see which way of avoiding another magical raw constant would
> be best and did a bit of a double take.
> 
> If you're in here, want to reimplement this thing in a few lines of
> scanf(%s) and strcasecmp()?  I can give it a go if you don't want to.

Zach, I think you'd do that really, really well.  ;)

-Eric

(sorry for just going for the expedient/obvious/1-char fix :) )
 
> - z
> 

Re: [PATCH] btrfs-progs: don't overrun "answer" array in cmds-chunk.c

[Miao Xie]

On Mon, 5 Aug 2013 20:57:57 -0700, Zach Brown wrote:
> On Mon, Aug 05, 2013 at 09:52:57PM -0500, Eric Sandeen wrote:
>> If an array is 5 chars in size:
>>
>>        char answer[5];
>>
>> and we write the 6th char (counting from 0)...
>>
>>        answer[5] = '\0';
> 
> *high fives*
> 
>> -	answer[5] = '\0';
>> +	answer[4] = '\0';
> 
> I went to see which way of avoiding another magical raw constant would
> be best and did a bit of a double take.
> 
> If you're in here, want to reimplement this thing in a few lines of
> scanf(%s) and strcasecmp()?  I can give it a go if you don't want to.
> 

I think it is better that moving it to utils.c because the other commands
may use it in the future.

Thanks
Miao

Re: [PATCH] btrfs-progs: don't overrun "answer" array in cmds-chunk.c

[Zach Brown]

> > If you're in here, want to reimplement this thing in a few lines of
> > scanf(%s) and strcasecmp()?  I can give it a go if you don't want to.
> 
> I think it is better that moving it to utils.c because the other commands
> may use it in the future.

I disagree.  Let's stick to only writing the code that we need.
Implementing and testing code that meets future needs that we make up
isn't a good use of our time.  If someone has to tweak this in the
future, so be it.  They'll actually be in a position to implement and
test their needs.

Anyway, here's how I'd do a trivial y/n prompt.

- z

>From 2c46c2b81061f1c55de07a80d9d177a7df7b33cb Mon Sep 17 00:00:00 2001
From: Zach Brown <zab@redhat.com>
Date: Tue, 6 Aug 2013 11:30:21 -0700
Subject: [PATCH] btrfs-progs: simplify ask_user()

Eric noticed the trivial stack overflow bug in ask_user().  I went to
see the context for that fix and found that ask_user() was a bit much.

This fixes the overflow bug that Eric found, endless spinning on scanf()
errors, removes dead code, and leaves us with a trivial helper.

Signed-off-by: Zach Brown <zab@redhat.com>
---
 cmds-chunk.c | 65 +++++++++++++-----------------------------------------------
 1 file changed, 14 insertions(+), 51 deletions(-)

diff --git a/cmds-chunk.c b/cmds-chunk.c
index 03314de..35a5c69 100644
--- a/cmds-chunk.c
+++ b/cmds-chunk.c
@@ -1307,58 +1307,22 @@ fail_close_fd:
 	return ret;
 }
 
-static int ask_user(char *question, int defval)
+/*
+ * This reads a line from the stdin and only returns non-zero if the
+ * first whitespace delimited token is a case insensitive match with yes
+ * or y.
+ */
+static int ask_user(char *question)
 {
-	char answer[5];
-	char *defstr;
-	int i;
-
-	if (defval == 1)
-		defstr = "[Y/n]";
-	else if (defval == 0)
-		defstr = "[y/N]";
-	else if (defval == -1)
-		defstr = "[y/n]";
-	else
-		BUG_ON(1);
-again:
-	printf("%s%s? ", question, defstr);
-
-	i = 0;
-	while (i < 4 && scanf("%c", &answer[i])) {
-		if (answer[i] == '\n') {
-			answer[i] = '\0';
-			break;
-		} else if (answer[i] == ' '){
-			answer[i] = '\0';
-			if (i == 0)
-				continue;
-			else
-				break;
-		} else if (answer[i] >= 'A' && answer[i] <= 'Z') {
-			answer[i] += 'a' - 'A';
-		}
-		i++;
-	}
-	answer[5] = '\0';
-	__fpurge(stdin);
-
-	if (strlen(answer) == 0) {
-		if (defval != -1)
-			return defval;
-		else
-			goto again;
-	}
+	char buf[30] = {0,};
+	char *saveptr = NULL;
+	char *answer;
 
-	if (!strcmp(answer, "yes") ||
-	    !strcmp(answer, "y"))
-		return 1;
-
-	if (!strcmp(answer, "no") ||
-	    !strcmp(answer, "n"))
-		return 0;
+	printf("%s [y/N]: ", question);
 
-	goto again;
+	return fgets(buf, sizeof(buf) - 1, stdin) &&
+	       (answer = strtok_r(buf, " \t\n\r", &saveptr)) &&
+	       (!strcasecmp(answer, "yes") || !strcasecmp(answer, "y"));
 }
 
 static int btrfs_get_device_extents(u64 chunk_object,
@@ -1752,8 +1716,7 @@ static int btrfs_recover_chunk_tree(char *path, int verbose, int yes)
 	}
 
 	if (!rc.yes) {
-		ret = ask_user("We are going to rebuild the chunk tree on disk, it might destroy the old metadata on the disk, Are you sure",
-			       0);
+		ret = ask_user("We are going to rebuild the chunk tree on disk, it might destroy the old metadata on the disk, Are you sure?");
 		if (!ret) {
 			ret = BTRFS_CHUNK_TREE_REBUILD_ABORTED;
 			goto fail_close_ctree;
-- 
1.7.11.7