2.14 KB, patch
|Details | Diff | Splinter Review|
1.31 KB, patch
Alexander Sack: approval1.8.0.next+
|Details | Diff | Splinter Review|
Loading this URL in a Win32 trunk build crashes reliably in nsDocument::RetrieveRelevantHeaders. It's a buffer overrun that gets caught by the VC8 CRT. The overrun is a sprintf of a date, the PRExplodedTime .tm_year is 19107. Looks like we're probably getting a bogus date value from the server, I haven't looked into it further.
This is old code (circa 2005), why is it breaking now? Firefox 2 and a trunk from a couple months back don't crash (but current trunk does), I'll try to narrow down a regression range. Regardless of what might have changed elsewher, we should absolutely NOT be using sprintf here or anywhere. If PR_FormatTime() doesn't work for some reason we should use (PR_)snprintf.
2007100705 doesn't crash, 2007101405 does. Sorry for the big range but it's taking me 4 minutes a pop to download these with my current flaky connection.
crash was introduced between 10/11 and 10/12: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-11+&maxdate=2007-10-12+04%3A00&cvsroot=%2Fcvsroot No doubt it's the one-line NSPR tag bump hiding a bunch of changes, one of which is in prtime.c to fix bug 239371 (support 5-digit years). In this case the server is sending a "last-modified" time of "Fri, 2 Nov 19107 00:00:01 GMT". [haven't seen years like that since we were fixing y2k bugs] We can't fault NSPR here but we can certainly worry about where else we're assuming years fit in four digits
Created attachment 287076 [details] [diff] [review] patch v1
Did we pull that NSPR change onto the 1.8 branch? We wouldn't see a crash there, since this is the VC8 CRT checking stack overflow.
Doesn't happen on Fx2008, we get "2007" in the .tm_year.
Comment on attachment 287076 [details] [diff] [review] patch v1 r=wtc. I would indent the format string and arguments for PR_snprintf to align with the open parenthesis. We have the option of resolving NSPR bug 239371 WONTFIX. The bug fix to parse five-digit years hasn't appeared in any NSPR final release yet. Another option is for the callers of PR_ParseTimeString to validate that the parsed time is within a reasonable range. In any case, changing sprintf to PR_snprintf is a good change.
Created attachment 287128 [details] [diff] [review] mochitest This is a dead simple mochitest that just sends the same Last-Modified header as the URL, and has a no-op test inside the document to ensure that the document loaded without crashing. Browser crashes for me on stock trunk, passes with dveditz' patch.
Comment on attachment 287076 [details] [diff] [review] patch v1 dveditz, for PR_snprintf, the format string should ideally be: "%02ld/%02ld/%04hd %02ld:%02ld:%02ld" NSPR uses ld for PRInt32 and hd for PRInt16.
Comment on attachment 287076 [details] [diff] [review] patch v1 r+sr=jst
In comment #3 dveditz asked > We can't fault NSPR here but we can certainly worry about where else we're > assuming years fit in four digits I looked at all occurrences of "tm_year" in the SeaMonkey LXR: http://lxr.mozilla.org/seamonkey/search?string=tm_year I examined all direct uses of tm_year. I didn't examine some of the data whose values come from tm_year. I found only two other potential problems. In both cases we pass tm_year to sprintf: http://lxr.mozilla.org/seamonkey/source/calendar/libical/src/test/regression-utils.c#14 http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/ParseFTPList.cpp#1711 In the first case, the buffer is 1024-byte long. So buffer overflow is impossible. The second case is in test code (function do_it) that is commented out. It's not easy to see what the buffer size is, but this is in dead code.
Created attachment 287556 [details] [diff] [review] update w/review comments Updated with review formatting suggestions, carrying over r/sr+.
Comment on attachment 287556 [details] [diff] [review] update w/review comments a=endgame drivers for M9
This didn't get landed yet? We probably shouldn't ship this in b1. :-/
Please land it immediately on GECKO190_20071106_RELBRANCH and then contact either me or cf in IRC, or email release-drivers@ We'll leave it as approved1.9+ as well for when the tree re-opens.
Comment on attachment 287556 [details] [diff] [review] update w/review comments a=beltzner for 1.9 once the tree re-opens
Comment on attachment 287556 [details] [diff] [review] update w/review comments After a quick IRC chat with dveditz, we've decided to leave this for after M9 so as to keep forward motion.
FIXED: attachment 287556 [details] [diff] [review] checked in
The Mochitest needs to be committed as well.
Mochitest checked in.
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007112905 Minefield/3.0b2pre. No crash when loading the URL in the bug.
Clearing branch wanted flag assuming the regressing change will not land in older NSPR versions.
Dan, it's a good idea to land this patch on the Mozilla 1.8 branch for two reasons. 1. The Mozilla 1.8 branch may upgrade to NSPR 4.7.x. 2. Linux distributions may use the Mozilla 1.8 branch with a system NSPR at version 4.7.x.
fair enough, it's certainly not going to hurt anything.
Comment on attachment 287556 [details] [diff] [review] update w/review comments approved for 220.127.116.11, a=dveditz for release-drivers
not really blocking, but approving the patch
Comment on attachment 287556 [details] [diff] [review] update w/review comments I checked in this patch on the MOZILLA_1_8_BRANCH for 18.104.22.168. Checking in nsDocument.cpp; /cvsroot/mozilla/content/base/src/nsDocument.cpp,v <-- nsDocument.cpp new revision: 3.566.2.37; previous revision: 3.566.2.36 done
I _cannot_ crash with the 22.214.171.124 Win32 builds (Vista/XP), using http://man.he.net/man2/lseek as the URL, so obviously can't verify this in the .12 candidates (comment 6 seems to indicate that branch builds didn't (wouldn't?) crash, since they get "2007"--rather than 19107--returned).
Even if the NSPR changes went into the 1.8 branch, you still wouldn't crash there, because the crash is actually triggered by the stack safety stuff in VC8. You'd just get silent stack corruption.
Stephen: you can verify that I checked in the patch on the MOZILLA_1_8_BRANCH correctly. (I already did that.)
Verified using Bonsai: http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsDocument.cpp&branch=MOZILLA_1_8_BRANCH&root=/cvsroot&subdir=mozilla/content/base/src&command=DIFF_FRAMESET&rev1=3.566.2.36&rev2=3.566.2.37, for MOZILLA_1_8_BRANCH. Replacing fixed126.96.36.199 with verified188.8.131.52.
distro patch blocks 184.108.40.206
Comment on attachment 287556 [details] [diff] [review] update w/review comments a=asac for 220.127.116.11 (unmodified distro patch)
MOZILLA_1_8_0_BRANCH: Checking in content/base/src/nsDocument.cpp; /cvsroot/mozilla/content/base/src/nsDocument.cpp,v <-- nsDocument.cpp new revision: 3.518.104.22.168.20; previous revision: 3.522.214.171.124.19 done