Last Comment Bug 737519 - attachment1 [details] [diff] [review]_body of nsProxySendRunnable will not be non-null terminated string.
: attachment1_body of nsProxySendRunnable will not be non-null terminated string.
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Import (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: Thunderbird 22.0
Assigned To: Makoto Kato [:m_kato]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-20 10:59 PDT by Makoto Kato [:m_kato]
Modified: 2013-03-26 07:38 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
fix (12.81 KB, patch)
2012-03-20 12:19 PDT, Makoto Kato [:m_kato]
mozilla: feedback+
Details | Diff | Review
fix v1.1 (9.10 KB, patch)
2012-08-20 01:00 PDT, Makoto Kato [:m_kato]
standard8: review+
Details | Diff | Review
Updated for Neil's comments (21.08 KB, patch)
2013-03-25 11:36 PDT, Mark Banner (:standard8)
neil: superreview+
Details | Diff | Review

Description Makoto Kato [:m_kato] 2012-03-20 10:59:18 PDT
attachment1 [details] [diff] [review]_body of nsProxySendRunnable can pass non-null string since attachment1 [details] [diff] [review]_body_length is length of it.  But attachment1 [details] [diff] [review]_body is used as null-terminated string.
Comment 1 Makoto Kato [:m_kato] 2012-03-20 12:19:39 PDT
Created attachment 607655 [details] [diff] [review]
fix
Comment 2 Makoto Kato [:m_kato] 2012-08-20 01:00:16 PDT
Created attachment 653303 [details] [diff] [review]
fix v1.1

rebased for current c-c
Comment 3 Mark Banner (:standard8) 2012-09-25 02:04:36 PDT
Comment on attachment 653303 [details] [diff] [review]
fix v1.1

Looks good, this will need a quick sr as well though.
Comment 4 neil@parkwaycc.co.uk 2012-09-25 05:29:28 PDT
Sorry, but I can't actually see where the problem is.

Both nsEudoraCompose and nsOutlookCompose pass the .get() and .Length() of an nsCString to nsImportService::CreateRFC822Message that goes on to create an nsProxySendRunnable that assumes that the string is null-terminated (which it fortunately is) and copies it (it would obviously be much more efficient to pass these as a const nsACString& instead) then (on the main thread) passes the (null-terminated) copy to nsMsgComposeAndSend::CreateRFC822Message.
Comment 5 Mark Banner (:standard8) 2012-09-25 05:38:01 PDT
I assumed this was just tidying up the interfaces and removing the extra variable and improving efficiency...
Comment 6 neil@parkwaycc.co.uk 2012-09-25 11:21:04 PDT
Comment on attachment 653303 [details] [diff] [review]
fix v1.1

>diff --git a/mailnews/compose/public/nsIMsgSend.idl b/mailnews/compose/public/nsIMsgSend.idl
>-                           in string aBody,
>-                           in unsigned long aBodyLength,
>+                           in ACString aBody,
>diff --git a/mailnews/compose/src/nsMsgSend.cpp b/mailnews/compose/src/nsMsgSend.cpp
>-            aMsgType, aMsgBody,
>-            aMsgBodyLength,
>+            aMsgType,
>+            aMsgBody.IsEmpty() ? nullptr : PromiseFlatCString(aMsgBody).get(),
>+            aMsgBody.Length(),
Ah, well as cleanup then it mostly makes sense, except I'm not really keen on this change, until Init and SnarfAndCopyBody are changed to use nsCString too.

>-                                      m_bodyType.get(), m_body.get(),
>-                                      m_bodyLength, m_isDraft, m_loadedAttachments,
>+                                      m_bodyType.get(), m_body,
>+                                      m_isDraft, m_loadedAttachments,
Instead I would just use m_body.Length() here for now.
Comment 7 Wayne Mery (:wsmwk, NI for questions) 2012-12-04 16:01:13 PST
might want to catch this before it bitrots :)
Comment 8 Mark Banner (:standard8) 2013-03-25 11:36:04 PDT
Created attachment 729095 [details] [diff] [review]
Updated for Neil's comments

I've adapted the patch for Neil's review comments and fixed bitrot. As it turns out it was easier to change a few additional interfaces to make this all consistent.

It passed unit tests on try server.
Comment 9 neil@parkwaycc.co.uk 2013-03-25 17:05:53 PDT
Comment on attachment 729095 [details] [diff] [review]
Updated for Neil's comments

>-      if (newBody)
Nit: Variable is never used, so it can be removed completely.

>+  const char *body = PromiseFlatCString(aBody).get();
This is technically unsafe, although because you only have the one caller then I think you'll find it happens to work. (If you had written the caller to use StringHead instead of Trim then it would not have worked.)
One way to fix it is by making aBody a const nsCString& so you can safely use a plain get() instead.
[At some point m_attachment1_body should become an nsCString and these functions will just do direct string API on it.]
Comment 10 Mark Banner (:standard8) 2013-03-26 07:38:57 PDT
https://hg.mozilla.org/comm-central/rev/28429b5084d7

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