Re: Segfault in libthr.so on 9.0-BETA2 (with stunnel FWIW)

From: Kostik Belousov <kostikbel_at_gmail.com>
Date: Sun, 18 Sep 2011 18:56:49 +0300
On Sun, Sep 18, 2011 at 01:56:50PM +0200, Jilles Tjoelker wrote:
> On Wed, Sep 14, 2011 at 11:04:56PM +0300, Kostik Belousov wrote:
> > tzload() allocates ~80KB for the local variables. The backtrace you provided
> > shows the nested call to tzload(), so there is total 160KB of the stack
> > space consumed.
> 
> > By default, stack for the amd64 thread is 4MB, that should be plenty. This
> > is not the case for ezm3. Possibly, stunnel also reduces the size of the
> > thread stack.
> 
> > Please, try the patch below. I did not tested it, only compiled. I see
> > that now tzload allocates only ~300 bytes on the stack.
> 
> 80KB seems quite a lot indeed, good to bring it down.
> 
> > diff --git a/contrib/tzcode/stdtime/localtime.c b/contrib/tzcode/stdtime/localtime.c
> > index 80b70ac..55d55e0 100644
> > --- a/contrib/tzcode/stdtime/localtime.c
> > +++ b/contrib/tzcode/stdtime/localtime.c
> [snip]
> > _at__at_ -406,16 +409,24 _at__at_ register const int	doextend;
> >  		** to hold the longest file name string that the implementation
> >  		** guarantees can be opened."
> >  		*/
> > -		char		fullname[FILENAME_MAX + 1];
> > +		char		*fullname;
> > +
> > +		fullname = malloc(FILENAME_MAX + 1);
> > +		if (fullname == NULL)
> > +			goto out;
> >  
> >  		if (name[0] == ':')
> >  			++name;
> >  		doaccess = name[0] == '/';
> >  		if (!doaccess) {
> > -			if ((p = TZDIR) == NULL)
> > +			if ((p = TZDIR) == NULL) {
> > +				free(fullname);
> >  				return -1;
> > -			if ((strlen(p) + 1 + strlen(name) + 1) >= sizeof fullname)
> > +			}
> > +			if ((strlen(p) + 1 + strlen(name) + 1) >= sizeof fullname) {
> 
> This sizeof is now the sizeof of a pointer. The comparison should be
> against FILENAME_MAX + 1 instead.
> 
> Alternatively, the name could be created using asprintf().

You are right. I fixed the defect.
Updated patch below.

diff --git a/contrib/tzcode/stdtime/localtime.c b/contrib/tzcode/stdtime/localtime.c
index 80b70ac..b1981b6 100644
--- a/contrib/tzcode/stdtime/localtime.c
+++ b/contrib/tzcode/stdtime/localtime.c
_at__at_ -380,13 +380,16 _at__at_ register const int	doextend;
 	int		fid;
 	int		stored;
 	int		nread;
+	int		res;
 	union {
 		struct tzhead	tzhead;
 		char		buf[2 * sizeof(struct tzhead) +
 					2 * sizeof *sp +
 					4 * TZ_MAX_TIMES];
-	} u;
+	} *u;
 
+	u = NULL;
+	res = -1;
 	sp->goback = sp->goahead = FALSE;
 
 	/* XXX The following is from OpenBSD, and I'm not sure it is correct */
_at__at_ -406,16 +409,24 _at__at_ register const int	doextend;
 		** to hold the longest file name string that the implementation
 		** guarantees can be opened."
 		*/
-		char		fullname[FILENAME_MAX + 1];
+		char		*fullname;
+
+		fullname = malloc(FILENAME_MAX + 1);
+		if (fullname == NULL)
+			goto out;
 
 		if (name[0] == ':')
 			++name;
 		doaccess = name[0] == '/';
 		if (!doaccess) {
-			if ((p = TZDIR) == NULL)
+			if ((p = TZDIR) == NULL) {
+				free(fullname);
 				return -1;
-			if ((strlen(p) + 1 + strlen(name) + 1) >= sizeof fullname)
+			}
+			if (strlen(p) + 1 + strlen(name) >= FILENAME_MAX) {
+				free(fullname);
 				return -1;
+			}
 			(void) strcpy(fullname, p);
 			(void) strcat(fullname, "/");
 			(void) strcat(fullname, name);
_at__at_ -426,37 +437,45 _at__at_ register const int	doextend;
 				doaccess = TRUE;
 			name = fullname;
 		}
-		if (doaccess && access(name, R_OK) != 0)
+		if (doaccess && access(name, R_OK) != 0) {
+			free(fullname);
 		     	return -1;
-		if ((fid = _open(name, OPEN_MODE)) == -1)
+		}
+		if ((fid = _open(name, OPEN_MODE)) == -1) {
+			free(fullname);
 			return -1;
+		}
 		if ((_fstat(fid, &stab) < 0) || !S_ISREG(stab.st_mode)) {
+			free(fullname);
 			_close(fid);
 			return -1;
 		}
 	}
