Received-SPF: pass (google.com: domain of linux-kernel+bounces-104682-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <dd1b378d-6750-419f-8c46-a8f42c0ebe11@linux.microsoft.com>
Date: Fri, 15 Mar 2024 09:56:22 -0700
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination for
 keyfile format
To: Shradha Gupta <shradhagupta@linux.microsoft.com>
Cc: linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
 "K. Y. Srinivasan" <kys@microsoft.com>,
 Haiyang Zhang <haiyangz@microsoft.com>, Wei Liu <wei.liu@kernel.org>,
 Dexuan Cui <decui@microsoft.com>, Long Li <longli@microsoft.com>,
 Michael Kelley <mikelley@microsoft.com>, Olaf Hering <olaf@aepfle.de>,
 Ani Sinha <anisinha@redhat.com>, Shradha Gupta <shradhagupta@microsoft.com>
References: <1710247112-7414-1-git-send-email-shradhagupta@linux.microsoft.com>
 <3bf8844a-3e19-4105-8cce-2b1f8f98d3bc@linux.microsoft.com>
 <20240313052212.GB22465@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
 <20240315140914.GA14685@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
Content-Language: en-CA
From: Easwar Hariharan <eahariha@linux.microsoft.com>
In-Reply-To: <20240315140914.GA14685@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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