[PATCH] getaddrinfo() does not detect invalid numeric services (bug 16208)

[Petr Menšík]

I think value 0 is completely valid. It is important especially when 
AI_PASSIVE is used. Then it allows auto-allocating new random port. I do 
not see a reason why that should be refused as invalid. I think that 
would cause regression.

At least AF_VSOCK (man 7 vsock) has svm_port defined as unsigned int. I 
think that is the reason why AF_INET and AF_INET6 only were restricted 
to 16bit value. I am not sure even bind() will work with that. Socktype 
were checked because only TCP and UDP has defined port to 16 bit 
unsigned int. I am not sure how SOCK_SEQPACKET can be used over IP and 
whether it has the same ports. I would assume yes, it would, but does 
someone actually know that? Same with SOCK_DCCP.

It seems SOCK_RAW uses sockaddr_in, according to man page. Therefore it 
should be included as well in ai_socktype check, if that is still needed.

Is it safe to assume AF_INET and AF_INET6 has the same bit size of port 
for any socket types? Is there any case known, where that assumption 
based only on ai_family would not be true? I have never used anything 
except inet protocols and unspec myself.

Regards,
Petr

On 02. 12. 24 16:54, Wilco Dijkstra wrote:
> Hi Petr,
>
> So this looks like an alternative fix for this bug, previous proposal was [1] (and my review [2]).
>
> @@ -2377,6 +2377,12 @@ getaddrinfo (const char *name, const char *service,
>   
>   	  gaih_service.num = -1;
>   	}
> +      else if ((hints->ai_family == AF_INET || hints->ai_family == AF_INET6)
> +               && (hints->ai_socktype == SOCK_STREAM || hints->ai_socktype == SOCK_DGRAM)
> +               && gaih_service.num > UINT16_MAX)
> +        {
> +          return EAI_SERVICE;
> +        }
>
> The interesting thing is that this patch does something different than [1], both in
> terms of overflow checking of strtoul() (which I think should be added) and the
> return value when overflow is detected (where it's not clear what should be returned).
I think EAI_SERVICE is better. EAI_NONAME has somehow too many meaning 
already, too many ors already for its definition.
> Also what is the reason for checking ai_family and ai_socktype too? Can the service
> be outside the valid range 1..65535 in other cases, and thus we should support full
> 64-bit range?
At least AF_VSOCK supports unsigned int. That is just 32 bits, but more 
than common 16 bits.
>
> Cheers,
> Wilco
>
> [1]https://sourceware.org/pipermail/libc-alpha/2024-September/159785.html
> [2]https://sourceware.org/pipermail/libc-alpha/2024-November/161184.html
>
-- 
Petr Menšík
Senior Software Engineer, RHEL
Red Hat,http://www.redhat.com/
PGP: DFCF908DB7C87E8E529925BC4931CA5B6C9FC5CB
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20241203/613b10ac/attachment.htm>