Closed Bug 331216 Opened 14 years ago Closed 14 years ago

thunderbird crashes while receiving mail via pop3

Categories

(NSPR :: NSPR, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 247896

People

(Reporter: bugs-mozilla-90ximscv, Assigned: wtc)

References

Details

Attachments

(4 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.1) Gecko/20060305 Firefox/1.5.0.1
Build Identifier: version 1.5 (20060207)

Thunderbird crashes while receiving mail via pop3.  Thunderbird prints this on the console:

Assertion failure: tm.tm_month > -1 && tm.tm_mday > 0 && tm.tm_hour > -1 && tm.tm_min > -1 && tm.tm_sec > -1, at ../../../../mozilla/nsprpub/pr/src/misc/prtime.c:1558
Aborted

and quits with exit code 134.

I figured out that one specific mail on the pop3 server was causing the crash.  I removed this message from the server (it was spam, of course) via telnet and checked for new mail with Thunderbird again and it worked as expected.

I'll append the problematic mail as an attachment.

Reproducible: Always

Steps to Reproduce:
this is a debug build doing a pr_assert, which does an exit, right? Does a release build have any problem?
This mail causes the crash.  The Date header seems to be the problem.
(In reply to comment #1)
> this is a debug build doing a pr_assert, which does an exit, right?

I'm using gentoo linux and mozilla-thunderbird-1.5-r1 from the portage tree with use flags "-crypt -debug -gnome +ipv6 -ldap -xinerama -xprint".  I hope it is a release build but I don't know.  Is this a gentoo problem?

> Does a release build have any problem?

Opening the attached mail via "File -> Open Saved Message..." also crashes my thunderbird, so we don't need a pop3 server to test it.  The problem doesn't occure with the thunderbird version from http://download.mozilla.org/?product=thunderbird-1.5&os=linux&lang=en-US

But why doesn't a failed assert condition cause any problems in the release build?  Of course, the immediate exit won't happen with assertions disabled, but the whole program seems to have no problems with tm.tm_month <= -1 or something like that.  Can you explain what's happening here?  Is this a bug or not?  Maybe you want do decrease severity because there seem to be no problems with the release build.
that looks like a debug build.

PR_ASSERT always exits/crashes, by design, in debug builds. It wouldn't be my choice of behavior, however.  But we have to live with it/work around it. Yeah, the rest of the code doesn't mind that problem.
the problem is the stupid space in the time portion of the date
Assignee: mscott → wtchang
Component: General → NSPR
Product: Thunderbird → NSPR
QA Contact: general → wtchang
Version: unspecified → 4.7.3
Attached patch this seems to work (obsolete) — Splinter Review
i should note that i'm doing this hacking on osx/ppc, so all/all :)

the date format is almost certainly entirely invalid. but...

unpatched:
doppler:~/obj-powerpc-apple-darwin7.9.0-camino/nsprpub/pr/tests timeless$ ./date " Two, 21 Mar 2006 13: 7:22  0000"Input:  Two, 21 Mar 2006 13: 7:22  0000 ... 2006-3-21 10:27:22
Tue Mar 21 10:27:22 2006 -0800 {0, 22, 27, 10, 21, 2, 2006, 2, 79, { -28800, 0}}

patched:
doppler:~/dbg-powerpc-apple-darwin7.9.0-mac/nsprpub/pr/tests timeless$ ./date " Two, 21 Mar 2006 13: 7:22  0000"
Input:  Two, 21 Mar 2006 13: 7:22  0000 ... 2006-3-21 13:7:22
Tue Mar 21 13:07:22 2006 -0800 {0, 22, 7, 13, 21, 2, 2006, 2, 79, { -28800, 0}}
OS: Linux → All
Hardware: PC → All
max@ritlabs.com: afaict The Bat! is violating RFC 2822

ftp://ftp.rfc-editor.org/in-notes/rfc2822.txt

FWS             =       ([*WSP CRLF] 1*WSP) /   ; Folding white space
                        obs-FWS
ctext           =       NO-WS-CTL /     ; Non white space controls
                        %d33-39 /       ; The rest of the US-ASCII
                        %d42-91 /       ;  characters not including "(",
                        %d93-126        ;  ")", or "\"
ccontent        =       ctext / quoted-pair / comment
comment         =       "(" *([FWS] ccontent) [FWS] ")"
CFWS            =       *([FWS] comment) (([FWS] comment) / FWS)

time            =       time-of-day FWS zone
time-of-day     =       hour ":" minute [ ":" second ]
hour            =       2DIGIT / obs-hour
minute          =       2DIGIT / obs-minute
second          =       2DIGIT / obs-second
obs-day-of-week =       [CFWS] day-name [CFWS]
obs-year        =       [CFWS] 2*DIGIT [CFWS]
obs-month       =       CFWS month-name CFWS
obs-day         =       [CFWS] 1*2DIGIT [CFWS]
obs-hour        =       [CFWS] 2DIGIT [CFWS]
obs-minute      =       [CFWS] 2DIGIT [CFWS]
obs-second      =       [CFWS] 2DIGIT [CFWS]
obs-zone        =       "UT" / "GMT" /          ; Universal Time

http://www.ietf.org/rfc/rfc0822.txt

     2.6.  NRULE:  SPECIFIC REPETITION

          "<n>(element)" is equivalent to "<n>*<n>(element)"; that is,
     exactly  <n>  occurrences  of (element). Thus 2DIGIT is a 2-digit
     number, and 3ALPHA is a string of three alphabetic characters.

Could you please pass a note along for us?
(In reply to comment #8)

> max@ritlabs.com: afaict The Bat! is violating RFC 2822

The Date format is invalid, but it's a spam mail.  Are you sure The Bat! produces such headers?

Anyway, thunderbird should cope with any conceivable **** in the Date field.
i'm not sure, i'm going based on the X-Mailer header. I don't usually read that header, especially not for spam, so I have no idea what people normally forge in there.

Out of curiosity, what date value would you expect to render for that input? :)

wrt crashing, yeah crashing isn't acceptable (currently release builds don't crash, although their output is imo fairly bad, which is the reason for the debug abort, it's trying to highlight the unhandled case so that someone will "fix it" - either by not using the code or by enhancing the code to handle the input).
*** Bug 331220 has been marked as a duplicate of this bug. ***
Blocks: 247896
*** Bug 332779 has been marked as a duplicate of this bug. ***
Attachment #215874 - Flags: review?(wtchang)
As timeless found out, this is the key change to avoid
storing an invalid value in tmp_min, which causes an
assertion failure.  At this point, 'rest' points to the
first character after the colon (because of the "rest++;"
statement above), so 'end' should be set to 'rest' rather
than 'rest + 1'.  Otherwise, the while loop that looks for
a sequence of digits would not look at the first character
after the colon.
Attachment #217516 - Flags: superreview?(darin)
Attachment #217516 - Flags: review?(timeless)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
This patch improves on timeless's patch named
"this seems to work", which only allows a space
in the minute field.  This patch also allows a
space in the second field.  I am not sure if we
want to allow the spaces.  Also, this patch is
simplistic so it will allow not only "13: 7:22"
but also "13: 17:22".  I believe most people
don't consider the latter a valid HH:MM:SS
string.
Attachment #215874 - Attachment is obsolete: true
Attachment #215874 - Flags: review?(wtchang)
Comment on attachment 217516 [details] [diff] [review]
Minimal patch to fix the assertion failure

>Index: mozilla/nsprpub/pr/src/misc/prtime.c

>                                 while (*rest && *rest != ':')
>                                   rest++;

Suppose we break out of this while loop because *rest is '\0' ...


>                                 rest++;

This would advance rest to point beyond the end of the string,
potentially to an address that is not part of the allocation.


> 
>                                 /* move over the colon, and parse minutes */
> 
>+                                end = rest;
>                                 while (*end >= '0' && *end <= '9')
>                                   end++;

Here we dereference that bad address.  Isn't that a problem?
Darin, thanks for the code review.  The problem of stepping
beyond the end of string can't happen.  When we enter the
while loop in question:

  while (*rest && *rest != ':')
    rest++;
  rest++;

'rest' points to the first digit in a range of digits,
and the character right after the range of digits is a
colon, pointed to by 'end'.

So, the while loop sees only digits before it sees the
colon, which implies that the while loop cannot see the
end of string before it sees the colon.  So we can remove
the *rest test from the while loop because it is always
true:

  while (*rest != ':')
    rest++;
  rest++;

Since 'end' already points at the colon, we can write
the code simply as:

  rest = end + 1;

So I made this change in this patch to make the code easier
to understand.
Attachment #217516 - Attachment is obsolete: true
Attachment #217578 - Flags: superreview?(darin)
Attachment #217578 - Flags: review?(timeless)
Attachment #217516 - Flags: superreview?(darin)
Attachment #217516 - Flags: review?(timeless)
Comment on attachment 217578 [details] [diff] [review]
Minimal patch to fix the assertion failure, v2

yeah, that's making a lot more sense.

+                                rest = end + 1;
+                                end = rest;

you could simplify that...

rest = ++end;

although keeping track of which patches fix which problems is too much for me at 3am.
Attachment #217578 - Flags: review?(timeless) → review+
*** Bug 333266 has been marked as a duplicate of this bug. ***
Status: ASSIGNED → NEW
(yeah, the "reassign" bit /is/ too close to the "commit" button)
Comment on attachment 217578 [details] [diff] [review]
Minimal patch to fix the assertion failure, v2

yeah, i like timeless's suggestion too.  sr=darin either way.
Attachment #217578 - Flags: superreview?(darin) → superreview+
I implemented timeless's suggested change.

I checked in this patch on the NSPR trunk (NSPR 4.7)
and NSPRPUB_PRE_4_2_CLIENT_BRANCH (Mozilla 1.9 alpha).

Checking in prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.24; previous revision: 3.23
done

Checking in prtime.c;
/cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v  <--  prtime.c
new revision: 3.12.2.12; previous revision: 3.12.2.11
done

Let me know if you think we need to fix this bug on the
MOZILLA_1_8_BRANCH.
Attachment #217578 - Attachment is obsolete: true

*** This bug has been marked as a duplicate of 247896 ***
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → DUPLICATE
Target Milestone: --- → 4.7
Version: 4.7.3 → 4.6
I'd definitely like this fixed on the 1.8 branch - as NSPR module owner, it's your call.
Target Milestone: 4.7 → 4.6.2
Attachment #218057 - Flags: approval1.8.1.1?
Attachment #218057 - Flags: approval1.8.1.1?
You need to log in before you can comment on or make changes to this bug.