Bug 1482720 - always check status of SetSpecInternal() so bad specs don't cause a crash. r=aceman a=jorgk BETA_60_CONTINUATION
authorJorg K <jorgk@jorgk.com>
Mon, 13 Aug 2018 08:45:40 +0200
branchBETA_60_CONTINUATION
changeset 32461 5bae0c9b333a
parent 32460 09c4e2c198c3
child 32462 8fa92657e83d
push id385
push userclokep@gmail.com
push date2018-09-04 23:26 +0000
reviewersaceman, jorgk
bugs1482720
Bug 1482720 - always check status of SetSpecInternal() so bad specs don't cause a crash. r=aceman a=jorgk
mailnews/base/util/nsMsgUtils.cpp
mailnews/compose/src/nsMsgComposeService.cpp
mailnews/compose/src/nsSmtpUrl.cpp
mailnews/local/src/nsMailboxService.cpp
mailnews/local/src/nsMailboxUrl.cpp
--- a/mailnews/base/util/nsMsgUtils.cpp
+++ b/mailnews/base/util/nsMsgUtils.cpp
@@ -198,18 +198,19 @@ nsresult CreateStartupUrl(const char *ur
   else if (PL_strncasecmp(uri, "news", 4) == 0)
   {
     nsCOMPtr<nsINntpUrl> nntpUrl = do_CreateInstance(kCNntpUrlCID, &rv);
     if (NS_SUCCEEDED(rv) && nntpUrl)
       rv = nntpUrl->QueryInterface(NS_GET_IID(nsIMsgMailNewsUrl), getter_AddRefs(newUri));
   }
 
   if (newUri) {
-    // SetSpec can fail, for mailbox urls, but we still have a url.
-    (void)newUri->SetSpecInternal(nsDependentCString(uri));
+    // SetSpecInternal must not fail, or else the URL won't have a base URL and we'll crash later.
+    rv = newUri->SetSpecInternal(nsDependentCString(uri));
+    NS_ENSURE_SUCCESS(rv, rv);
 
     newUri.forget(aUrl);
   }
   return rv;
 }
 
 
 // Where should this live? It's a utility used to convert a string priority,
--- a/mailnews/compose/src/nsMsgComposeService.cpp
+++ b/mailnews/compose/src/nsMsgComposeService.cpp
@@ -46,17 +46,16 @@
 #include "nsISelection.h"
 #include "nsUTF8Utils.h"
 #include "mozilla/intl/LineBreaker.h"
 #include "mozilla/Services.h"
 #include "mimemoz2.h"
 #include "nsIArray.h"
 #include "nsArrayUtils.h"
 #include "nsIURIMutator.h"
-#include "mozilla/Unused.h"
 
 #ifdef MSGCOMP_TRACE_PERFORMANCE
 #include "mozilla/Logging.h"
 #include "nsIMsgHdr.h"
 #include "nsIMsgMessageService.h"
 #include "nsMsgUtils.h"
 #endif
 
@@ -1326,24 +1325,24 @@ nsMsgComposeService::RunMessageThroughMi
     mimeConverter->SetOriginalMsgURI(mailboxUri.get());
   }
   if (fileUrl || PromiseFlatCString(aMsgURI).Find("&type=application/x-message-display") >= 0)
     rv = NS_NewURI(getter_AddRefs(url), mailboxUri);
   else
     rv = messageService->GetUrlForUri(PromiseFlatCString(aMsgURI).get(), getter_AddRefs(url), aMsgWindow);
   NS_ENSURE_SUCCESS(rv, rv);
 
-  // ignore errors here - it's not fatal, and in the case of mailbox messages,
-  // we're always passing in an invalid spec...
   nsCOMPtr<nsIMsgMailNewsUrl> mailnewsurl = do_QueryInterface(url);
   if (!mailnewsurl) {
     NS_WARNING("Trying to run a message through MIME which doesn't have a nsIMsgMailNewsUrl?");
     return NS_ERROR_UNEXPECTED;
   }
