bug-inetutils
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: ftp: Cleanup port number to string conversion.


From: Erik Auerswald
Subject: Re: ftp: Cleanup port number to string conversion.
Date: Thu, 1 Aug 2024 19:30:51 +0200

Hi Collin,

On Thu, Aug 01, 2024 at 09:52:51AM +0200, Erik Auerswald wrote:
> 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);
> >       |   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > 
> > [...]
> 
> 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.

I have looked into this a bit, I'd say that my idea is worse than your
approach, because it would require too many changes.

> > [...]
> > @@ -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.

I'd still prefer keeping snprintf() here.

Best regards,
Erik



reply via email to

[Prev in Thread] Current Thread [Next in Thread]