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
