[PATCH 1/2] Btrfs-progs: set string end sing '\0' for property

[PATCH 1/2] Btrfs-progs: set string end sing '\0' for property

[Liu Bo]

Set string "xattr_name" 's end with '\0' so that it won't be
violated in memory.

With this fix, xfstest/btrfs/048 can pass on my box.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 props.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/props.c b/props.c
index 4d0aeea..53223a3 100644
--- a/props.c
+++ b/props.c
@@ -135,6 +135,7 @@ static int prop_compression(enum prop_object_type type,
 	}
 	memcpy(xattr_name, XATTR_BTRFS_PREFIX, XATTR_BTRFS_PREFIX_LEN);
 	memcpy(xattr_name + XATTR_BTRFS_PREFIX_LEN, name, strlen(name));
+	xattr_name[XATTR_BTRFS_PREFIX_LEN + strlen(name)] = '\0';
 
 	if (value)
 		sret = fsetxattr(fd, xattr_name, value, strlen(value), 0);
-- 
1.8.2.1

[PATCH 2/2] Btrfs-progs: open file with O_RDONLY on getting property

[Liu Bo]

We don't need change an object when getting its property,
so O_RDWR is not necessary in this case.  Moreover, the object
may be readonly, with O_RDWR it will fail.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 props.c |  5 ++++-
 utils.c | 14 ++++++++++++--
 utils.h |  1 +
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/props.c b/props.c
index 53223a3..a49f8e0 100644
--- a/props.c
+++ b/props.c
@@ -120,7 +120,10 @@ static int prop_compression(enum prop_object_type type,
 	char *buf = NULL;
 	char *xattr_name = NULL;
 
-	fd = open_file_or_dir(object, &dirstream);
+	if (!value)
+		fd = open_file_or_dir_ro(object, &dirstream);
+	else
+		fd = open_file_or_dir(object, &dirstream);
 	if (fd == -1) {
 		ret = -errno;
 		fprintf(stderr, "ERROR: open %s failed. %s\n",
diff --git a/utils.c b/utils.c
index 3e9c527..110a0d1 100644
--- a/utils.c
+++ b/utils.c
@@ -1622,7 +1622,7 @@ u64 parse_size(char *s)
 	return strtoull(s, NULL, 10) * mult;
 }
 
-int open_file_or_dir(const char *fname, DIR **dirstream)
+static int __open_file_or_dir(const char *fname, DIR **dirstream, int ro)
 {
 	int ret;
 	struct stat st;
@@ -1638,7 +1638,7 @@ int open_file_or_dir(const char *fname, DIR **dirstream)
 			return -1;
 		fd = dirfd(*dirstream);
 	} else if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
-		fd = open(fname, O_RDWR);
+		fd = open(fname, (!!ro) ? O_RDONLY : O_RDWR);
 	} else {
 		/*
 		 * we set this on purpose, in case the caller output
@@ -1655,6 +1655,16 @@ int open_file_or_dir(const char *fname, DIR **dirstream)
 	return fd;
 }
 
+int open_file_or_dir(const char *fname, DIR **dirstream)
+{
+	return __open_file_or_dir(fname, dirstream, 0);
+}
+
+int open_file_or_dir_ro(const char *fname, DIR **dirstream)
+{
+	return __open_file_or_dir(fname, dirstream, 1);
+}
+
 void close_file_or_dir(int fd, DIR *dirstream)
 {
 	if (dirstream)
diff --git a/utils.h b/utils.h
index 3c62066..5489cc2 100644
--- a/utils.h
+++ b/utils.h
@@ -72,6 +72,7 @@ int btrfs_scan_block_devices(int run_ioctl);
 u64 parse_size(char *s);
 u64 arg_strtou64(const char *str);
 int open_file_or_dir(const char *fname, DIR **dirstream);
+int open_file_or_dir_ro(const char *fname, DIR **dirstream);
 void close_file_or_dir(int fd, DIR *dirstream);
 int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 		struct btrfs_ioctl_dev_info_args **di_ret);
-- 
1.8.2.1

Re: [PATCH 2/2] Btrfs-progs: open file with O_RDONLY on getting property

[Filipe David Manana]

On Fri, Apr 25, 2014 at 7:29 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> We don't need change an object when getting its property,
> so O_RDWR is not necessary in this case.  Moreover, the object
> may be readonly, with O_RDWR it will fail.


This was already addressed here Liu:
https://patchwork.kernel.org/patch/3995151/

thanks

>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  props.c |  5 ++++-
>  utils.c | 14 ++++++++++++--
>  utils.h |  1 +
>  3 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/props.c b/props.c
> index 53223a3..a49f8e0 100644
> --- a/props.c
> +++ b/props.c
> @@ -120,7 +120,10 @@ static int prop_compression(enum prop_object_type type,
>         char *buf = NULL;
>         char *xattr_name = NULL;
>
> -       fd = open_file_or_dir(object, &dirstream);
> +       if (!value)
> +               fd = open_file_or_dir_ro(object, &dirstream);
> +       else
> +               fd = open_file_or_dir(object, &dirstream);
>         if (fd == -1) {
>                 ret = -errno;
>                 fprintf(stderr, "ERROR: open %s failed. %s\n",
> diff --git a/utils.c b/utils.c
> index 3e9c527..110a0d1 100644
> --- a/utils.c
> +++ b/utils.c
> @@ -1622,7 +1622,7 @@ u64 parse_size(char *s)
>         return strtoull(s, NULL, 10) * mult;
>  }
>
> -int open_file_or_dir(const char *fname, DIR **dirstream)
> +static int __open_file_or_dir(const char *fname, DIR **dirstream, int ro)
>  {
>         int ret;
>         struct stat st;
> @@ -1638,7 +1638,7 @@ int open_file_or_dir(const char *fname, DIR **dirstream)
>                         return -1;
>                 fd = dirfd(*dirstream);
>         } else if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
> -               fd = open(fname, O_RDWR);
> +               fd = open(fname, (!!ro) ? O_RDONLY : O_RDWR);
>         } else {
>                 /*
>                  * we set this on purpose, in case the caller output
> @@ -1655,6 +1655,16 @@ int open_file_or_dir(const char *fname, DIR **dirstream)
>         return fd;
>  }
>
> +int open_file_or_dir(const char *fname, DIR **dirstream)
> +{
> +       return __open_file_or_dir(fname, dirstream, 0);
> +}
> +
> +int open_file_or_dir_ro(const char *fname, DIR **dirstream)
> +{
> +       return __open_file_or_dir(fname, dirstream, 1);
> +}
> +
>  void close_file_or_dir(int fd, DIR *dirstream)
>  {
>         if (dirstream)
> diff --git a/utils.h b/utils.h
> index 3c62066..5489cc2 100644
> --- a/utils.h
> +++ b/utils.h
> @@ -72,6 +72,7 @@ int btrfs_scan_block_devices(int run_ioctl);
>  u64 parse_size(char *s);
>  u64 arg_strtou64(const char *str);
>  int open_file_or_dir(const char *fname, DIR **dirstream);
> +int open_file_or_dir_ro(const char *fname, DIR **dirstream);
>  void close_file_or_dir(int fd, DIR *dirstream);
>  int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
>                 struct btrfs_ioctl_dev_info_args **di_ret);
> --
> 1.8.2.1
>



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 1/2] Btrfs-progs: set string end sing '\0' for property

[Filipe David Manana]

On Fri, Apr 25, 2014 at 7:29 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> Set string "xattr_name" 's end with '\0' so that it won't be
> violated in memory.
>
> With this fix, xfstest/btrfs/048 can pass on my box.
>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  props.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/props.c b/props.c
> index 4d0aeea..53223a3 100644
> --- a/props.c
> +++ b/props.c
> @@ -135,6 +135,7 @@ static int prop_compression(enum prop_object_type type,
>         }
>         memcpy(xattr_name, XATTR_BTRFS_PREFIX, XATTR_BTRFS_PREFIX_LEN);
>         memcpy(xattr_name + XATTR_BTRFS_PREFIX_LEN, name, strlen(name));
> +       xattr_name[XATTR_BTRFS_PREFIX_LEN + strlen(name)] = '\0';

Buffer overrun. You need to sum + 1 to the malloc argument above.

Thanks Liu.

>
>         if (value)
>                 sret = fsetxattr(fd, xattr_name, value, strlen(value), 0);
> --
> 1.8.2.1
>



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH 1/2] Btrfs-progs: set string end sing '\0' for property

[Liu Bo]

On Fri, Apr 25, 2014 at 11:07:49AM +0100, Filipe David Manana wrote:
> On Fri, Apr 25, 2014 at 7:29 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> > Set string "xattr_name" 's end with '\0' so that it won't be
> > violated in memory.
> >
> > With this fix, xfstest/btrfs/048 can pass on my box.
> >
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  props.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/props.c b/props.c
> > index 4d0aeea..53223a3 100644
> > --- a/props.c
> > +++ b/props.c
> > @@ -135,6 +135,7 @@ static int prop_compression(enum prop_object_type type,
> >         }
> >         memcpy(xattr_name, XATTR_BTRFS_PREFIX, XATTR_BTRFS_PREFIX_LEN);
> >         memcpy(xattr_name + XATTR_BTRFS_PREFIX_LEN, name, strlen(name));
> > +       xattr_name[XATTR_BTRFS_PREFIX_LEN + strlen(name)] = '\0';
> 
> Buffer overrun. You need to sum + 1 to the malloc argument above.

Oops, I made that mistake.

thanks,
-liubo

Re: [PATCH 2/2] Btrfs-progs: open file with O_RDONLY on getting property

[Liu Bo]

On Fri, Apr 25, 2014 at 11:04:39AM +0100, Filipe David Manana wrote:
> On Fri, Apr 25, 2014 at 7:29 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> > We don't need change an object when getting its property,
> > so O_RDWR is not necessary in this case.  Moreover, the object
> > may be readonly, with O_RDWR it will fail.
> 
> 
> This was already addressed here Liu:
> https://patchwork.kernel.org/patch/3995151/

Okay, please ignore this.

thanks,
-liubo

> 
> thanks
> 
> >
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  props.c |  5 ++++-
> >  utils.c | 14 ++++++++++++--
> >  utils.h |  1 +
> >  3 files changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/props.c b/props.c
> > index 53223a3..a49f8e0 100644
> > --- a/props.c
> > +++ b/props.c
> > @@ -120,7 +120,10 @@ static int prop_compression(enum prop_object_type type,
> >         char *buf = NULL;
> >         char *xattr_name = NULL;
> >
> > -       fd = open_file_or_dir(object, &dirstream);
> > +       if (!value)
> > +               fd = open_file_or_dir_ro(object, &dirstream);
> > +       else
> > +               fd = open_file_or_dir(object, &dirstream);
> >         if (fd == -1) {
> >                 ret = -errno;
> >                 fprintf(stderr, "ERROR: open %s failed. %s\n",
> > diff --git a/utils.c b/utils.c
> > index 3e9c527..110a0d1 100644
> > --- a/utils.c
> > +++ b/utils.c
> > @@ -1622,7 +1622,7 @@ u64 parse_size(char *s)
> >         return strtoull(s, NULL, 10) * mult;
> >  }
> >
> > -int open_file_or_dir(const char *fname, DIR **dirstream)
> > +static int __open_file_or_dir(const char *fname, DIR **dirstream, int ro)
> >  {
> >         int ret;
> >         struct stat st;
> > @@ -1638,7 +1638,7 @@ int open_file_or_dir(const char *fname, DIR **dirstream)
> >                         return -1;
> >                 fd = dirfd(*dirstream);
> >         } else if (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)) {
> > -               fd = open(fname, O_RDWR);
> > +               fd = open(fname, (!!ro) ? O_RDONLY : O_RDWR);
> >         } else {
> >                 /*
> >                  * we set this on purpose, in case the caller output
> > @@ -1655,6 +1655,16 @@ int open_file_or_dir(const char *fname, DIR **dirstream)
> >         return fd;
> >  }
> >
> > +int open_file_or_dir(const char *fname, DIR **dirstream)
> > +{
> > +       return __open_file_or_dir(fname, dirstream, 0);
> > +}
> > +
> > +int open_file_or_dir_ro(const char *fname, DIR **dirstream)
> > +{
> > +       return __open_file_or_dir(fname, dirstream, 1);
> > +}
> > +
> >  void close_file_or_dir(int fd, DIR *dirstream)
> >  {
> >         if (dirstream)
> > diff --git a/utils.h b/utils.h
> > index 3c62066..5489cc2 100644
> > --- a/utils.h
> > +++ b/utils.h
> > @@ -72,6 +72,7 @@ int btrfs_scan_block_devices(int run_ioctl);
> >  u64 parse_size(char *s);
> >  u64 arg_strtou64(const char *str);
> >  int open_file_or_dir(const char *fname, DIR **dirstream);
> > +int open_file_or_dir_ro(const char *fname, DIR **dirstream);
> >  void close_file_or_dir(int fd, DIR *dirstream);
> >  int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
> >                 struct btrfs_ioctl_dev_info_args **di_ret);
> > --
> > 1.8.2.1
> >
> 
> 
> 
> -- 
> Filipe David Manana,
> 
> "Reasonable men adapt themselves to the world.
>  Unreasonable men adapt the world to themselves.
>  That's why all progress depends on unreasonable men."

[PATCH v2] Btrfs-progs: set string end sing '\0' for property

[Liu Bo]

Set string "xattr_name" 's end with '\0' so that it won't be
violated in memory.

With this fix, xfstest/btrfs/048 can pass on my box.

Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
v2: avoid buffer overflow of malloc().

 props.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/props.c b/props.c
index 9ff63ca..cc87454 100644
--- a/props.c
+++ b/props.c
@@ -131,13 +131,14 @@ static int prop_compression(enum prop_object_type type,
 		goto out;
 	}
 
-	xattr_name = malloc(XATTR_BTRFS_PREFIX_LEN + strlen(name));
+	xattr_name = malloc(XATTR_BTRFS_PREFIX_LEN + strlen(name) + 1);
 	if (!xattr_name) {
 		ret = -ENOMEM;
 		goto out;
 	}
 	memcpy(xattr_name, XATTR_BTRFS_PREFIX, XATTR_BTRFS_PREFIX_LEN);
 	memcpy(xattr_name + XATTR_BTRFS_PREFIX_LEN, name, strlen(name));
+	xattr_name[XATTR_BTRFS_PREFIX_LEN + strlen(name)] = '\0';
 
 	if (value)
 		sret = fsetxattr(fd, xattr_name, value, strlen(value), 0);
-- 
1.7.7

Re: [PATCH v2] Btrfs-progs: set string end sing '\0' for property

[Filipe David Manana]

On Fri, Apr 25, 2014 at 11:44 AM, Liu Bo <bo.li.liu@oracle.com> wrote:
> Set string "xattr_name" 's end with '\0' so that it won't be
> violated in memory.
>
> With this fix, xfstest/btrfs/048 can pass on my box.
>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

Reviewed-by: Filipe Manana <fdmanana@gmail.com>

> ---
> v2: avoid buffer overflow of malloc().
>
>  props.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/props.c b/props.c
> index 9ff63ca..cc87454 100644
> --- a/props.c
> +++ b/props.c
> @@ -131,13 +131,14 @@ static int prop_compression(enum prop_object_type type,
>                 goto out;
>         }
>
> -       xattr_name = malloc(XATTR_BTRFS_PREFIX_LEN + strlen(name));
> +       xattr_name = malloc(XATTR_BTRFS_PREFIX_LEN + strlen(name) + 1);
>         if (!xattr_name) {
>                 ret = -ENOMEM;
>                 goto out;
>         }
>         memcpy(xattr_name, XATTR_BTRFS_PREFIX, XATTR_BTRFS_PREFIX_LEN);
>         memcpy(xattr_name + XATTR_BTRFS_PREFIX_LEN, name, strlen(name));
> +       xattr_name[XATTR_BTRFS_PREFIX_LEN + strlen(name)] = '\0';
>
>         if (value)
>                 sret = fsetxattr(fd, xattr_name, value, strlen(value), 0);
> --
> 1.7.7
>



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."