[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