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

[Shradha Gupta]

On Fri, Mar 15, 2024 at 09:56:22AM -0700, Easwar Hariharan wrote:
> 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
got it. Thanks.