Fix wrong memory free on check_is_root

From: Satoru Takeuchi <[email protected]>

When "/" is Btrfs, "btrfs property <subcommand> /" regards it as non-root by mistake.

check_is_root() regards @object as a file system root if the following two conditions are satisfied.

a) Both @object and its parent directory are Btrfs object (file system root, subvolume, inode, and device used for Btrfs).
b) fsid of the above mentioned two objects are different.

It doesn't work if @object is "/" because, in this case, fsid of "/" and its parent (it's also "/"), are the same.

* Test environment

Two Btrfs file system (not subvolume) "/" and "/home/sat/mnt".

* How to reproduce

Submit "btrfs prop get" against the above mentioned file systems.

* Test Result

** Actual result (without my patch)

==========================

# btrfs prop get /home/sat/mnt/
ro=false
label=                 # label is displayed because it's a file system root
# btrfs prop get /
ro=false               # label is not displayed even if it's a file system root
==========================
** Expected result (with my patch)

==========================

# ./btrfs-new prop get btrfs-auto-test/
# ./btrfs-new prop get /home/sat/mnt/
ro=false
label=
# ./btrfs-new prop get /             
ro=false
label=foo            # label is displayed
===========================

Signed-off-by Satoru Takeuchi <[email protected]>

Reported-by Naohiro Aota <[email protected]>

---
 cmds-property.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

cmds-property.c +0 -0

index a4bc127..640af99 100644

cmds-property.c +9 -1

+		goto out;
+	}
+	if (!strcmp(rp, "/")) {
+		ret = 0;
+		goto free_rp_out;
+	}
 
 	tmp = malloc(strlen(object) + 5);
 	if (!tmp) {
 		ret = -ENOMEM;
-		goto out;
+		goto free_rp_out;
 	}
 	strcpy(tmp, object);
 	if (tmp[strlen(tmp) - 1] != '/')

@@ -165,6 +176,8 @@ static int check_is_root(const char *object)
 
 free_tmp_out:
 	free(tmp);
+free_rp_out:
+	free(rp);
 out:
 	return ret;
 }

Quoted 59 lines · Satoru Takeuchi

From: Satoru Takeuchi <[email protected]>

When "/" is Btrfs, "btrfs property <subcommand> /" regards it as non-root by mistake.

check_is_root() regards @object as a file system root if the following two conditions are satisfied.

a) Both @object and its parent directory are Btrfs object (file system root, subvolume, inode, and device used for Btrfs).
b) fsid of the above mentioned two objects are different.

It doesn't work if @object is "/" because, in this case, fsid of "/" and its parent (it's also "/"), are the same.

* Test environment

Two Btrfs file system (not subvolume) "/" and "/home/sat/mnt".

* How to reproduce

Submit "btrfs prop get" against the above mentioned file systems.

* Test Result

** Actual result (without my patch)

==========================

# btrfs prop get /home/sat/mnt/
ro=false
label=                 # label is displayed because it's a file system root
# btrfs prop get /
ro=false               # label is not displayed even if it's a file system root
==========================
** Expected result (with my patch)

==========================

# ./btrfs-new prop get btrfs-auto-test/
# ./btrfs-new prop get /home/sat/mnt/
ro=false
label=
# ./btrfs-new prop get /
ro=false
label=foo            # label is displayed
===========================

Signed-off-by Satoru Takeuchi <[email protected]>

Reported-by Naohiro Aota <[email protected]>

---
  cmds-property.c | 15 ++++++++++++++-
  1 file changed, 14 insertions(+), 1 deletion(-)

cmds-property.c +0 -0

index a4bc127..640af99 100644

cmds-property.c static int check_is_root(const char *object) · line 132

Quoted 8 lines

cmds-property.c +5 -0

@@ -125,11 +125,22 @@ static int check_is_root(const char *object)
  	u8 fsid[BTRFS_FSID_SIZE];
  	u8 fsid2[BTRFS_FSID_SIZE];
  	char *tmp;
