| Message ID | 20190906095846.30592-1-git@vladimir.panteleev.md (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: mkfs: fix xattr enumeration | expand |
On 6.09.19 г. 12:58 ч., Vladimir Panteleev wrote: > Use the return value of listxattr instead of tokenizing. > > The end of the extended attribute list is indicated by the return > value, not an empty list item (two consecutive NULs). Using strtok > in this way thus sometimes caused add_xattr_item to reuse stack data > in xattr_list from the previous invocation, thus querying attributes > that are not actually in the file's xattr list. > > Issue: #194 > Signed-off-by: Vladimir Panteleev <git@vladimir.panteleev.md> Can you elaborate how to trigger this? I tried by creating a folder with 2 files and set 5 xattr to the first file and 1 to the second. Then I run mkfs.btrfs -r /path/to/dir /dev/vdc and stepped through the code with gdb and didn't see any issues. Ideally I'd like to see a regression test for this issue. Your code looks correct. > --- > mkfs/rootdir.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c > index 51411e02..c86159e7 100644 > --- a/mkfs/rootdir.c > +++ b/mkfs/rootdir.c > @@ -228,10 +228,9 @@ static int add_xattr_item(struct btrfs_trans_handle *trans, > int ret; > int cur_name_len; > char xattr_list[XATTR_LIST_MAX]; > + char *xattr_list_end; > char *cur_name; > char cur_value[XATTR_SIZE_MAX]; > - char delimiter = '\0'; > - char *next_location = xattr_list; > > ret = llistxattr(file_name, xattr_list, XATTR_LIST_MAX); > if (ret < 0) { > @@ -243,10 +242,10 @@ static int add_xattr_item(struct btrfs_trans_handle *trans, > if (ret == 0) > return ret; > > - cur_name = strtok(xattr_list, &delimiter); > - while (cur_name != NULL) { > + xattr_list_end = xattr_list + ret; > + cur_name = xattr_list; > + while (cur_name < xattr_list_end) { > cur_name_len = strlen(cur_name); > - next_location += cur_name_len + 1; > > ret = lgetxattr(file_name, cur_name, cur_value, XATTR_SIZE_MAX); > if (ret < 0) { > @@ -266,7 +265,7 @@ static int add_xattr_item(struct btrfs_trans_handle *trans, > file_name); > } > > - cur_name = strtok(next_location, &delimiter); > + cur_name += cur_name_len + 1; > } > > return ret; >
Hi Nikolay, Unfortunately, as I mentioned in the issue, I have also been unable to reproduce this issue locally. Please see here: https://github.com/kdave/btrfs-progs/issues/194 The reporter tested the patch and confirmed that it worked. Additionally, they have provided strace output which appears to confirm that the bug description in the patch commit message indeed corresponds to the behavior they are observing on their machine. Perhaps the issue might be reproducible in an environment closer to the reporter's (looks like some Fedora distro judging by the uname). On Mon, 9 Sep 2019 at 11:22, Nikolay Borisov <nborisov@suse.com> wrote: > > > > On 6.09.19 г. 12:58 ч., Vladimir Panteleev wrote: > > Use the return value of listxattr instead of tokenizing. > > > > The end of the extended attribute list is indicated by the return > > value, not an empty list item (two consecutive NULs). Using strtok > > in this way thus sometimes caused add_xattr_item to reuse stack data > > in xattr_list from the previous invocation, thus querying attributes > > that are not actually in the file's xattr list. > > > > Issue: #194 > > Signed-off-by: Vladimir Panteleev <git@vladimir.panteleev.md> > > Can you elaborate how to trigger this? I tried by creating a folder with > 2 files and set 5 xattr to the first file and 1 to the second. Then I > run mkfs.btrfs -r /path/to/dir /dev/vdc and stepped through the code > with gdb and didn't see any issues. Ideally I'd like to see a regression > test for this issue. > > Your code looks correct. > > > --- > > mkfs/rootdir.c | 11 +++++------ > > 1 file changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c > > index 51411e02..c86159e7 100644 > > --- a/mkfs/rootdir.c > > +++ b/mkfs/rootdir.c > > @@ -228,10 +228,9 @@ static int add_xattr_item(struct btrfs_trans_handle *trans, > > int ret; > > int cur_name_len; > > char xattr_list[XATTR_LIST_MAX]; > > + char *xattr_list_end; > > char *cur_name; > > char cur_value[XATTR_SIZE_MAX]; > > - char delimiter = '\0'; > > - char *next_location = xattr_list; > > > > ret = llistxattr(file_name, xattr_list, XATTR_LIST_MAX); > > if (ret < 0) { > > @@ -243,10 +242,10 @@ static int add_xattr_item(struct btrfs_trans_handle *trans, > > if (ret == 0) > > return ret; > > > > - cur_name = strtok(xattr_list, &delimiter); > > - while (cur_name != NULL) { > > + xattr_list_end = xattr_list + ret; > > + cur_name = xattr_list; > > + while (cur_name < xattr_list_end) { > > cur_name_len = strlen(cur_name); > > - next_location += cur_name_len + 1; > > > > ret = lgetxattr(file_name, cur_name, cur_value, XATTR_SIZE_MAX); > > if (ret < 0) { > > @@ -266,7 +265,7 @@ static int add_xattr_item(struct btrfs_trans_handle *trans, > > file_name); > > } > > > > - cur_name = strtok(next_location, &delimiter); > > + cur_name += cur_name_len + 1; > > } > > > > return ret; > >
On 9.09.19 г. 14:32 ч., Vladimir Panteleev wrote: > Hi Nikolay, > > Unfortunately, as I mentioned in the issue, I have also been unable to > reproduce this issue locally. > > Please see here: > https://github.com/kdave/btrfs-progs/issues/194 > > The reporter tested the patch and confirmed that it worked. > Additionally, they have provided strace output which appears to > confirm that the bug description in the patch commit message indeed > corresponds to the behavior they are observing on their machine. > > Perhaps the issue might be reproducible in an environment closer to > the reporter's (looks like some Fedora distro judging by the uname). Right, looking at the kernel portion listxattr just copies the attribute names into the passed user pointer. So if the data copied is less than XATTR_LIST_MAX then the data beyond the return value of llistxattr is undefined which could cause the described problem. I agree that your code is more correct as it handles data only in the range [0...ret]. Consider this patch: Reviewed-by: Nikolay Borisov <nborisov@suse.com>
On Fri, Sep 06, 2019 at 09:58:46AM +0000, Vladimir Panteleev wrote: > Use the return value of listxattr instead of tokenizing. > > The end of the extended attribute list is indicated by the return > value, not an empty list item (two consecutive NULs). Using strtok > in this way thus sometimes caused add_xattr_item to reuse stack data > in xattr_list from the previous invocation, thus querying attributes > that are not actually in the file's xattr list. > > Issue: #194 > Signed-off-by: Vladimir Panteleev <git@vladimir.panteleev.md> Added to devel, thanks.
diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c index 51411e02..c86159e7 100644 --- a/mkfs/rootdir.c +++ b/mkfs/rootdir.c @@ -228,10 +228,9 @@ static int add_xattr_item(struct btrfs_trans_handle *trans, int ret; int cur_name_len; char xattr_list[XATTR_LIST_MAX]; + char *xattr_list_end; char *cur_name; char cur_value[XATTR_SIZE_MAX]; - char delimiter = '\0'; - char *next_location = xattr_list; ret = llistxattr(file_name, xattr_list, XATTR_LIST_MAX); if (ret < 0) { @@ -243,10 +242,10 @@ static int add_xattr_item(struct btrfs_trans_handle *trans, if (ret == 0) return ret; - cur_name = strtok(xattr_list, &delimiter); - while (cur_name != NULL) { + xattr_list_end = xattr_list + ret; + cur_name = xattr_list; + while (cur_name < xattr_list_end) { cur_name_len = strlen(cur_name); - next_location += cur_name_len + 1; ret = lgetxattr(file_name, cur_name, cur_value, XATTR_SIZE_MAX); if (ret < 0) { @@ -266,7 +265,7 @@ static int add_xattr_item(struct btrfs_trans_handle *trans, file_name); } - cur_name = strtok(next_location, &delimiter); + cur_name += cur_name_len + 1; } return ret;
Use the return value of listxattr instead of tokenizing. The end of the extended attribute list is indicated by the return value, not an empty list item (two consecutive NULs). Using strtok in this way thus sometimes caused add_xattr_item to reuse stack data in xattr_list from the previous invocation, thus querying attributes that are not actually in the file's xattr list. Issue: #194 Signed-off-by: Vladimir Panteleev <git@vladimir.panteleev.md> --- mkfs/rootdir.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)