Bug 1333038 - Use 'modern' pointers to fix crash due to nsMsgLineStreamBuffer object being deleted while still in use. r=mkmelin a=jorgk
authorJorg K <jorgk@jorgk.com>
Mon, 29 Oct 2018 09:28:55 +0100
changeset 31935 53892f6ffc3a37e0f0c9a4290950f079a68f9425
parent 31934 73ed0515afc2a98dd34890ada1a80324c42daf0a
child 31936 dc6b5da0d19e640c5f267fba283414c924951eff
push id104
push usermozilla@jorgk.com
push dateTue, 20 Nov 2018 12:03:09 +0000
treeherdercomm-esr60@c597411fe241 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmkmelin, jorgk
bugs1333038
Bug 1333038 - Use 'modern' pointers to fix crash due to nsMsgLineStreamBuffer object being deleted while still in use. r=mkmelin a=jorgk Suspected "use after free" in nsMsgLineStreamBuffer::ReadNextLine() leading to crash since object may be destroyed while still in use on another thread.
mailnews/base/util/nsMsgLineBuffer.cpp
mailnews/base/util/nsMsgLineBuffer.h
mailnews/compose/src/nsSmtpProtocol.cpp
mailnews/compose/src/nsSmtpProtocol.h
mailnews/imap/src/nsImapMailFolder.cpp
mailnews/imap/src/nsImapProtocol.cpp
mailnews/imap/src/nsImapProtocol.h
mailnews/imap/src/nsImapService.cpp
mailnews/local/src/nsMailboxProtocol.cpp
mailnews/local/src/nsMailboxProtocol.h
mailnews/local/src/nsPop3Protocol.cpp
mailnews/local/src/nsPop3Protocol.h
mailnews/news/src/nsNNTPProtocol.cpp
mailnews/news/src/nsNNTPProtocol.h
--- a/mailnews/base/util/nsMsgLineBuffer.cpp
+++ b/mailnews/base/util/nsMsgLineBuffer.cpp
@@ -264,17 +264,16 @@ nsMsgLineStreamBuffer::nsMsgLineStreamBu
   m_dataBufferSize = aBufferSize;
 }
 
 nsMsgLineStreamBuffer::~nsMsgLineStreamBuffer()
 {
   PR_FREEIF(m_dataBuffer); // release our buffer...
 }
 
-
 nsresult nsMsgLineStreamBuffer::GrowBuffer(int32_t desiredSize)
 {
   char* newBuffer = (char *) PR_REALLOC(m_dataBuffer, desiredSize);
   NS_ENSURE_TRUE(newBuffer, NS_ERROR_OUT_OF_MEMORY);
   m_dataBuffer = newBuffer;
   m_dataBufferSize = desiredSize;
   return NS_OK;
 }
