[tz] ALL_STATE not "all" enough

Paul Eggert eggert at cs.ucla.edu
Sun Jun 15 06:45:24 UTC 2014


enh wrote:
> we'll fix this in Android (the first hunk in
> https://android-review.googlesource.com/#/c/94653/) but it would be nice if
> this were fixed upstream too so we don't have to diverge more than we
> already have.

Thanks for mentioning this.  I installed the attached patch into the 
experimental repository on github.  This should fix the problem you 
mentioned, along with a couple of other problems of the same ilk.  This 
patch tries to things a bit so more-efficiently than what Android does, 
by calling malloc at most once per function.

This patch assumes the buffer-overrun patch that I recently posted to 
the list.
-------------- next part --------------
diff --git a/NEWS b/NEWS
index ee48de9..442f8c9 100644
--- a/NEWS
+++ b/NEWS
@@ -11,6 +11,9 @@ Unreleased, experimental changes
 
     A stack-overrun bug has been fixed in 'mktime' and similar functions.
 
+    'localtime', 'mktime', etc. now use much less stack space if ALL_STATE
+    is defined.  (Thanks to Elliott Hughes for reporting the problem.)
+
   Changes affecting commentary
 
     Commentary now uses UTF-8 instead of US-ASCII, allowing the use of
diff --git a/localtime.c b/localtime.c
index 5eecdc1..b45ed0d 100644
--- a/localtime.c
+++ b/localtime.c
@@ -335,22 +335,7 @@ tzload(register const char *name, register struct state *const sp,
 					2 * sizeof *sp +
 					4 * TZ_MAX_TIMES];
 	} u_t;
-#ifdef ALL_STATE
-	register u_t * const		up = malloc(sizeof *up);
-#else /* !defined ALL_STATE */
-	u_t				u;
-	register u_t * const		up = &u;
-#endif /* !defined ALL_STATE */
-
-	sp->goback = sp->goahead = FALSE;
-
-	if (up == NULL)
-		return -1;
-
-	if (name == NULL && (name = TZDEFAULT) == NULL)
-		goto oops;
-	{
-		register int	doaccess;
+	union local_storage {
 		/*
 		** Section 4.9.1 of the C standard says that
 		** "FILENAME_MAX expands to an integral constant expression
@@ -360,29 +345,56 @@ tzload(register const char *name, register struct state *const sp,
 		*/
 		char		fullname[FILENAME_MAX + 1];
 
-		if (name[0] == ':')
-			++name;
-		doaccess = name[0] == '/';
-		if (!doaccess) {
-			if ((p = TZDIR) == NULL)
-				goto oops;
-			if ((strlen(p) + strlen(name) + 1) >= sizeof fullname)
-				goto oops;
-			(void) strcpy(fullname, p);
-			(void) strcat(fullname, "/");
-			(void) strcat(fullname, name);
-			/*
-			** Set doaccess if '.' (as in "../") shows up in name.
-			*/
-			if (strchr(name, '.') != NULL)
-				doaccess = TRUE;
-			name = fullname;
-		}
-		if (doaccess && access(name, R_OK) != 0)
+		/* The main part of the storage for this function.  */
+		struct {
+			u_t u;
+			struct state st;
+		} u;
+	};
+	register char *fullname;
+	register u_t *up;
+	register int doaccess;
+	register union local_storage *lsp;
+#ifdef ALL_STATE
+	lsp = malloc(sizeof *lsp);
+	if (!lsp)
+		return -1;
+#else /* !defined ALL_STATE */
+	union local_storage ls;
+	lsp = &ls;
+#endif /* !defined ALL_STATE */
+	fullname = lsp->fullname;
+	up = &lsp->u.u;
+
+	sp->goback = sp->goahead = FALSE;
+
+	if (! name) {
+		name = TZDEFAULT;
+		if (! name)
 			goto oops;
-		if ((fid = open(name, OPEN_MODE)) == -1)
+	}
+
+	if (name[0] == ':')
+		++name;
+	doaccess = name[0] == '/';
+	if (!doaccess) {
+		p = TZDIR;
+		if (! p || sizeof lsp->fullname - 1 <= strlen(p) + strlen(name))
 			goto oops;
+		strcpy(fullname, p);
+		strcat(fullname, "/");
+		strcat(fullname, name);
+		/* Set doaccess if '.' (as in "../") shows up in name.  */
+		if (strchr(name, '.'))
+			doaccess = TRUE;
+		name = fullname;
 	}
+	if (doaccess && access(name, R_OK) != 0)
+		goto oops;
+	fid = open(name, OPEN_MODE);
+	if (fid < 0)
+		goto oops;
+
 	nread = read(fid, up->buf, sizeof up->buf);
 	if (close(fid) < 0 || nread <= 0)
 		goto oops;
@@ -515,36 +527,36 @@ tzload(register const char *name, register struct state *const sp,
 	if (doextend && nread > 2 &&
 		up->buf[0] == '\n' && up->buf[nread - 1] == '\n' &&
 		sp->typecnt + 2 <= TZ_MAX_TYPES) {
-			struct state	ts;
+			struct state	*ts = &lsp->u.st;
 			register int	result;
 
 			up->buf[nread - 1] = '\0';
-			result = tzparse(&up->buf[1], &ts, FALSE);
-			if (result == 0 && ts.typecnt == 2 &&
-				sp->charcnt + ts.charcnt <= TZ_MAX_CHARS) {
+			result = tzparse(&up->buf[1], ts, FALSE);
+			if (result == 0 && ts->typecnt == 2 &&
+				sp->charcnt + ts->charcnt <= TZ_MAX_CHARS) {
 					for (i = 0; i < 2; ++i)
-						ts.ttis[i].tt_abbrind +=
+						ts->ttis[i].tt_abbrind +=
 							sp->charcnt;
-					for (i = 0; i < ts.charcnt; ++i)
+					for (i = 0; i < ts->charcnt; ++i)
 						sp->chars[sp->charcnt++] =
-							ts.chars[i];
+							ts->chars[i];
 					i = 0;
-					while (i < ts.timecnt &&
-						ts.ats[i] <=
+					while (i < ts->timecnt &&
+						ts->ats[i] <=
 						sp->ats[sp->timecnt - 1])
 							++i;
-					while (i < ts.timecnt &&
+					while (i < ts->timecnt &&
 					    sp->timecnt < TZ_MAX_TIMES) {
 						sp->ats[sp->timecnt] =
-							ts.ats[i];
+							ts->ats[i];
 						sp->types[sp->timecnt] =
 							sp->typecnt +
-							ts.types[i];
+							ts->types[i];
 						++sp->timecnt;
 						++i;
 					}
-					sp->ttis[sp->typecnt++] = ts.ttis[0];
-					sp->ttis[sp->typecnt++] = ts.ttis[1];
+					sp->ttis[sp->typecnt++] = ts->ttis[0];
+					sp->ttis[sp->typecnt++] = ts->ttis[1];
 			}
 	}
 	if (sp->timecnt > 1) {
@@ -1867,8 +1879,8 @@ time1(struct tm *const tmp,
 	register int			sameind, otherind;
 	register int			i;
 	register int			nseen;
-	int				seen[TZ_MAX_TYPES];
-	int				types[TZ_MAX_TIMES];
+	char				seen[TZ_MAX_TYPES];
+	unsigned char			types[TZ_MAX_TIMES];
 	int				okay;
 
 	if (tmp == NULL) {


More information about the tz mailing list