Re: ftp: Cleanup port number to string conversion.

Hi Collin,

On Wed, Jul 31, 2024 at 10:29:03PM -0700, Collin Funk wrote:
> 
> I haven't pushed this in case anyone wants to comment beforehand. The
> change was prompted by these warnings:

Thanks for looking into fixing warnings! :-)

> ftp.c: In function 'hookup':
> ftp.c:145:45: warning: '%u' directive output may be truncated writing between 1 and 10 bytes into a region of size 9 [-Wformat-truncation=]
>   145 |   snprintf (portstr, sizeof (portstr) - 1, "%u", port);
>       |                                             ^~
> ftp.c:145:44: note: using the range [0, 4294967295] for directive argument
>   145 |   snprintf (portstr, sizeof (portstr) - 1, "%u", port);
>       |                                            ^~~~
> ftp.c:145:3: note: 'snprintf' output between 2 and 11 bytes into a destination of size 9
>   145 |   snprintf (portstr, sizeof (portstr) - 1, "%u", port);
>       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> In this case I don't think these warnings point to anything dangerous,
> but only highlight not-so clear code. The portstr is 'static' so it will
> always be NUL terminated at the start of the program. Then the only
> caller to this hookup function verifies that 'port' is 1-65535 inclusive
> so it won't overflow.

I also think that this warning only points out possible information loss.
Overflow is prevented by the size parameter set to one less than the
buffer size, so the NUL byte at the end of the buffer is not overwritten.
The buffer is sufficiently large for a port number followed by a NUL byte.

> To make this more clear we change 'port' to in_port_t. POSIX requires
> this to be equivalent to uint16_t so GCC can tell the buffer won't
> overflow. Then also reduce the buffer size from 10 to 6 using Gnulib's
> INT_STRLEN_BOUND. Makes things more clear IMO.

This seems OK.

A different approach could be to use an int for port, but check the
value at the start of the hookup() function.  That would avoid relying
on a check in a different file and thus might be more robust against
future changes.  This might require some extra code to tell GCC that
this is OK, I did not check that.

> [...]
> @@ -142,7 +143,7 @@ hookup (char *host, int port)
>    rhost = strdup (host);
>  #endif
>  
> -  snprintf (portstr, sizeof (portstr) - 1, "%u", port);
> +  sprintf (portstr, "%u", port);
>    memset (&hisctladdr, 0, sizeof (hisctladdr));
>    memset (&hints, 0, sizeof (hints));
>  

I prefer the original code with snprintf() here, because it directly
shows that it does not overflow the buffer and that the last byte of
the buffer is unchanged.

Thanks,
Erik