[RESPOST,BTRFS-PROGS] btrfs_read_dev_super(): uninitialized variable

Commit Message

Goffredo Baroncelli Sept. 11, 2012, 5:31 p.m. UTC

This is a repost because I rebased the change. The first attempt was 
done with the email "[BTRFS-PROGS][BUG][PATCH] Incorrect detection of a 
removed device  [was Re: “Bug”-report: inconsistency kernel <-> tools]" 
dated 08/31/2012.

In the function btrfs_read_dev_super() it is possible to use the 
variable fsid without initialisation.

In btrfs_read_dev_super(), during the scan of the superblock the 
variable fsid is initialised with the value of fsid of the first 
superblock. But if the first superblock contains an incorrect signature 
this initialisation is skipped.

int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 
sb_bytenr)
{
          u8 fsid[BTRFS_FSID_SIZE];
[...]

line 933:

          for (i = 0; i < BTRFS_SUPER_MIRROR_MAX; i++) {
                  bytenr = btrfs_sb_offset(i);
                  ret = pread64(fd, &buf, sizeof(buf), bytenr);
                  if (ret < sizeof(buf))
                          break;

                  if (btrfs_super_bytenr(&buf) != bytenr ||
                      strncmp((char *)(&buf.magic), BTRFS_MAGIC,
                              sizeof(buf.magic)))
                          continue;

                  if (i == 0)
                          memcpy(fsid, buf.fsid, sizeof(fsid));
                  else if (memcmp(fsid, buf.fsid, sizeof(fsid)))
                          continue;

                 [...]
          }

This bug could be triggered by the command "btrfs device delete", which 
zeros the signature of the first superblock.

The enclosed patch corrects the initialisation of the fsid variable; 
moreover if the fsid are different between the superblocks (the original 
one and its backups) the function fails because the device cannot be 
trusted. Finally it is handled the special case when the magic fields is 
zeroed in the *first* superblock. In this case the device is skipped.

Please apply, thank.

You can pull from
	http://cassiopea.homelinux.net/git/btrfs-progs-unstable.git
branch
	btrfs_read_dev_super-bug
	

BR
G.Baroncelli

Patch

diff --git a/disk-io.c b/disk-io.c
index b21a87f..82fc3b8 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -910,6 +910,7 @@  struct btrfs_root *open_ctree_fd(int fp, const char *path, u64 sb_bytenr,
 int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
 {
 	u8 fsid[BTRFS_FSID_SIZE];
+	int fsid_is_initialized = 0;
 	struct btrfs_super_block buf;
 	int i;
 	int ret;
@@ -936,15 +937,26 @@  int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
 		if (ret < sizeof(buf))
 			break;
 
-		if (btrfs_super_bytenr(&buf) != bytenr ||
-		    strncmp((char *)(&buf.magic), BTRFS_MAGIC,
+		if (btrfs_super_bytenr(&buf) != bytenr )
+			continue;
+		/* if magic is NULL, the device was removed */
+		if (buf.magic == 0 && i==0)
+			return -1;
+		if (strncmp((char *)(&buf.magic), BTRFS_MAGIC,
 			    sizeof(buf.magic)))
 			continue;
 
-		if (i == 0)
+		if (!fsid_is_initialized){
 			memcpy(fsid, buf.fsid, sizeof(fsid));
-		else if (memcmp(fsid, buf.fsid, sizeof(fsid)))
-			continue;
+			fsid_is_initialized = 1;
+		} else if (memcmp(fsid, buf.fsid, sizeof(fsid))) {
+			/* 
+			 * the superblocks (the original one and
+			 * its backups) contain data of different
+			 * filesystems -> the disk cannot be trusted
+			 */
+			return -1;
+		}
 
 		if (btrfs_super_generation(&buf) > transid) {
 			memcpy(sb, &buf, sizeof(*sb));