--- a/mailnews/base/util/nsMsgLineBuffer.h
+++ b/mailnews/base/util/nsMsgLineBuffer.h
@@ -65,39 +65,42 @@ protected:
 // read but unprocessed bytes in a buffer. I envision the primary use of this to be our mail protocols such as imap, news and
 // pop which need to process line by line data being returned in the form of a proxied stream from the server.
 
 class nsIInputStream;
 
 class NS_MSG_BASE nsMsgLineStreamBuffer
 {
 public:
+  NS_INLINE_DECL_REFCOUNTING(nsMsgLineStreamBuffer)
+
   // aBufferSize -- size of the buffer you want us to use for buffering stream data
   // aEndOfLinetoken -- The delimiter string to be used for determining the end of line. This
   //              allows us to parse platform specific end of line endings by making it
   //            a parameter.
   // aAllocateNewLines -- true if you want calls to ReadNextLine to allocate new memory for the line.
   //            if false, the char * returned is just a ptr into the buffer. Subsequent calls to
   //            ReadNextLine will alter the data so your ptr only has a life time of a per call.
   // aEatCRLFs  -- true if you don't want to see the CRLFs on the lines returned by ReadNextLine.
   //         false if you do want to see them.
   // aLineToken -- Specify the line token to look for, by default is LF ('\n') which cover as well CRLF. If
   //            lines are terminated with a CR only, you need to set aLineToken to CR ('\r')
   nsMsgLineStreamBuffer(uint32_t aBufferSize, bool aAllocateNewLines,
                         bool aEatCRLFs = true, char aLineToken = '\n'); // specify the size of the buffer you want the class to use....
-  virtual ~nsMsgLineStreamBuffer();
 
   // Caller must free the line returned using PR_Free
   // aEndOfLinetoken -- delimiter used to denote the end of a line.
   // aNumBytesInLine -- The number of bytes in the line returned
   // aPauseForMoreData -- There is not enough data in the stream to make a line at this time...
   char * ReadNextLine(nsIInputStream * aInputStream, uint32_t &anumBytesInLine, bool &aPauseForMoreData, nsresult *rv = nullptr, bool addLineTerminator = false);
   nsresult GrowBuffer(int32_t desiredSize);
   void ClearBuffer();
   bool NextLineAvailable();
+private:
+  virtual ~nsMsgLineStreamBuffer();
 protected:
   bool m_eatCRLFs;
   bool m_allocateNewLines;
   char * m_dataBuffer;
   uint32_t m_dataBufferSize;
   uint32_t m_startPos;
   uint32_t m_numBytesInBuffer;
   char m_lineToken;
--- a/mailnews/compose/src/nsSmtpProtocol.cpp
+++ b/mailnews/compose/src/nsSmtpProtocol.cpp
@@ -225,17 +225,16 @@ nsSmtpProtocol::nsSmtpProtocol(nsIURI * 
     : nsMsgAsyncWriteProtocol(aURL)
 {
 }
 
 nsSmtpProtocol::~nsSmtpProtocol()
 {
   // free our local state
   PR_Free(m_dataBuf);
-  delete m_lineStreamBuffer;
 }
 
 nsresult nsSmtpProtocol::Initialize(nsIURI * aURL)
 {
     NS_PRECONDITION(aURL, "invalid URL passed into Smtp Protocol");
     nsresult rv = NS_OK;
 
     m_flags = 0;
--- a/mailnews/compose/src/nsSmtpProtocol.h
+++ b/mailnews/compose/src/nsSmtpProtocol.h
@@ -123,17 +123,17 @@ private:
     // Generic state information -- What state are we in? What state do we want to go to
     // after the next response? What was the last response code? etc.
     SmtpState m_nextState;
     SmtpState m_nextStateAfterResponse;
     int32_t m_responseCode;    /* code returned from Smtp server */
     int32_t m_previousResponseCode;
     int32_t m_continuationResponse;
     nsCString m_responseText;   /* text returned from Smtp server */
-    nsMsgLineStreamBuffer *m_lineStreamBuffer; // used to efficiently extract lines from the incoming data stream
+    RefPtr<nsMsgLineStreamBuffer> m_lineStreamBuffer; // used to efficiently extract lines from the incoming data stream
 
     nsTArray<nsCString> m_addresses;
     uint32_t       m_addressesLeft;
     nsCString m_mailAddr;
     nsCString m_helloArgument;
     int32_t        m_sizelimit;
 
     // *** the following should move to the smtp server when we support
--- a/mailnews/imap/src/nsImapMailFolder.cpp
+++ b/mailnews/imap/src/nsImapMailFolder.cpp
@@ -8374,17 +8374,17 @@ nsImapMailFolder::CopyFileToOfflineStore
   }
   msgParser->SetEnvelopePos(offset);
 
   rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), srcFile);
   if (NS_SUCCEEDED(rv) && inputStream)
   {
     // Now, parse the temp file to (optionally) copy to
     // the offline store for the cur folder.
-    nsMsgLineStreamBuffer *inputStreamBuffer =
+    RefPtr<nsMsgLineStreamBuffer> inputStreamBuffer =
       new nsMsgLineStreamBuffer(FILE_IO_BUFFER_SIZE, true, false);
     int64_t fileSize;
     srcFile->GetFileSize(&fileSize);
     uint32_t bytesWritten;
     rv = NS_OK;
     msgParser->SetState(nsIMsgParseMailMsgState::ParseHeadersState);
     msgParser->SetNewMsgHdr(fakeHdr);
     bool needMoreData = false;
@@ -8440,17 +8440,16 @@ nsImapMailFolder::CopyFileToOfflineStore
 
     // Gloda needs this notification to index the fake message.
     nsCOMPtr<nsIMsgFolderNotificationService>
       notifier(do_GetService(NS_MSGNOTIFICATIONSERVICE_CONTRACTID));
     if (notifier)
       notifier->NotifyMsgsClassified(messages, false, false);
     inputStream->Close();
     inputStream = nullptr;
-    delete inputStreamBuffer;
   }
   if (offlineStore)
     offlineStore->Close();
   return rv;
 }
 
 nsresult
 nsImapMailFolder::OnCopyCompleted(nsISupports *srcSupport, nsresult rv)
