On 12/18/2012 10:21 AM, Miao Xie wrote: > On mon, 17 Dec 2012 18:34:41 +0100, Goffredo Baroncelli wrote: >> On 12/17/2012 02:30 PM, Jeff Liu wrote: >>> On 12/17/2012 07:57 PM, Miao Xie wrote: >>>> On mon, 17 Dec 2012 19:22:11 +0800, Jeff Liu wrote: >>>>> Introduce a new ioctl BTRFS_IOC_SET_FSLABEL to change the label of a >>>>> mounted file system. >>>>> >>>>> Signed-off-by: Jie Liu <jeff....@oracle.com> >>>>> Signed-off-by: Anand Jain <anand.j...@oracle.com> >>>>> Cc: Miao Xie <mi...@cn.fujitsu.com> >> [...] >>>>> + >>>>> + if (strlen(label) > BTRFS_LABEL_SIZE - 1) >>>>> + return -EINVAL; >>>> >>>> I think we should use strnlen() >>> AFAICS, strnlen() is better only if the caller need to get the length of >>> a length-limited string and make use of it proceeding, which means that >>> the procedure would not return an error even if the length is beyond the >>> limit. Or if the caller need to examine if a length-limited string is >>> nul-terminated or not in a manner below, >>> if (strnlen(buf, MAX_BUF_SIZE) == MAX_BUF_SIZE) { >>> .... >>> } >>> >>> I don't think it really needed here since the logic is clear with >>> strlen(), or Am I miss anything? >> >> I think that Miao fears strlen() searching a zero could go beyond the >> page limit touching an un-mapped page and raising an segmentation fault.... > > Yes, so I think the following check is better. > > if (strnlen(buf, BTRFS_LABEL_SIZE) == BTRFS_LABEL_SIZE) > return -EINVAL; Generally speaking, the user would not input a large string for normal purpose, so strnlen() will always have a bit waste(can be ignore here) with the counter self-check. i.e. for (; count--, ;). > Thanks > Miao > >> I think that we should change the code as >> >> + label[BTRFS_LABEL_SIZE - 1] = 0; >> + >> + if (strlen(label) > BTRFS_LABEL_SIZE - 1) >> + return -EINVAL; Both suggestion are fine to me, but I prefer to above approach.
Thanks, -Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html