-  mozilla::Unused << mailnewsurl->SetSpecInternal(mailboxUri);
+  // SetSpecInternal must not fail, or else the URL won't have a base URL and we'll crash later.
+  rv = mailnewsurl->SetSpecInternal(mailboxUri);
+  NS_ENSURE_SUCCESS(rv, rv);
 
   // if we are forwarding a message and that message used a charset over ride
   // then use that over ride charset instead of the charset specified in the message
   nsCString mailCharset;
   if (aMsgWindow)
   {
     bool charsetOverride;
     if (NS_SUCCEEDED(aMsgWindow->GetCharsetOverride(&charsetOverride)) && charsetOverride)
--- a/mailnews/compose/src/nsSmtpUrl.cpp
+++ b/mailnews/compose/src/nsSmtpUrl.cpp
@@ -683,18 +683,17 @@ nsSmtpUrl::nsSmtpUrl() : nsMsgMailNewsUr
 
 nsSmtpUrl::~nsSmtpUrl()
 {
 }
 
 NS_IMETHODIMP
 nsSmtpUrl::Init(const nsACString &aSpec)
 {
-  SetSpecInternal(aSpec);
-  return NS_OK;
+  return SetSpecInternal(aSpec);
 }
 
 NS_IMPL_ISUPPORTS_INHERITED(nsSmtpUrl, nsMsgMailNewsUrl, nsISmtpUrl)
 
 ////////////////////////////////////////////////////////////////////////////////////
 // Begin nsISmtpUrl specific support
 
 ////////////////////////////////////////////////////////////////////////////////////
--- a/mailnews/local/src/nsMailboxService.cpp
+++ b/mailnews/local/src/nsMailboxService.cpp
@@ -547,30 +547,29 @@ NS_IMETHODIMP nsMailboxService::NewURI(c
                                        nsIURI *aBaseURI,
                                        nsIURI **_retval)
 {
   NS_ENSURE_ARG_POINTER(_retval);
   *_retval = 0;
   nsresult rv;
   nsCOMPtr<nsIMsgMailNewsUrl> aMsgUri = do_CreateInstance(NS_MAILBOXURL_CONTRACTID, &rv);
   NS_ENSURE_SUCCESS(rv, rv);
-  // SetSpec calls below may fail if the mailbox url is of the form
-  // mailbox://<account>/<mailbox name>?... instead of
-  // mailbox://<path to folder>?.... This is the case for pop3 download urls.
-  // We know this, and the failure is harmless.
+  // SetSpecInternal must not fail, or else the URL won't have a base URL and we'll crash later.
   if (aBaseURI)
   {
     nsAutoCString newSpec;
     rv = aBaseURI->Resolve(aSpec, newSpec);
     NS_ENSURE_SUCCESS(rv, rv);
-    (void)aMsgUri->SetSpecInternal(newSpec);
+    rv = aMsgUri->SetSpecInternal(newSpec);
+    NS_ENSURE_SUCCESS(rv, rv);
   }
   else
   {
-    (void)aMsgUri->SetSpecInternal(aSpec);
+    rv = aMsgUri->SetSpecInternal(aSpec);
+    NS_ENSURE_SUCCESS(rv, rv);
   }
   aMsgUri.forget(_retval);
 
   return rv;
 }
 
 NS_IMETHODIMP nsMailboxService::NewChannel(nsIURI *aURI, nsIChannel **_retval)
 {
--- a/mailnews/local/src/nsMailboxUrl.cpp
+++ b/mailnews/local/src/nsMailboxUrl.cpp
@@ -412,17 +412,20 @@ nsresult nsMailboxUrl::ParseUrl()
 
   GetPathQueryRef(m_file);
   return NS_OK;
 }
 
 nsresult nsMailboxUrl::SetSpecInternal(const nsACString &aSpec)
 {
   nsresult rv = nsMsgMailNewsUrl::SetSpecInternal(aSpec);
-  if (NS_SUCCEEDED(rv))
+  // Do not try to parse URLs of the form
+  // mailbox://user@domain@server/folder?number=nn since this will fail.
+  // Check for format lacking absolute path.
+  if (NS_SUCCEEDED(rv) && PromiseFlatCString(aSpec).Find("///") != kNotFound)
     rv = ParseUrl();
   return rv;
 }
 
 nsresult nsMailboxUrl::SetQuery(const nsACString &aQuery)
 {
   nsresult rv = nsMsgMailNewsUrl::SetQuery(aQuery);
   if (NS_SUCCEEDED(rv))