-	nread = _read(fid, u.buf, sizeof u.buf);
+	u = malloc(sizeof(*u));
+	if (u == NULL)
+		goto out;
+	nread = _read(fid, u->buf, sizeof u->buf);
 	if (_close(fid) < 0 || nread <= 0)
-		return -1;
+		goto out;
 	for (stored = 4; stored <= 8; stored *= 2) {
 		int		ttisstdcnt;
 		int		ttisgmtcnt;
 
-		ttisstdcnt = (int) detzcode(u.tzhead.tzh_ttisstdcnt);
-		ttisgmtcnt = (int) detzcode(u.tzhead.tzh_ttisgmtcnt);
-		sp->leapcnt = (int) detzcode(u.tzhead.tzh_leapcnt);
-		sp->timecnt = (int) detzcode(u.tzhead.tzh_timecnt);
-		sp->typecnt = (int) detzcode(u.tzhead.tzh_typecnt);
-		sp->charcnt = (int) detzcode(u.tzhead.tzh_charcnt);
-		p = u.tzhead.tzh_charcnt + sizeof u.tzhead.tzh_charcnt;
+		ttisstdcnt = (int) detzcode(u->tzhead.tzh_ttisstdcnt);
+		ttisgmtcnt = (int) detzcode(u->tzhead.tzh_ttisgmtcnt);
+		sp->leapcnt = (int) detzcode(u->tzhead.tzh_leapcnt);
+		sp->timecnt = (int) detzcode(u->tzhead.tzh_timecnt);
+		sp->typecnt = (int) detzcode(u->tzhead.tzh_typecnt);
+		sp->charcnt = (int) detzcode(u->tzhead.tzh_charcnt);
+		p = u->tzhead.tzh_charcnt + sizeof u->tzhead.tzh_charcnt;
 		if (sp->leapcnt < 0 || sp->leapcnt > TZ_MAX_LEAPS ||
 			sp->typecnt <= 0 || sp->typecnt > TZ_MAX_TYPES ||
 			sp->timecnt < 0 || sp->timecnt > TZ_MAX_TIMES ||
 			sp->charcnt < 0 || sp->charcnt > TZ_MAX_CHARS ||
 			(ttisstdcnt != sp->typecnt && ttisstdcnt != 0) ||
 			(ttisgmtcnt != sp->typecnt && ttisgmtcnt != 0))
-				return -1;
-		if (nread - (p - u.buf) <
+				goto out;
+		if (nread - (p - u->buf) <
 			sp->timecnt * stored +		/* ats */
 			sp->timecnt +			/* types */
 			sp->typecnt * 6 +		/* ttinfos */
_at__at_ -464,7 +483,7 _at__at_ register const int	doextend;
 			sp->leapcnt * (stored + 4) +	/* lsinfos */
 			ttisstdcnt +			/* ttisstds */
 			ttisgmtcnt)			/* ttisgmts */
-				return -1;
+				goto out;
 		for (i = 0; i < sp->timecnt; ++i) {
 			sp->ats[i] = (stored == 4) ?
 				detzcode(p) : detzcode64(p);
_at__at_ -473,7 +492,7 _at__at_ register const int	doextend;
 		for (i = 0; i < sp->timecnt; ++i) {
 			sp->types[i] = (unsigned char) *p++;
 			if (sp->types[i] >= sp->typecnt)
-				return -1;
+				goto out;
 		}
 		for (i = 0; i < sp->typecnt; ++i) {
 			struct ttinfo *	ttisp;
_at__at_ -483,11 +502,11 _at__at_ register const int	doextend;
 			p += 4;
 			ttisp->tt_isdst = (unsigned char) *p++;
 			if (ttisp->tt_isdst != 0 && ttisp->tt_isdst != 1)
-				return -1;
+				goto out;
 			ttisp->tt_abbrind = (unsigned char) *p++;
 			if (ttisp->tt_abbrind < 0 ||
 				ttisp->tt_abbrind > sp->charcnt)
-					return -1;
+					goto out;
 		}
 		for (i = 0; i < sp->charcnt; ++i)
 			sp->chars[i] = *p++;
_at__at_ -512,7 +531,7 _at__at_ register const int	doextend;
 				ttisp->tt_ttisstd = *p++;
 				if (ttisp->tt_ttisstd != TRUE &&
 					ttisp->tt_ttisstd != FALSE)
-						return -1;
+						goto out;
 			}
 		}
 		for (i = 0; i < sp->typecnt; ++i) {
_at__at_ -525,7 +544,7 _at__at_ register const int	doextend;
 				ttisp->tt_ttisgmt = *p++;
 				if (ttisp->tt_ttisgmt != TRUE &&
 					ttisp->tt_ttisgmt != FALSE)
-						return -1;
+						goto out;
 			}
 		}
 		/*
_at__at_ -558,11 +577,11 _at__at_ register const int	doextend;
 		/*
 		** If this is an old file, we're done.
 		*/