--- a/mailnews/imap/src/nsImapProtocol.cpp
+++ b/mailnews/imap/src/nsImapProtocol.cpp
@@ -572,17 +572,16 @@ nsImapProtocol::Initialize(nsIImapHostSe
   return NS_OK;
 }
 
 nsImapProtocol::~nsImapProtocol()
 {
   PR_Free(m_fetchBodyIdList);
 
   PR_Free(m_dataOutputBuf);
-  delete m_inputStreamBuffer;
 
   // **** We must be out of the thread main loop function
   NS_ASSERTION(!m_imapThreadIsRunning, "Oops, thread is still running.\n");
 }
 
 const nsCString&
 nsImapProtocol::GetImapHostName()
 {
--- a/mailnews/imap/src/nsImapProtocol.h
+++ b/mailnews/imap/src/nsImapProtocol.h
@@ -331,17 +331,17 @@ private:
   nsCOMPtr<nsIImapUrl> m_runningUrl; // the nsIImapURL that is currently running
   nsImapAction m_imapAction;  // current imap action associated with this connection...
 
   nsCString             m_hostName;
   nsCString             m_userName;
   nsCString             m_serverKey;
   nsCString             m_realHostName;
   char                  *m_dataOutputBuf;
-  nsMsgLineStreamBuffer * m_inputStreamBuffer;
+  RefPtr<nsMsgLineStreamBuffer> m_inputStreamBuffer;
   uint32_t              m_allocatedSize; // allocated size
   uint32_t        m_totalDataSize; // total data size
   uint32_t        m_curReadIndex;  // current read index
   nsCString       m_trashFolderPath;
 
   // Output stream for writing commands to the socket
   nsCOMPtr<nsISocketTransport>  m_transport;
   nsCOMPtr<nsIInputStream> m_inputStream;
--- a/mailnews/imap/src/nsImapService.cpp
+++ b/mailnews/imap/src/nsImapService.cpp
@@ -2050,19 +2050,20 @@ nsresult nsImapService::OfflineAppendFro
         nsCOMPtr <nsIInputStream> inputStream;
         nsCOMPtr <nsIMsgParseMailMsgState> msgParser = do_CreateInstance(NS_PARSEMAILMSGSTATE_CONTRACTID, &rv);
         msgParser->SetMailDB(destDB);
 
         rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), aFile);
         if (NS_SUCCEEDED(rv) && inputStream)
         {
           // now, copy the temp file to the offline store for the dest folder.
-          nsMsgLineStreamBuffer *inputStreamBuffer = new nsMsgLineStreamBuffer(FILE_IO_BUFFER_SIZE,
-                                                                               true,    // allocate new lines
-                                                                               false);  // leave CRLFs on the returned string
+          RefPtr<nsMsgLineStreamBuffer> inputStreamBuffer =
+            new nsMsgLineStreamBuffer(FILE_IO_BUFFER_SIZE,
+                                      true,    // allocate new lines
+                                      false);  // leave CRLFs on the returned string
           int64_t fileSize;
           aFile->GetFileSize(&fileSize);
           uint32_t bytesWritten;
           rv = NS_OK;
 //        rv = inputStream->Read(inputBuffer, inputBufferSize, &bytesRead);
 //        if (NS_SUCCEEDED(rv) && bytesRead > 0)
           msgParser->SetState(nsIMsgParseMailMsgState::ParseHeadersState);
           msgParser->SetNewMsgHdr(newMsgHdr);
@@ -2098,17 +2099,16 @@ nsresult nsImapService::OfflineAppendFro
               if (msgStore)
                 msgStore->FinishNewMessage(offlineStore, fakeHdr);
             }
           }
           // tell the listener we're done.
           inputStream->Close();
           inputStream = nullptr;
           aListener->OnStopRunningUrl(aUrl, NS_OK);
