Last Comment Bug 402150 - Buffer overrun [@ nsDocument::RetrieveRelevantHeaders] at provided URL
: Buffer overrun [@ nsDocument::RetrieveRelevantHeaders] at provided URL
[sg:moderate] 1-byte overrun on trunk...
: crash, fixed1.8.0.15, regression, verified1.8.1.12
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Windows XP
: P1 critical (vote)
: mozilla1.9beta2
Assigned To: Daniel Veditz [:dveditz]
: Andrew Overholt [:overholt]
Depends on:
Blocks: 239371
  Show dependency treegraph
Reported: 2007-11-01 18:52 PDT by Ted Mielczarek [:ted.mielczarek]
Modified: 2011-06-13 10:01 PDT (History)
13 users (show)
jst: blocking1.9+
dveditz: wanted1.8.1.x+
ted: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1 (1.37 KB, patch)
2007-11-02 01:48 PDT, Daniel Veditz [:dveditz]
jst: review+
wtc: review+
jst: superreview+
Details | Diff | Splinter Review
mochitest (2.14 KB, patch)
2007-11-02 11:18 PDT, Ted Mielczarek [:ted.mielczarek]
jst: review+
Details | Diff | Splinter Review
update w/review comments (1.31 KB, patch)
2007-11-06 10:25 PST, Daniel Veditz [:dveditz]
dveditz: review+
dveditz: superreview+
dveditz: approval1.8.1.12+
mbeltzner: approvalM9-
mbeltzner: approval1.9+
Details | Diff | Splinter Review

Description Ted Mielczarek [:ted.mielczarek] 2007-11-01 18:52:36 PDT
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.
Comment 1 Daniel Veditz [:dveditz] 2007-11-01 21:00:59 PDT
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.
Comment 2 Daniel Veditz [:dveditz] 2007-11-01 21:29:34 PDT
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.
Comment 3 Daniel Veditz [:dveditz] 2007-11-01 23:09:01 PDT
crash was introduced between 10/11 and 10/12:

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
Comment 4 Daniel Veditz [:dveditz] 2007-11-02 01:48:12 PDT
Created attachment 287076 [details] [diff] [review]
patch v1
Comment 5 Ted Mielczarek [:ted.mielczarek] 2007-11-02 05:18:44 PDT
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.
Comment 6 Ted Mielczarek [:ted.mielczarek] 2007-11-02 06:06:52 PDT
Doesn't happen on Fx2008, we get "2007" in the .tm_year.
Comment 7 Wan-Teh Chang 2007-11-02 10:31:04 PDT
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.
Comment 8 Ted Mielczarek [:ted.mielczarek] 2007-11-02 11:18:17 PDT
Created attachment 287128 [details] [diff] [review]

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 9 Wan-Teh Chang 2007-11-02 14:17:00 PDT
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 10 Johnny Stenback (:jst, 2007-11-02 17:26:36 PDT
Comment on attachment 287076 [details] [diff] [review]
patch v1

Comment 11 Wan-Teh Chang 2007-11-03 16:13:30 PDT
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:

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:

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.
Comment 12 Daniel Veditz [:dveditz] 2007-11-06 10:25:41 PST
Created attachment 287556 [details] [diff] [review]
update w/review comments

Updated with review formatting suggestions, carrying over r/sr+.
Comment 13 Mike Beltzner [:beltzner, not reading bugmail] 2007-11-06 10:38:18 PST
Comment on attachment 287556 [details] [diff] [review]
update w/review comments

a=endgame drivers for M9
Comment 14 Vladimir Vukicevic [:vlad] [:vladv] 2007-11-06 10:38:29 PST
"formattedTime" :(
Comment 15 Ted Mielczarek [:ted.mielczarek] 2007-11-06 16:07:54 PST
This didn't get landed yet?  We probably shouldn't ship this in b1.  :-/
Comment 16 Mike Beltzner [:beltzner, not reading bugmail] 2007-11-06 16:22:37 PST
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 17 Mike Beltzner [:beltzner, not reading bugmail] 2007-11-06 16:23:07 PST
Comment on attachment 287556 [details] [diff] [review]
update w/review comments

a=beltzner for 1.9 once the tree re-opens
Comment 18 Mike Beltzner [:beltzner, not reading bugmail] 2007-11-06 16:28:40 PST
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.
Comment 19 Daniel Veditz [:dveditz] 2007-11-09 14:55:26 PST
FIXED: attachment 287556 [details] [diff] [review] checked in
Comment 20 Jeff Walden [:Waldo] (remove +bmo to email) 2007-11-10 17:45:14 PST
The Mochitest needs to be committed as well.
Comment 21 Ted Mielczarek [:ted.mielczarek] 2007-11-11 08:50:25 PST
Mochitest checked in.
Comment 22 Marcia Knous [:marcia - use ni] 2007-11-29 10:29:56 PST
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.
Comment 23 Daniel Veditz [:dveditz] 2007-12-19 10:37:34 PST
Clearing branch wanted flag assuming the regressing change will not land in older NSPR versions.
Comment 24 Wan-Teh Chang 2007-12-19 14:31:58 PST
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.
Comment 25 Daniel Veditz [:dveditz] 2007-12-20 01:04:12 PST
fair enough, it's certainly not going to hurt anything.
Comment 26 Daniel Veditz [:dveditz] 2008-01-08 15:44:15 PST
Comment on attachment 287556 [details] [diff] [review]
update w/review comments

approved for, a=dveditz for release-drivers
Comment 27 Daniel Veditz [:dveditz] 2008-01-08 15:44:40 PST
not really blocking, but approving the patch
Comment 28 Wan-Teh Chang 2008-01-12 18:47:12 PST
Comment on attachment 287556 [details] [diff] [review]
update w/review comments

I checked in this patch on the MOZILLA_1_8_BRANCH for

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
Comment 29 Stephen Donner [:stephend] 2008-01-28 20:05:21 PST
I _cannot_ crash with the Win32 builds (Vista/XP), using 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).
Comment 30 Ted Mielczarek [:ted.mielczarek] 2008-01-29 08:06:39 PST
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.
Comment 31 Wan-Teh Chang 2008-01-29 09:31:50 PST
Stephen: you can verify that I checked in the patch on the MOZILLA_1_8_BRANCH correctly.
(I already did that.)
Comment 33 Alexander Sack 2008-03-12 09:39:42 PDT
distro patch blocks
Comment 34 Alexander Sack 2008-03-12 09:40:53 PDT
Comment on attachment 287556 [details] [diff] [review]
update w/review comments

a=asac for

(unmodified distro patch)
Comment 35 Reed Loden [:reed] (use needinfo?) 2008-03-22 00:52:25 PDT

Checking in content/base/src/nsDocument.cpp;
/cvsroot/mozilla/content/base/src/nsDocument.cpp,v  <--  nsDocument.cpp
new revision: 3.566.; previous revision: 3.566.

Note You need to log in before you can comment on or make changes to this bug.