Last Comment Bug 856506 - crash in nsSmtpProtocol when parsing IDN
: crash in nsSmtpProtocol when parsing IDN
Status: RESOLVED FIXED
[regression:TB20]
: crash, regression
Product: MailNews Core
Classification: Components
Component: Composition (show other bugs)
: 20
: All All
: -- critical (vote)
: Thunderbird 23.0
Assigned To: Makoto Kato [:m_kato] (PTO 9/22-9/25)
:
Mentors:
Depends on:
Blocks: 127399
  Show dependency treegraph
 
Reported: 2013-03-31 21:07 PDT by Makoto Kato [:m_kato] (PTO 9/22-9/25)
Modified: 2013-04-14 08:45 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed
+
fixed


Attachments
fix (1.19 KB, patch)
2013-03-31 21:09 PDT, Makoto Kato [:m_kato] (PTO 9/22-9/25)
standard8: review+
standard8: approval‑comm‑beta+
Details | Diff | Splinter Review
test (1.79 KB, patch)
2013-04-01 00:00 PDT, Makoto Kato [:m_kato] (PTO 9/22-9/25)
standard8: review+
Details | Diff | Splinter Review

Description Makoto Kato [:m_kato] (PTO 9/22-9/25) 2013-03-31 21:07:19 PDT
This is regression of bug 127399 (IDN support).  When lastAt is nullptr, the following crash occur due to invalid IDN?.

This bug was filed from the Socorro interface and is 
report bp-4c8b81bc-16e9-4c44-988e-874862130330 .
============================================================= 
0 	msvcr100.dll 	memcpy 	f:\dd\vctools\crt_bld\SELF_X86\crt\src\INTEL\memcpy.asm:185
1 	xul.dll 	nsACString_internal::Assign 	xpcom/string/src/nsTSubstring.cpp:314
2 	xul.dll 	nsACString_internal::Assign 	xpcom/string/src/nsTSubstring.cpp:290
3 	xul.dll 	nsACString_internal::Assign 	xpcom/string/src/nsTSubstring.cpp:308
4 	xul.dll 	nsACString_internal::Assign 	xpcom/string/src/nsTSubstring.cpp:290
5 	xul.dll 	nsSmtpProtocol::LoadUrl 	mailnews/compose/src/nsSmtpProtocol.cpp:1772
6 	xul.dll 	NS_MsgLoadSmtpUrl 	mailnews/compose/src/nsSmtpService.cpp:224
7 	xul.dll 	nsSmtpService::SendMailMessage 	mailnews/compose/src/nsSmtpService.cpp:105
8 	xul.dll 	nsMsgComposeAndSend::DeliverFileAsMail 	mailnews/compose/src/nsMsgSend.cpp:3640
9 	xul.dll 	nsMsgComposeAndSend::DeliverMessage 	mailnews/compose/src/nsMsgSend.cpp:3494
10 	xul.dll 	nsMsgComposeAndSend::GatherMimeAttachments 	mailnews/compose/src/nsMsgSend.cpp:1013
11 	xul.dll 	nsMsgAttachmentHandler::UrlExit 	mailnews/compose/src/nsMsgAttachmentHandler.cpp:1222
12 	xul.dll 	FetcherURLDoneCallback 	mailnews/compose/src/nsMsgAttachmentHandler.cpp:470
13 	xul.dll 	nsURLFetcher::OnStopRequest 	mailnews/compose/src/nsURLFetcher.cpp:281
14 	xul.dll 	nsDocumentOpenInfo::OnStopRequest 	uriloader/base/nsURILoader.cpp:297
15 	xul.dll 	nsBaseChannel::OnStopRequest 	netwerk/base/src/nsBaseChannel.cpp:737
16 	xul.dll 	nsInputStreamPump::OnStateStop 	netwerk/base/src/nsInputStreamPump.cpp:552
17 	xul.dll 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:374
18 	xul.dll 	nsOutputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:82
19 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
20 	xul.dll 	NS_ProcessNextEvent_P 	objdir-tb/mozilla/xpcom/build/nsThreadUtils.cpp:238
21 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:82
22 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:208
23 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:182
24 	xul.dll 	nsBaseAppShell::Run 	widget/xpwidgets/nsBaseAppShell.cpp:163
25 	xul.dll 	nsAppShell::Run 	widget/windows/nsAppShell.cpp:154
26 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:288
27 	xul.dll 	XREMain::XRE_mainRun 	toolkit/xre/nsAppRunner.cpp:3823
28 	xul.dll 	XREMain::XRE_main 	toolkit/xre/nsAppRunner.cpp:3890
29 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:4093
30 	thunderbird.exe 	do_main 	mail/app/nsMailApp.cpp:111
31 	thunderbird.exe 	NS_internal_main 	mail/app/nsMailApp.cpp:200
32 	thunderbird.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:105
33 	thunderbird.exe 	__tmainCRTStartup 	crtexe.c:552
34 	kernel32.dll 	BaseThreadInitThunk 	
35 	ntdll.dll 	__RtlUserThreadStart 	
36 	ntdll.dll 	_RtlUserThreadStart
Comment 1 Makoto Kato [:m_kato] (PTO 9/22-9/25) 2013-03-31 21:09:10 PDT
Created attachment 731761 [details] [diff] [review]
fix
Comment 2 Makoto Kato [:m_kato] (PTO 9/22-9/25) 2013-04-01 00:00:30 PDT
Created attachment 731775 [details] [diff] [review]
test
Comment 3 :aceman 2013-04-02 08:08:37 PDT
So is the comment "// Fortunately, we will always have an @ in each mailbox address." in the code right? Does it need some update?
Comment 4 Mark Banner (:standard8) 2013-04-09 02:33:40 PDT
(In reply to :aceman from comment #3)
> So is the comment "// Fortunately, we will always have an @ in each mailbox
> address." in the code right? Does it need some update?

So yes, that's wrong really. In theory we could have just a local address (no domain), but I'm not sure if that's still really used these days.

I think for now, the patch is probably fine (with the fix to the comment) to fix the crash, and we'll see if there's any issues when we push it out.
Comment 5 Mark Banner (:standard8) 2013-04-09 02:36:46 PDT
Comment on attachment 731761 [details] [diff] [review]
fix

Review of attachment 731761 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ -1768,3 @@
>            if (firstEvil)
>            {
>              // Fortunately, we will always have an @ in each mailbox address.

As aceman, says we should update, probably remove this comment, because it isn't true.
Comment 6 Mark Banner (:standard8) 2013-04-09 12:51:27 PDT
Comment on attachment 731761 [details] [diff] [review]
fix

[Triage Comment]
Due to the crashiness of this in TB 20 beta, and the fact we're just about to spin the one beta for this cycle, I've landed this on the beta branch:

https://hg.mozilla.org/releases/comm-beta/rev/1d1de403c584

We'll land on trunk and aurora once trunk re-opens.
Comment 7 Karsten Düsterloh 2013-04-10 23:01:57 PDT
(In reply to Mark Banner (:standard8) from comment #4)
> (In reply to :aceman from comment #3)
> > So is the comment "// Fortunately, we will always have an @ in each mailbox
> > address." in the code right? Does it need some update?
> 
> So yes, that's wrong really.

Well, it wasn't when I wrote it; or rather: the codepath from clicking "Send" in the editor down here would have killed any other addresses. I'm a bit puzzled about what changed actually?

> In theory we could have just a local address (no domain),

Of course.

> but I'm not sure if that's still really used these days.

At least we didn't support it.
Which, otoh, I do loath sometimes, so I should not have relied on this assumption here.

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