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.
