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

PATCH In review Eric Sandeen 3 participants 6 messages Aug 6, 2013 Btrfs file system

lore

Share & tools

lore

Eric Sandeen

author

2013-08-06 02:53 UTC #0 lore raw

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

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 <[email protected]>

---

cmds-chunk.c +1 -1

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

Miao Xie

Reviewed-by on v1 1/1

2013-08-06 03:16 UTC #1 lore raw

Quoted 10 lines · 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:

Quoted 7 lines

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 <[email protected]>
---

Thanks to fix this problem.

Reviewed-by Miao Xie <[email protected]>

Quoted 21 lines

cmds-chunk.c +1 -2
index 03314de..7c3257c 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Zach Brown

2013-08-06 03:58 UTC #2 lore raw

In reply to Eric Sandeen

Quoted 7 lines · 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';

*high fives*

cmds-chunk.c again: · line 1343

Quoted 2 lines

cmds-chunk.c +1 -1

-	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

Eric Sandeen

author

2013-08-06 04:03 UTC #3 lore raw

Quoted 11 lines · Zach Brown

Quoted 7 lines · 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';
*high fives*

Quoted 8 lines

cmds-chunk.c again: · line 1343
Quoted 2 lines
cmds-chunk.c +1 -1
-	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 :) )

Quoted 2 lines

- z

Miao Xie

2013-08-06 05:56 UTC #4 lore raw

In reply to Zach Brown

Quoted 11 lines · Zach Brown

Quoted 7 lines · 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';
*high fives*

Quoted 9 lines

cmds-chunk.c again: · line 1343
Quoted 2 lines
cmds-chunk.c +1 -1
-	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

Zach Brown

2013-08-06 18:49 UTC #5 lore raw

Quoted 5 lines

Quoted 2 lines
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

Quoted 1 line

From 2c46c2b81061f1c55de07a80d9d177a7df7b33cb Mon Sep 17 00:00:00 2001

From: Zach Brown <[email protected]>
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 <[email protected]>

---
 cmds-chunk.c | 65 +++++++++++++-----------------------------------------------
 1 file changed, 14 insertions(+), 51 deletions(-)

cmds-chunk.c +14 ~~-51~~

index 03314de..35a5c69 100644
@@ -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;

Signature · 1 line

1.7.11.7