Re: [PATCH v2] lib/cfgfile: replace strcat with strlcat

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 3/8/2019 2:02 PM, Bruce Richardson wrote:
> On Fri, Mar 08, 2019 at 12:45:50PM +0000, Chaitanya Babu Talluri wrote:
>> Replace strcat with strlcat to avoid buffer overflow.
>>
>> Fixes: a6a47ac9c2 ("cfgfile: rework load function")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Chaitanya Babu Talluri <tallurix.chaitanya.babu@intel.com>
>> ---
>> v2: Instead of strcat, used strlcat.
>> ---
>>  lib/librte_cfgfile/rte_cfgfile.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/librte_cfgfile/rte_cfgfile.c b/lib/librte_cfgfile/rte_cfgfile.c
>> index 7d8c941ea..3296bb6f8 100644
>> --- a/lib/librte_cfgfile/rte_cfgfile.c
>> +++ b/lib/librte_cfgfile/rte_cfgfile.c
>> @@ -8,6 +8,7 @@
>>  #include <ctype.h>
>>  #include <errno.h>
>>  #include <rte_common.h>
>> +#include <rte_string_fns.h>
>>  
>>  #include "rte_cfgfile.h"
>>  
>> @@ -224,10 +225,11 @@ rte_cfgfile_load_with_params(const char *filename, int flags,
>>  			_strip(split[1], strlen(split[1]));
>>  			char *end = memchr(split[1], '\\', strlen(split[1]));
>>  
>> +			size_t split_len = strlen(split[1]) + 1;
>>  			while (end != NULL) {
>>  				if (*(end+1) == params->comment_character) {
>>  					*end = '\0';
>> -					strcat(split[1], end+1);
>> +					strlcat(split[1], end+1, split_len);
> 
> I don't think this will do what you want. Remember that strlcat takes the
> total length of the buffer, which means that if split_len is set to the
> current length (as you do before the while statement), then passing that
> as the length parameter will cause strlcat to do nothing, since it sees the
> buffer as already full.

The logic doesn't lengthen the 'split[1]' content, indeed it reduces the initial
size although it uses string concatenation, that is why it should be OK to use
'split_len' here.

What code does is, it finds specific char in 'split' buffer and removes it by
shifting remaining chars one byte to the left. So it shouldn't pass the initial
size of the buffer.

There is a overlapping strings concern, which 'strcat' & 'strlcat' don't
support, but I guess it is OK here since we are sure that strings are separated
by a NULL, so where a char read and written should be different although overall
dst and src buffers overlap.