-          delete inputStreamBuffer;
         }
         offlineStore->Close();
       }
     }
   }
 
   if (destDB)
     destDB->Close(true);
--- a/mailnews/local/src/nsMailboxProtocol.cpp
+++ b/mailnews/local/src/nsMailboxProtocol.cpp
@@ -49,23 +49,20 @@ static LazyLogModule MAILBOX("Mailbox");
  *
  * fortezza: proxy auth is huge, buffer increased to 8k (sigh).
  */
 #define OUTPUT_BUFFER_SIZE (4096*2)
 
 nsMailboxProtocol::nsMailboxProtocol(nsIURI * aURI)
     : nsMsgProtocol(aURI)
 {
-  m_lineStreamBuffer =nullptr;
 }
 
 nsMailboxProtocol::~nsMailboxProtocol()
 {
-  // free our local state
-  delete m_lineStreamBuffer;
 }
 
 nsresult nsMailboxProtocol::OpenMultipleMsgTransport(uint64_t offset, int64_t size)
 {
   nsresult rv;
 
   nsCOMPtr<nsIStreamTransportService> serv =
       do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID, &rv);
--- a/mailnews/local/src/nsMailboxProtocol.h
+++ b/mailnews/local/src/nsMailboxProtocol.h
@@ -68,17 +68,17 @@ public:
 private:
   nsCOMPtr<nsIMailboxUrl>  m_runningUrl; // the nsIMailboxURL that is currently running
   nsMailboxAction m_mailboxAction; // current mailbox action associated with this connection...
   uint64_t m_msgOffset;
   // Event sink handles
   nsCOMPtr<nsIStreamListener> m_mailboxParser;
 
   // Local state for the current operation
-  nsMsgLineStreamBuffer   * m_lineStreamBuffer; // used to efficiently extract lines from the incoming data stream
+  RefPtr<nsMsgLineStreamBuffer> m_lineStreamBuffer; // used to efficiently extract lines from the incoming data stream
 
   // Generic state information -- What state are we in? What state do we want to go to
   // after the next response? What was the last response code? etc.
   MailboxStatesEnum  m_nextState;
   MailboxStatesEnum  m_initialState;
 
   int64_t   mCurrentProgress;
 
--- a/mailnews/local/src/nsPop3Protocol.cpp
+++ b/mailnews/local/src/nsPop3Protocol.cpp
@@ -449,17 +449,16 @@ NS_INTERFACE_MAP_END_INHERITING(nsMsgPro
 // nsPop3Protocol class implementation
 
 nsPop3Protocol::nsPop3Protocol(nsIURI* aURL)
 : nsMsgProtocol(aURL),
   m_bytesInMsgReceived(0),
   m_totalFolderSize(0),
   m_totalDownloadSize(0),
   m_totalBytesReceived(0),
-  m_lineStreamBuffer(nullptr),
   m_pop3ConData(nullptr)
 {
 }
 
 nsresult nsPop3Protocol::Initialize(nsIURI * aURL)
 {
   MOZ_LOG(POP3LOGMODULE, LogLevel::Debug, (POP3LOG("Initialize()")));
 
@@ -610,19 +609,16 @@ void nsPop3Protocol::Cleanup()
     m_pop3ConData->newuidl = nullptr;
   }
 
   net_pop3_free_state(m_pop3ConData->uidlinfo);
 
   FreeMsgInfo();
   PR_Free(m_pop3ConData->only_uidl);
   PR_Free(m_pop3ConData);
-
-  delete m_lineStreamBuffer;
-  m_lineStreamBuffer = nullptr;
 }
 
 void nsPop3Protocol::SetCapFlag(uint32_t flag)
 {
     m_pop3ConData->capability_flags |= flag;
 }
 
 void nsPop3Protocol::ClearCapFlag(uint32_t flag)
