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.liu@xxxxxxxxxx>
>>>>> Signed-off-by: Anand Jain <anand.jain@xxxxxxxxxx>
>>>>> Cc: Miao Xie <miaox@xxxxxxxxxxxxxx>
>> [...]
>>>>> +
>>>>> + 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
