git: 25945af47e7a - main - tftpd: silence gcc overflow warnings

The branch main has been updated by des:

URL: https://cgit.FreeBSD.org/src/commit/?id=25945af47e7a1d06c44c8c160045a866e90569ab

commit 25945af47e7a1d06c44c8c160045a866e90569ab
Author:     Dag-Erling Smørgrav <des@FreeBSD.org>
AuthorDate: 2024-05-10 21:15:54 +0000
Commit:     Dag-Erling Smørgrav <des@FreeBSD.org>
CommitDate: 2024-05-10 21:16:26 +0000

    tftpd: silence gcc overflow warnings
    
    GCC 13 complains that we might be writing too much to an on-stack buffer
    when createing a filename.
    
    In practice there is a check that filename isn't too long given the
    time format and other static characters so GCC is incorrect, but GCC
    isn't wrong that we're potentially trying to put a MAXPATHLEN length
    string + some other characters into a MAXPATHLEN buffer (if you ignore
    the check GCC can't realistically evaluate at compile time).
    
    Switch to snprintf to populate filename to ensure that future logic
    errors don't result in a stack overflow.
    
    Shorten the questionably named yyyymmdd buffer enough to slience the
    warning (checking the snprintf return value isn't sufficent) while
    preserving maximum flexibility for admins who use the -F option.
    
    MFC after:      3 days
    Sponsored by:   Klara, Inc.
    Reviewed by:    brooks
    Differential Revision:  https://reviews.freebsd.org/D45086
---
 libexec/tftpd/tftpd.c | 43 +++++++++++++++++++++++++++++--------------
 1 file changed, 29 insertions(+), 14 deletions(-)

diff --git a/libexec/tftpd/tftpd.c b/libexec/tftpd/tftpd.c
index 80497738f60d..3f67ad2920cf 100644
--- a/libexec/tftpd/tftpd.c
+++ b/libexec/tftpd/tftpd.c
@@ -611,12 +611,20 @@ tftp_rrq(int peer, char *recvbuffer, size_t size)
 static int
 find_next_name(char *filename, int *fd)
 {
-	int i;
+	/*
+	 * GCC "knows" that we might write all of yyyymmdd plus the static
+	 * elemenents in the format into into newname and thus complains
+	 * unless we reduce the size.  This array is still too big, but since
+	 * the format is user supplied, it's not clear what a better limit
+	 * value would be and this is sufficent to silence the warnings.
+	 */
+	static const int suffix_len = strlen("..00");
+	char yyyymmdd[MAXPATHLEN - suffix_len];
+	char newname[MAXPATHLEN];
+	int i, ret;
 	time_t tval;
-	size_t len;
+	size_t len, namelen;
 	struct tm lt;
-	char yyyymmdd[MAXPATHLEN];
-	char newname[MAXPATHLEN];
 
 	/* Create the YYYYMMDD part of the filename */
 	time(&tval);
@@ -624,26 +632,33 @@ find_next_name(char *filename, int *fd)
 	len = strftime(yyyymmdd, sizeof(yyyymmdd), newfile_format, &lt);
 	if (len == 0) {
 		syslog(LOG_WARNING,
-			"Filename suffix too long (%d characters maximum)",
-			MAXPATHLEN);
+			"Filename suffix too long (%zu characters maximum)",
+			sizeof(yyyymmdd) - 1);
 		return (EACCESS);
 	}
 
 	/* Make sure the new filename is not too long */
-	if (strlen(filename) > MAXPATHLEN - len - 5) {
+	namelen = strlen(filename);
+	if (namelen >= sizeof(newname) - len - suffix_len) {
 		syslog(LOG_WARNING,
-			"Filename too long (%zd characters, %zd maximum)",
-			strlen(filename), MAXPATHLEN - len - 5);
+			"Filename too long (%zu characters, %zu maximum)",
+			namelen,
+			sizeof(newname) - len - suffix_len - 1);
 		return (EACCESS);
 	}
 
 	/* Find the first file which doesn't exist */
 	for (i = 0; i < 100; i++) {
-		sprintf(newname, "%s.%s.%02d", filename, yyyymmdd, i);
-		*fd = open(newname,
-		    O_WRONLY | O_CREAT | O_EXCL,
-		    S_IRUSR | S_IWUSR | S_IRGRP |
-		    S_IWGRP | S_IROTH | S_IWOTH);
+		ret = snprintf(newname, sizeof(newname), "%s.%s.%02d",
+		    filename, yyyymmdd, i);
+		/*
+		 * Size checked above so this can't happen, we'd use a
+		 * (void) cast, but gcc intentionally ignores that if
+		 * snprintf has __attribute__((warn_unused_result)).
+		 */
+		if (ret < 0 || (size_t)ret >= sizeof(newname))
+			__unreachable();
+		*fd = open(newname, O_WRONLY | O_CREAT | O_EXCL, 0666);
 		if (*fd > 0)
 			return 0;
 	}