--- a/mailnews/local/src/nsPop3Protocol.h
+++ b/mailnews/local/src/nsPop3Protocol.h
@@ -318,17 +318,17 @@ private:
   virtual int32_t Pop3SendData(const char * dataBuffer, bool aSuppressLogging = false);
 
   virtual const char* GetType() override {return "pop3";}
 
   nsCOMPtr<nsIURI> m_url;
   nsCOMPtr<nsIPop3Sink> m_nsIPop3Sink;
   nsCOMPtr<nsIPop3IncomingServer> m_pop3Server;
 
-  nsMsgLineStreamBuffer   * m_lineStreamBuffer; // used to efficiently extract lines from the incoming data stream
+  RefPtr<nsMsgLineStreamBuffer> m_lineStreamBuffer; // used to efficiently extract lines from the incoming data stream
   Pop3ConData* m_pop3ConData;
   void FreeMsgInfo();
   void Abort();
 
   bool m_tlsEnabled;
   int32_t m_socketType;
   bool m_password_already_sent;
   bool m_needToRerunUrl;
--- a/mailnews/news/src/nsNNTPProtocol.cpp
+++ b/mailnews/news/src/nsNNTPProtocol.cpp
@@ -260,17 +260,16 @@ NS_IMPL_ISUPPORTS_INHERITED(nsNNTPProtoc
 
 nsNNTPProtocol::nsNNTPProtocol(nsINntpIncomingServer *aServer, nsIURI *aURL,
                                nsIMsgWindow *aMsgWindow)
 : nsMsgProtocol(aURL),
   m_connectionBusy(false),
   m_nntpServer(aServer)
 {
   m_ProxyServer = nullptr;
-  m_lineStreamBuffer = nullptr;
   m_responseText = nullptr;
   m_dataBuf = nullptr;
 
   m_cancelFromHdr = nullptr;
   m_cancelNewsgroups = nullptr;
   m_cancelDistribution = nullptr;
   m_cancelID = nullptr;
 
@@ -294,19 +293,16 @@ nsNNTPProtocol::nsNNTPProtocol(nsINntpIn
 
 nsNNTPProtocol::~nsNNTPProtocol()
 {
   MOZ_LOG(NNTP, LogLevel::Info,("(%p) destroying",this));
   if (m_nntpServer) {
     m_nntpServer->WriteNewsrcFile();
     m_nntpServer->RemoveConnection(this);
   }
-  if (m_lineStreamBuffer) {
-     delete m_lineStreamBuffer;
-  }
   if (mUpdateTimer) {
     mUpdateTimer->Cancel();
     mUpdateTimer = nullptr;
   }
   Cleanup();
 }
 
 void nsNNTPProtocol::Cleanup()  //free char* member variables
--- a/mailnews/news/src/nsNNTPProtocol.h
+++ b/mailnews/news/src/nsNNTPProtocol.h
@@ -198,17 +198,17 @@ private:
   nsCOMPtr <nsINNTPNewsgroupList> m_newsgroupList;
   nsCOMPtr <nsINNTPArticleList> m_articleList;
 
   nsCOMPtr <nsIMsgNewsFolder> m_newsFolder;
   nsCOMPtr <nsIMsgWindow> m_msgWindow;
 
   nsCOMPtr<nsIAsyncInputStream> mDisplayInputStream;
   nsCOMPtr<nsIAsyncOutputStream> mDisplayOutputStream;
-  nsMsgLineStreamBuffer   * m_lineStreamBuffer; // used to efficiently extract lines from the incoming data stream
+  RefPtr<nsMsgLineStreamBuffer> m_lineStreamBuffer; // used to efficiently extract lines from the incoming data stream
   // the nsINntpURL that is currently running
   nsCOMPtr<nsINntpUrl> m_runningURL;
   bool        m_connectionBusy;
   bool        m_fromCache;  // is this connection from the cache?
   PRTime      m_lastActiveTimeStamp;
   nsNewsAction m_newsAction;
   nsCOMPtr <nsISupports> m_consumer;