Re: [PATCH v2] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination for keyfile format

[Easwar Hariharan <eahariha@linux.microsoft.com>]

On 3/15/2024 7:09 AM, Shradha Gupta wrote:

<snip>

>>>>  }
>>>>  
>>>> +static int process_dns_gateway_nm(FILE *f, char *ip_string, int type,
>>>> +				  int ip_sec)
>>>> +{
>>>> +	char addr[INET6_ADDRSTRLEN], *output_str;
>>>> +	int ip_offset = 0, error = 0, ip_ver;
>>>> +	char *param_name;
>>>> +
>>>> +	output_str = (char *)calloc(INET6_ADDRSTRLEN * MAX_IP_ENTRIES,
>>>> +				    sizeof(char));
>>>> +
>>>> +	if (!output_str)
>>>> +		return -ENOMEM;
>>>> +
>>>> +	memset(addr, 0, sizeof(addr));
>>>> +
>>>> +	if (type == DNS) {
>>>> +		param_name = "dns";
>>>> +	} else if (type == GATEWAY) {
>>>> +		param_name = "gateway";
>>>> +	} else {
>>>> +		error = -EINVAL;
>>>> +		goto cleanup;
>>>> +	}
>>>> +
>>>> +	while (parse_ip_val_buffer(ip_string, &ip_offset, addr,
>>>> +				   (MAX_IP_ADDR_SIZE * 2))) {
>>>> +		ip_ver = ip_version_check(addr);
>>>> +		if (ip_ver < 0)
>>>> +			continue;
>>>> +
>>>> +		if ((ip_ver == IPV4 && ip_sec == IPV4) ||
>>>> +		    (ip_ver == IPV6 && ip_sec == IPV6)) {
>>>> +			if (((INET6_ADDRSTRLEN * MAX_IP_ENTRIES) - strlen(output_str)) >
>>>> +			    (strlen(addr))) {
>>>> +				strcat(output_str, addr);
>>>> +				strcat(output_str, ",");
>>>
>>> Prefer strncat() here
> Is this needed with the bound check above. I am trying to keep parity with the rest of the 
> file.
>>>
<snip>

I missed this earlier because my comment was more of a general best practice comment.

Note that in the worst case where your bounds check (INET6_ADDRSTRLEN*MAX_IP_ENTRIES) - strlen(output_str) equals (strlen(addr) + 1),
you will be adding strlen(addr)+1(","), and end up with no ASCII NUL '\0' delimiter.

If you're going to rely on the bounds check to ensure correctness, you'll need to correct that. Generally speaking, strncat would still
be helpful in case the bounds check changes in the future.

Thanks,
Easwar