-		if (u.tzhead.tzh_version[0] == '\0')
+		if (u->tzhead.tzh_version[0] == '\0')
 			break;
-		nread -= p - u.buf;
+		nread -= p - u->buf;
 		for (i = 0; i < nread; ++i)
-			u.buf[i] = p[i];
+			u->buf[i] = p[i];
 		/*
 		** If this is a narrow integer time_t system, we're done.
 		*/
_at__at_ -570,39 +589,43 _at__at_ register const int	doextend;
 			break;
 	}
 	if (doextend && nread > 2 &&
-		u.buf[0] == '\n' && u.buf[nread - 1] == '\n' &&
+		u->buf[0] == '\n' && u->buf[nread - 1] == '\n' &&
 		sp->typecnt + 2 <= TZ_MAX_TYPES) {
-			struct state	ts;
+			struct state	*ts;
 			register int	result;
 
-			u.buf[nread - 1] = '\0';
-			result = tzparse(&u.buf[1], &ts, FALSE);
-			if (result == 0 && ts.typecnt == 2 &&
-				sp->charcnt + ts.charcnt <= TZ_MAX_CHARS) {
+			ts = malloc(sizeof(*ts));
+			if (ts == NULL)
+				goto out;
+			u->buf[nread - 1] = '\0';
+			result = tzparse(&u->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];
 			}
+			free(ts);
 	}
 	if (sp->timecnt > 1) {
 		for (i = 1; i < sp->timecnt; ++i)
_at__at_ -620,7 +643,10 _at__at_ register const int	doextend;
 					break;
 		}
 	}
-	return 0;
+	res = 0;
+out:
+	free(u);
+	return (res);
 }
 
 static int

Received on Sun Sep 18 2011 - 13:57:34 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:18 UTC