kleink@netbsd.org wrote:
> Synopsis: Off-by-one error in libc/time/localtime.c:tzload()
> 
> State-Changed-From-To: suspended->closed
> State-Changed-By: kleink@netbsd.org
> State-Changed-When: Sun, 09 Oct 2005 11:04:43 +0000
> State-Changed-Why:
> Since this doesn't constitute an overrun possibility, and since it
> gathered no interest at tz@elsie, I'm closing the issue without
> code change - one hunk less needing merging on further imports.

The assumption of an off-by-one issue was based on my misunderstanding
the passing filenames longer than (FILENAME_MAX - 1) might provoke
undefined behaviour. However, (FILENAME_MAX - 1) is only the minimum
filename length that can be opened. So passing longer names is not
an issue at all and open() etc. might very well succeed for these
cases even.

Anyway for the record, I believe the following variant would be
much clearer and it's exactly what the manpage of strlcpy()
suggests.


Index: localtime.c
===================================================================
RCS file: /cvsroot/src/lib/libc/time/localtime.c,v
retrieving revision 1.37
diff -u -p -r1.37 localtime.c
--- localtime.c	16 Jul 2005 19:48:09 -0000	1.37
+++ localtime.c	9 Oct 2005 13:00:27 -0000
@@ -328,7 +328,7 @@ register struct state * const	sp;
 		** to hold the longest file name string that the implementation
 		** guarantees can be opened."
 		*/
-		char		fullname[FILENAME_MAX + 1];
+		char		fullname[FILENAME_MAX];
 
 		if (name[0] == ':')
 			++name;
@@ -336,11 +336,19 @@ register struct state * const	sp;
 		if (!doaccess) {
 			if ((p = TZDIR) == NULL)
 				return -1;
-			if ((strlen(p) + strlen(name) + 1) >= sizeof fullname)
+
+			if (strlcpy(fullname, p, sizeof fullname)
+				>= sizeof fullname)
+				return -1;
+
+			if (strlcat(fullname, "/", sizeof fullname)
+				>= sizeof fullname)
 				return -1;
-			(void) strcpy(fullname, p);	/* XXX strcpy is safe */
-			(void) strcat(fullname, "/");	/* XXX strcat is safe */
-			(void) strcat(fullname, name);	/* XXX strcat is safe */
+
+			if (strlcat(fullname, name, sizeof fullname)
+				>= sizeof fullname)
+				return -1;
+
 			/*
 			** Set doaccess if '.' (as in "../") shows up in name.
 			*/

-- 
Christian