Hi,
I haven't pushed this in case anyone wants to comment beforehand. The
change was prompted by these 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.
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.
Collin
>From 37fd1ef8920d635d1ac05a71fe92643c0e367214 Mon Sep 17 00:00:00 2001
From: Collin Funk <collin.fu...@gmail.com>
Date: Wed, 31 Jul 2024 22:14:01 -0700
Subject: [PATCH] ftp: Cleanup port number to string conversion.
* bootstrap.conf (gnulib_modules): Add intprops.
* ftp/ftp.c: Include intprops.h.
(portstr): Size buffer to fit exactly 'in_port_t' and a NUL byte.
(hookup): Use 'in_port_t' to represent the port instead of int. Use
sprintf since the buffer size is safe.
* ftp/extern.h (hookup): Adjust deceleration.
* ftp/ftp_var.h: Include netinet/in.h for type definitions.
---
bootstrap.conf | 1 +
ftp/extern.h | 2 +-
ftp/ftp.c | 7 ++++---
ftp/ftp_var.h | 3 +++
4 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/bootstrap.conf b/bootstrap.conf
index 9026eccc..e6f56a27 100644
--- a/bootstrap.conf
+++ b/bootstrap.conf
@@ -68,6 +68,7 @@ getusershell
git-version-gen
gitlog-to-changelog
glob
+intprops
inttostr
ioctl
malloc-gnu
diff --git a/ftp/extern.h b/ftp/extern.h
index f773ec82..e339ed72 100644
--- a/ftp/extern.h
+++ b/ftp/extern.h
@@ -80,7 +80,7 @@ int getreply (int);
char *globulize (char *);
char *gunique (char *);
void help (int, char **);
-char *hookup (char *, int);
+char *hookup (char *, in_port_t);
void site_idle (int, char **);
int initconn (void);
void intr (int sig);
diff --git a/ftp/ftp.c b/ftp/ftp.c
index 9ec40a70..344089d5 100644
--- a/ftp/ftp.c
+++ b/ftp/ftp.c
@@ -88,6 +88,7 @@
# include <idna.h>
#endif
+#include <intprops.h>
#include <timespec.h>
#include "ftp_var.h"
@@ -107,7 +108,7 @@ socklen_t ctladdrlen; /* Applies to all addresses. */
/* For temporary resolving: hookup() and initconn()/noport. */
static char ia[INET6_ADDRSTRLEN];
-static char portstr[10];
+static char portstr[INT_STRLEN_BOUND (in_port_t) + 1];
FILE *cin, *cout;
@@ -116,7 +117,7 @@ FILE *cin, *cout;
#endif
char *
-hookup (char *host, int port)
+hookup (char *host, in_port_t port)
{
struct addrinfo hints, *ai = NULL, *res = NULL;
struct timeval timeout;
@@ -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));
diff --git a/ftp/ftp_var.h b/ftp/ftp_var.h
index 63dc8d82..f1e7746c 100644
--- a/ftp/ftp_var.h
+++ b/ftp/ftp_var.h
@@ -53,6 +53,9 @@
#include <setjmp.h>
#include <progname.h>
+/* For in_port_t. */
+#include <netinet/in.h>
+
#ifndef FTP_EXTERN
# define FTP_EXTERN extern
#endif
--
2.45.2
