FW: stack overflow in tzload
I'm forwarding this message from Ted Unangst, who is not on the time zone mailing list. Those of you who are on the list, please direct replies appropriately. --ado -----Original Message----- From: Ted Unangst [mailto:ted.unangst@gmail.com] Sent: Monday, November 22, 2010 6:22 To: tz@lecserver.nci.nih.gov Subject: stack overflow in tzload Stack overflow as in there's not enough stack. I ran into this problem running a program on openbsd that was using a fairly small thread size. I switched to using malloc. Index: localtime.c =================================================================== RCS file: /home/tedu/cvs/src/lib/libc/time/localtime.c,v retrieving revision 1.35 diff -u -r1.35 localtime.c --- localtime.c 23 Aug 2010 22:35:34 -0000 1.35 +++ localtime.c 20 Nov 2010 23:32:09 -0000 @@ -340,7 +340,7 @@ char buf[2 * sizeof(struct tzhead) + 2 * sizeof *sp + 4 * TZ_MAX_TIMES]; - } u; + } *u; sp->goback = sp->goahead = FALSE; if (name != NULL && issetugid() != 0) @@ -383,28 +383,33 @@ if ((fid = open(name, OPEN_MODE)) == -1) return -1; } - nread = read(fid, u.buf, sizeof u.buf); - if (close(fid) < 0 || nread <= 0) + u = malloc(sizeof(*u)); + if (!u) { + close(fid); return -1; + } + nread = read(fid, u->buf, sizeof u->buf); + if (close(fid) < 0 || nread <= 0) + goto bad; 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 bad; + if (nread - (p - u->buf) < sp->timecnt * stored + /* ats */ sp->timecnt + /* types */ sp->typecnt * 6 + /* ttinfos */ @@ -412,7 +417,7 @@ sp->leapcnt * (stored + 4) + /* lsinfos */ ttisstdcnt + /* ttisstds */ ttisgmtcnt) /* ttisgmts */ - return -1; + goto bad; for (i = 0; i < sp->timecnt; ++i) { sp->ats[i] = (stored == 4) ? detzcode(p) : detzcode64(p); @@ -421,7 +426,7 @@ for (i = 0; i < sp->timecnt; ++i) { sp->types[i] = (unsigned char) *p++; if (sp->types[i] >= sp->typecnt) - return -1; + goto bad; } for (i = 0; i < sp->typecnt; ++i) { register struct ttinfo * ttisp; @@ -431,11 +436,11 @@ p += 4; ttisp->tt_isdst = (unsigned char) *p++; if (ttisp->tt_isdst != 0 && ttisp->tt_isdst != 1) - return -1; + goto bad; ttisp->tt_abbrind = (unsigned char) *p++; if (ttisp->tt_abbrind < 0 || ttisp->tt_abbrind > sp->charcnt) - return -1; + goto bad; } for (i = 0; i < sp->charcnt; ++i) sp->chars[i] = *p++; @@ -460,7 +465,7 @@ ttisp->tt_ttisstd = *p++; if (ttisp->tt_ttisstd != TRUE && ttisp->tt_ttisstd != FALSE) - return -1; + goto bad; } } for (i = 0; i < sp->typecnt; ++i) { @@ -473,7 +478,7 @@ ttisp->tt_ttisgmt = *p++; if (ttisp->tt_ttisgmt != TRUE && ttisp->tt_ttisgmt != FALSE) - return -1; + goto bad; } } /* @@ -506,11 +511,11 @@ /* ** 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. */ @@ -518,13 +523,13 @@ 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; register int result; - u.buf[nread - 1] = '\0'; - result = tzparse(&u.buf[1], &ts, FALSE); + 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) @@ -568,7 +573,11 @@ break; } } + free(u); return 0; +bad: + free(u); + return -1; } static int
Surely this sort of change should be put in only conditionally. On most machines, the change would will slow performance down due to the malloc overhead and would add one more point of failure. Only on machines with tiny stacks is the change a win.
On Nov 22, 2010, at 4:44 PM, Paul Eggert wrote:
Surely this sort of change should be put in only conditionally. On most machines, the change would will slow performance down due to the malloc overhead and would add one more point of failure. Only on machines with tiny stacks is the change a win.
s/machines/environments/ The original report said "I ran into this problem running a program on openbsd that was using a fairly small thread size." Presumably some piece of code explicitly requested that thread size by setting the stack size before creating the pthread or got the default stack size for the OS by *not* setting the stack size. I.e., this isn't just a question of a particular machine or even a particular OS; the best we could do at compile time would be to base it on the default pthread stack size on the platform. (If the *default* pthread stack size is big enough, we shouldn't do anything - whoever's requesting the smaller stack size shouldn't do so.)
On Nov 22, 4:44pm, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: FW: stack overflow in tzload | Surely this sort of change should be put in only conditionally. | On most machines, the change would will slow performance down due to the | malloc overhead and would add one more point of failure. | Only on machines with tiny stacks is the change a win. Sure, but these days the cost of malloc is really small, and having more ifdefs in the code just cause more maintenance. So we either adopt the change, or we just tell Ted that if you run with < 256K of stack you can expect lots of things to break - not just that - so give yourself more stack. Having said that I am in the malloc camp (and we should unif the code -DALL_STATE). christos
On Mon, Nov 22, 2010 at 11:28 PM, Christos Zoulas <christos@zoulas.com> wrote:
On Nov 22, 4:44pm, eggert@cs.ucla.edu (Paul Eggert) wrote: -- Subject: Re: FW: stack overflow in tzload
| Surely this sort of change should be put in only conditionally. | On most machines, the change would will slow performance down due to the | malloc overhead and would add one more point of failure. | Only on machines with tiny stacks is the change a win.
Sure, but these days the cost of malloc is really small, and having more ifdefs in the code just cause more maintenance. So we either adopt the change, or we just tell Ted that if you run with < 256K of stack you can expect lots of things to break - not just that - so give yourself more
All fair points, but it happens that this is the thing that broke. Smallish stacks are not uncommon if you expect to be creating many threads (the app in question is a of web server not of my own design, I'm not even sure what stack size it expects). The real wrinkle is that the tz code is part of the system library, I expect it to just work. If somebody had asked last week "How much stack does calling gmtime() require?", I suspect most people's answers would fall short of the mark, apart from maybe a very few people on this list. We can just patch this in openbsd, it's not that big a deal, but wanted to pass the change upstream as well.
what si about using alloc() ? System that do not provied it can use malloc, others will use the stack but we will take the stack only when needed and get an error (hopefully) when oom. re, wh Am 23.11.2010 00:56, schrieb Olson, Arthur David (NIH/NCI) [E]:
I'm forwarding this message from Ted Unangst, who is not on the time zone mailing list. Those of you who are on the list, please direct replies appropriately.
--ado
-----Original Message----- From: Ted Unangst [mailto:ted.unangst@gmail.com] Sent: Monday, November 22, 2010 6:22 To: tz@lecserver.nci.nih.gov Subject: stack overflow in tzload
Stack overflow as in there's not enough stack. I ran into this problem running a program on openbsd that was using a fairly small thread size. I switched to using malloc.
Index: localtime.c =================================================================== RCS file: /home/tedu/cvs/src/lib/libc/time/localtime.c,v retrieving revision 1.35 diff -u -r1.35 localtime.c --- localtime.c 23 Aug 2010 22:35:34 -0000 1.35 +++ localtime.c 20 Nov 2010 23:32:09 -0000 @@ -340,7 +340,7 @@ char buf[2 * sizeof(struct tzhead) + 2 * sizeof *sp + 4 * TZ_MAX_TIMES]; - } u; + } *u;
sp->goback = sp->goahead = FALSE; if (name != NULL && issetugid() != 0) @@ -383,28 +383,33 @@ if ((fid = open(name, OPEN_MODE)) == -1) return -1; } - nread = read(fid, u.buf, sizeof u.buf); - if (close(fid) < 0 || nread <= 0) + u = malloc(sizeof(*u)); + if (!u) { + close(fid); return -1; + } + nread = read(fid, u->buf, sizeof u->buf); + if (close(fid) < 0 || nread <= 0) + goto bad; 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 bad; + if (nread - (p - u->buf) < sp->timecnt * stored + /* ats */ sp->timecnt + /* types */ sp->typecnt * 6 + /* ttinfos */ @@ -412,7 +417,7 @@ sp->leapcnt * (stored + 4) + /* lsinfos */ ttisstdcnt + /* ttisstds */ ttisgmtcnt) /* ttisgmts */ - return -1; + goto bad; for (i = 0; i < sp->timecnt; ++i) { sp->ats[i] = (stored == 4) ? detzcode(p) : detzcode64(p); @@ -421,7 +426,7 @@ for (i = 0; i < sp->timecnt; ++i) { sp->types[i] = (unsigned char) *p++; if (sp->types[i] >= sp->typecnt) - return -1; + goto bad; } for (i = 0; i < sp->typecnt; ++i) { register struct ttinfo * ttisp; @@ -431,11 +436,11 @@ p += 4; ttisp->tt_isdst = (unsigned char) *p++; if (ttisp->tt_isdst != 0 && ttisp->tt_isdst != 1) - return -1; + goto bad; ttisp->tt_abbrind = (unsigned char) *p++; if (ttisp->tt_abbrind < 0 || ttisp->tt_abbrind > sp->charcnt) - return -1; + goto bad; } for (i = 0; i < sp->charcnt; ++i) sp->chars[i] = *p++; @@ -460,7 +465,7 @@ ttisp->tt_ttisstd = *p++; if (ttisp->tt_ttisstd != TRUE && ttisp->tt_ttisstd != FALSE) - return -1; + goto bad; } } for (i = 0; i < sp->typecnt; ++i) { @@ -473,7 +478,7 @@ ttisp->tt_ttisgmt = *p++; if (ttisp->tt_ttisgmt != TRUE && ttisp->tt_ttisgmt != FALSE) - return -1; + goto bad; } } /* @@ -506,11 +511,11 @@ /* ** 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. */ @@ -518,13 +523,13 @@ 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; register int result;
- u.buf[nread - 1] = '\0'; - result = tzparse(&u.buf[1], &ts, FALSE); + 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) @@ -568,7 +573,11 @@ break; } } + free(u); return 0; +bad: + free(u); + return -1; }
static int
participants (7)
-
christos＠zoulas.com -
Guy Harris -
Ian Abbott -
Olson, Arthur David (NIH/NCI) [E] -
Paul Eggert -
Ted Unangst -
walter harms