Received-SPF: pass (google.com: domain of linux-kernel+bounces-105644-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Sun, 17 Mar 2024 19:02:55 -0700
From: Shradha Gupta <shradhagupta@linux.microsoft.com>
To: Easwar Hariharan <eahariha@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>
Subject: Re: [PATCH v2] hv/hv_kvp_daemon: Handle IPv4 and Ipv6 combination
 for keyfile format
Message-ID: <20240318020255.GA28704@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net>
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>
 <dd1b378d-6750-419f-8c46-a8f42c0ebe11@linux.microsoft.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <dd1b378d-6750-419f-8c46-a8f42c0ebe11@linux.microsoft.com>
User-Agent: Mutt/1.5.21 (2010-09-15)

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.