+	char *rp;
+
+	rp = realpath(object, NULL);
+	if (!rp) {
+		ret = -ENOMEM;

Hi Satoru，

IMO, ret = -errno may be better, because there are many possible return values of realpath().

Thanks,
Gui

Quoted 16 lines

cmds-property.c +7 -1

+		goto out;
+	}
+	if (!strcmp(rp, "/")) {
+		ret = 0;
+		goto free_rp_out;
+	}
 
  	tmp = malloc(strlen(object) + 5);
  	if (!tmp) {
  		ret = -ENOMEM;
-		goto out;
+		goto free_rp_out;
  	}
  	strcpy(tmp, object);
  	if (tmp[strlen(tmp) - 1] != '/')

cmds-property.c static int check_is_root(const char *object) · line 183

Quoted 8 lines

cmds-property.c +2 -0

@@ -165,6 +176,8 @@ static int check_is_root(const char *object)
 
  free_tmp_out:
  	free(tmp);
+free_rp_out:
+	free(rp);
  out:
  	return ret;
  }

--
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

cmds-property.c +7 -6

 	if (ret < 0) {
 		fprintf(stderr, "ERROR: get_fsid for %s failed. %s\n", object,
 				strerror(-ret));
-		goto out;
+		goto free_tmp_out;
 	}
 
 	ret = get_fsid(tmp, fsid2, 1);
 	if (ret == -ENOTTY) {
 		ret = 0;
-		goto out;
+		goto free_tmp_out;
 	} else if (ret == -ENOTDIR) {
 		ret = 1;
-		goto out;
+		goto free_tmp_out;
 	} else if (ret < 0) {
 		fprintf(stderr, "ERROR: get_fsid for %s failed. %s\n", tmp,
 			strerror(-ret));
-		goto out;
+		goto free_tmp_out;
 	}
 
 	if (memcmp(fsid, fsid2, BTRFS_FSID_SIZE)) {
 		ret = 0;
-		goto out;
+		goto free_tmp_out;
 	}
 
 	ret = 1;
 
-out:
+free_tmp_out:
 	free(tmp);
+out:
 	return ret;
 }
 

cmds-property.c static int check_is_root(const char *object)

Quoted 16 lines · Satoru Takeuchi

From: Satoru Takeuchi <[email protected]>
Date: Thu, 18 Dec 2014 14:35:22 +0900

@tmp is freed even if its allocation fails.

Signed-off-by Satoru Takeuchi <[email protected]>

---
  cmds-property.c | 13 +++++++------
  1 file changed, 7 insertions(+), 6 deletions(-)

cmds-property.c +0 -0

index a764293..a4bc127 100644
@@ -140,31 +140,32 @@ static int check_is_root(const char *object)

Hi Satoru,

	" tmp = malloc()
	  if (!tmp) {
		...
		goto out;
	  }
	"

Actually, if the malloc() fails, it returns a NULL, and the free() handles the NULL internally.
I think there is no special need to deal with this case by the prog itself.

Thanks,
Gui

Quoted 28 lines

cmds-property.c +5 -5

  	if (ret < 0) {
  		fprintf(stderr, "ERROR: get_fsid for %s failed. %s\n", object,
  				strerror(-ret));
-		goto out;
+		goto free_tmp_out;
  	}
 
  	ret = get_fsid(tmp, fsid2, 1);
  	if (ret == -ENOTTY) {
  		ret = 0;
-		goto out;
+		goto free_tmp_out;
  	} else if (ret == -ENOTDIR) {
  		ret = 1;
-		goto out;
+		goto free_tmp_out;
  	} else if (ret < 0) {
  		fprintf(stderr, "ERROR: get_fsid for %s failed. %s\n", tmp,
  			strerror(-ret));
-		goto out;
+		goto free_tmp_out;
  	}
 
  	if (memcmp(fsid, fsid2, BTRFS_FSID_SIZE)) {
  		ret = 0;
-		goto out;
+		goto free_tmp_out;
  	}

cmds-property.c static int check_is_root(const char *object) · line 171

Quoted 10 lines

cmds-property.c +2 -1

 
  	ret = 1;
 
-out:
+free_tmp_out:
  	free(tmp);
+out:
  	return ret;
  }
 

--
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