Hi Goffredo,
Thanks for your review.
On 12/19/2012 02:00 AM, Goffredo Baroncelli wrote:
> Hi Jeff,
>
> On 12/18/2012 04:31 AM, Miao Xie wrote:
> [...]
>>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> [...]
>
>>> +static int btrfs_ioctl_set_fslabel(struct file *file, void __user *arg)
>>> +{
>>> + struct btrfs_root *root = BTRFS_I(fdentry(file)->d_inode)->root;
>>> + struct btrfs_super_block *super_block = root->fs_info->super_copy;
>>> + struct btrfs_trans_handle *trans;
>>> + char label[BTRFS_LABEL_SIZE];
>>> + int ret;
>>> +
>>> + if (!capable(CAP_SYS_ADMIN))
>>> + return -EPERM;
>>> +
>>> + if (copy_from_user(label, arg, sizeof(label)))
>>> + return -EFAULT;
>>> +
>>> + if (strnlen(label, BTRFS_LABEL_SIZE) == BTRFS_LABEL_SIZE)
>>> + return -EINVAL;
>>> +
>>> + ret = mnt_want_write_file(file);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + mutex_lock(&root->fs_info->volume_mutex);
>>> + trans = btrfs_start_transaction(root, 1);
>>> + if (IS_ERR(trans)) {
>>> + ret = PTR_ERR(trans);
>>> + goto out_unlock;
>>> + }
>>> +
>>> + strcpy(super_block->label, label);
>
> I think that you removed for mistake the following line
>
> + label[BTRFS_LABEL_SIZE - 1] = '\0';
I removed it since it was used to cut the label string off the max array
size but now we have the previous strnlen().
>
> In the V5 patch it was present.
>
> May be we could replace strcpy() with strlcpy(super_block->label, label,
> BTRFS_LABEL_SIZE-1) ?
That is ok to me. However, it should be strlcpy(super_block->label,
label, BTRFS_LABEL_SIZE) ranther than 'BTRFS_LABREL_SIZE -1' because
strlcpy() does "size - 1" internally. i.e.
strlcpy(char *d, const char *s, size_t size)
{
size_t ret = strlen(s);
.....
size_t len = (ret >= size) ? size - 1 : ret;
....
}
But does the current implementation make anything wrong? :)
Thanks,
-Jeff
>
> BR
> G.Baroncelli
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
