Re: [RFC PATCH] mkfs.f2fs: avoid dumplicate extension

[Chao Yu <chao@kernel.org>]

Hi Sheng Yong,

On 11/28/15 8:39 AM, Sheng Yong wrote:
> 
> On 11/27/2015 9:09 PM, Chao Yu wrote:
>> Hi Sheng Yong,
>>
>> On 11/28/15 12:55 AM, Sheng Yong wrote:
>>> Before copying user specified extension to extension_list, check if it is
>>> already in the list.
>>>
>>> Signed-off-by: Sheng Yong <shengyong1@huawei.com>
>>> ---
>>>  mkfs/f2fs_format.c | 23 ++++++++++++++++++++++-
>>>  1 file changed, 22 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/mkfs/f2fs_format.c b/mkfs/f2fs_format.c
>>> index 176bdea..b7ea19f 100644
>>> --- a/mkfs/f2fs_format.c
>>> +++ b/mkfs/f2fs_format.c
>>> @@ -118,6 +118,26 @@ const char *media_ext_lists[] = {
>>>  	NULL
>>>  };
>>>  
>>> +static int extension_exist(const char *name, const int len)
>>
>> bool is_extension_exist?
>>
>>> +{
>>> +	const char **extlist = media_ext_lists;
>>> +	int name_len, min_len;
>>> +	int ret;
>>> +
>>> +
>>> +	while (*extlist) {
>>> +		name_len = strlen(*extlist);
>>> +		min_len = name_len < len ? name_len : len;
>>> +		ret = strncmp(*extlist, name, min_len);
>>
>> How about using strcmp(*extlist, name)? It will return 0 only if the two strings
>> are the same.
>>
> Hi, Chao Yu
> 
> Thanks for your reply. I was considering strcmp may have security issue.

Could you share details about the security issue of using strcmp?

Thanks,

>>> +
>>> +		if (ret == 0 && name_len == len)
>>> +			return 1;
>>> +		extlist++;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static void configure_extension_list(void)
>>>  {
>>>  	const char **extlist = media_ext_lists;
>>> @@ -144,7 +164,8 @@ static void configure_extension_list(void)
>>>  	ue = strtok(ext_str, ",");
>>>  	while (ue != NULL) {
>>>  		name_len = strlen(ue);
>>
>> We should verify the length of extension to avoid memcpy overflow. I will send a
>> patch for fixing.
> I agree. And I think we should make sure there is a '\0' at the end of each string
> in extension_list.
>>
>>> -		memcpy(sb.extension_list[i++], ue, name_len);
>>> +		if (!extension_exist(ue, name_len))
>>
>> How do you think of comparing extension user defined with sb.extension_list?
>> In this way, duplication user defined can be avoided completely.
> Right. I'll send a v2 latter.
> 
> thanks,
> Sheng
>>
>> Thanks,
>>
>>> +			memcpy(sb.extension_list[i++], ue, name_len);
>>>  		ue = strtok(NULL, ",");
>>>  		if (i >= F2FS_MAX_EXTENSION)
>>>  			break;
>>>
>>
>> .
>>
> 

------------------------------------------------------------------------------