(A follow-up on Bug 239455) In various places of Mozilla Code (specifically Thunderbird), files are opened in write-mode, e.g using MsgNewBufferedFileOutputStream. In Linux systems for example, not not necessarily exclusively, the file is truncated to zero size by opening. If the disk is full or very close to full, writing will fail, leaving the file in a corrupt state, with loss of information. This problem can be solved by first writing to a temporary file, and then applying the move-file operation, which is atomic in some operating systems (e.g. Linux). MsgNewSafeBufferedFileOutputStream from Bug 239455 implements this functionality, and should be used in place of other methods, e.g. when there is raw text written out. Where sql databases are written, the case might be different. This bug should identify and fix code not safe against a full disk.

After Johannes finds the candidate spots in the code, we should probably decide on each of them whether it should be converted, depending on the importance of the file and it's possible size (e.g. we can't do this for places.sqlite that can grow into tens of megabytes). Also, this probably applies only to files that are rewritten in full at each open, not those that are appended to or modified in internal junks (like the databases).

Where possible, we should also look at making these I/O operations asynchronous; most current uses of SafeBufferedFileOutputStream block the UI thread while writing, which is something we're trying to reduce. From JavaScript we can use the new OS.File.writeAtomic() implementation; from C++ code I'm not sure if we have straightforward async I/O wrappers.

DXR search for NS_NewBufferedOutputStream http://dxr.mozilla.org/mozilla-central/search?q=NS_NewBufferedOutputStream&redirect=false http://dxr.mozilla.org/mozilla-central/source/dom/src/json/nsJSON.cpp#l130 JSON, only called from JS. Failure is being escalated -> no action http://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#l935 DeviceStorageFile::Write called from: http://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#l935 http://dxr.mozilla.org/mozilla-central/source/dom/devicestorage/nsDeviceStorage.cpp#l1911 What kind of data does this handle? Is it important to not override? Information needed. http://dxr.mozilla.org/mozilla-central/source/extensions/spellcheck/src/mozPersonalDictionary.cpp#l162 mozPersonalDictionary::Save() Here, should replace NS_NewBufferedOutputStream with MsgNewBufferedFileOutputStream. http://dxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#l8565 DumpToPNG: Debug code (DEBUG_Eli) -> no action http://dxr.mozilla.org/mozilla-central/source/modules/libjar/zipwriter/src/nsZipWriter.cpp#l269 nsZipWriter::Open tries to write to local file without any buffering first. If fails, uses buffering. No callers found. Maybe fine -> no action http://dxr.mozilla.org/mozilla-central/source/modules/libpref/src/Preferences.cpp#l781 Preferences::WritePrefFile User preferences. Uses safety, but can benefit from refactoring to use new function MsgNewBufferedFileOutputStream http://dxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#l1131 NS_BufferOutputStream function tries to do online-buffering, after opening called from: http://dxr.mozilla.org/mozilla-central/source/embedding/components/webbrowserpersist/src/nsWebBrowserPersist.cpp#l2288 nsWebBrowserPersist::MakeOutputStreamFromFile comment: "// XXX brade: get the right flags here!" possibly unnecessarily low-level (setting some flags on output file). Can this be simplified and replaced by MsgNewBufferedFileOutputStream? Has to do with Web browser, probably not important enough data to justify writing to temp file. http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/BackgroundFileSaver.cpp#l367 BackgroundFileSaver::ProcessStateChange Downloaded files are written into temp file already, so no need to change things here. Even so, a partially downloaded/corrupt file is unproblematic for the application. -> no action http://dxr.mozilla.org/mozilla-central/source/netwerk/cache/nsDiskCacheDeviceSQL.cpp#l1782 nsOfflineCacheDevice::OpenOutputStreamForEntry Caching. Nobody cares about safe caching -> no action http://dxr.mozilla.org/mozilla-central/source/netwerk/test/TestFileInput2.cpp#l159 FileSpecWorker:Run Test case -> no action http://dxr.mozilla.org/mozilla-central/source/rdf/base/src/nsRDFXMLDataSource.cpp#l771 RDFXMLDataSourceImpl::rdfXMLFlush already uses NS_NewSafeLocalFileOutputStream for one stream, but not for another, buffered stream. -> replace with MsgNewBufferedFileOutputStream. http://dxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsCertOverrideService.cpp#l355 nsCertOverrideService::Write PSM certificate file can benefit from safe writing -> replace with MsgNewBufferedFileOutputStream http://dxr.mozilla.org/mozilla-central/source/widget/windows/WinUtils.cpp#l755 AsyncEncodeAndWriteIcon::Run() Writing icons. nobody cares -> no action Same searching for NS_NewSafeLocalFileOutputStream http://dxr.mozilla.org/mozilla-central/search?q=NS_NewSafeLocalFileOutputStream gives these additional results http://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/LookupCache.cpp#l227 LookupCache::WriteFile() Lookup for urls ( I presume. Has its own variant of writing to a temporary file. What is this function trying to do with do_QueryInterface->Finish() ? Buffering would probably not hurt this function. Advice needed. The other 3 results have been mentioned before, they can benefit from refactoring and re-use of the MsgNewBufferedFileOutputStream function Preferences::WritePrefFile(nsIFile* aFile) RDFXMLDataSourceImpl::rdfXMLFlush(nsIURI *aURI) nsCertOverrideService::Write()

Any suggestions for other search terms?

Didn't we want to only check mailnews (comm-central) ?

Safe writing in case of full disk for preference file and personal dictionary.

Safe writing in case of full disk for nsMsgSaveAsListener & nsSaveMsgListener. in nsMsgAccountManager, just refactoring

WIP; all other occurances of "FileOutputStream" I could find are related to writing new or temporary files, i.e. where there is no danger of data loss through failed overwriting.

Using "grep -n FileOutputStream -R mailnews/ mail ldap/ editor/ config calendar/" : not modified: mailnews/base/src/nsMsgFolderCompactor.cpp -- not modified, uses own, quite tricky move method mailnews/base/util/nsMsgUtils.cpp -- is where MsgNewSafeBufferedFileOutputStream is defined mailnews/base/search/src/nsMsgFilterList.cpp -- appends to log file mailnews/imap/src/nsImapMailFolder.cpp, mailnews/imap/src/nsImapOfflineSync.cpp -- write tempfiles mailnews/mime/src/mimedrft.cpp, mailnews/mime/src/mimepbuf.cpp, mailnews/mime/src/mimemrel.cpp -- write tempfiles mailnews/news/src/nsNntpIncomingServer.cpp -- unclear mailnews/compose/src/nsMsgAttachmentHandler.cpp -- write tempfiles mailnews/compose/src/nsMsgSend.cpp, mailnews/compose/src/nsMsgSendLater.cpp -- write new/tempfiles mailnews/local/src/nsPop3Protocol.cpp, mailnews/local/src/nsMailboxProtocol.cpp, mailnews/local/src/nsMsgMaildirStore.cpp -- write new/tempfiles mailnews/import/src/ImportOutFile.cpp -- write new/tempfiles mailnews/import/eudora/src/nsEudoraMac.cpp -- writes new file mailnews/import/outlook/src/MapiMessage.cpp -- writes temp file mailnews/addrbook/src/nsAbManager.cpp -- exports to new file mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp -- writes new/temp file modified: mailnews/base/search/src/nsMsgFilterService.cpp -- only refactoring, no change mailnews/base/src/nsMsgAccountManager.cpp -- modified by patch mailnews/base/util/nsMsgMailNewsUrl.cpp -- modified by patch mailnews/base/src/nsMessenger.cpp -- modified by patch; actually also writes to new files, but those could be attachments I have not considered any JS code; if it is important, perhaps you can point out an example. Otherwise, let me know what you think about the patches, and about the files I did not touch.

Comment on attachment 812726 [details] [diff] [review] safewrite_comm-central.patch Review of attachment 812726 [details] [diff] [review]: ----------------------------------------------------------------- Just needs a couple of minor corrections. ::: mailnews/base/search/src/nsMsgFilterService.cpp @@ +131,1 @@ > filterFile, -1, 0600); I think you want MsgNew*Safe*BufferedFileOutputStream here. ::: mailnews/base/src/nsMessenger.cpp @@ +1616,5 @@ > } > else if (m_outputStream && mRequestHasStopped) > { > + nsCOMPtr<nsISafeOutputStream> safeStream = do_QueryInterface(m_outputStream); > + rv = safeStream->Finish(); See below - I don't think this needs to be a SafeOutputStream. @@ +1753,5 @@ > NS_IMETHODIMP > nsSaveMsgListener::OnStartRequest(nsIRequest* request, nsISupports* aSupport) > { > if (m_file) > + MsgNewSafeBufferedFileOutputStream(getter_AddRefs(m_outputStream), m_file, -1, ATTACHMENT_PERMISSION); The places where we construct nsSaveMsgListener make sure we're working with file names that don't already exist, so I don't think we need a safe output stream here. ::: mailnews/base/util/nsMsgMailNewsUrl.cpp @@ +911,5 @@ > // IMAP and NNTP use this nsMsgSaveAsListener here, though, so we > // have to close the stream before deleting the file, else data > // would still be written happily into a now non-existing file. > // (Windows doesn't care, btw, just unixoids do...) > + Trailing white space @@ +914,5 @@ > // (Windows doesn't care, btw, just unixoids do...) > + > + // Johannes Buchner: This problem should not occur anymore, as > + // MsgNewSafeBufferedFileOutputStream overrides the file by a move operation. > + // aFile->Remove(false); The commented out lines should be removed.

Comment on attachment 812688 [details] [diff] [review] safewrite_mozilla-central.patch I'm not a reviewer for this part of mozilla-central. Based on the change history of the files you've modified, Ehsan Akhgari or Benjamin Smedberg would be appropriate choices, or they may suggest someone else.

Comment on attachment 812688 [details] [diff] [review] safewrite_mozilla-central.patch Review of attachment 812688 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/spellcheck/src/mozPersonalDictionary.cpp @@ +160,2 @@ > nsCOMPtr<nsIOutputStream> bufferedOutputStream; > + if (NS_FAILED(res)) return res; You're not assigning to res before this, so this check is unnecessary. @@ +173,5 @@ > bufferedOutputStream->Write(utf8Key.get(), utf8Key.Length(), &bytesWritten); > bufferedOutputStream->Write("\n", 1, &bytesWritten); > } > + nsCOMPtr<nsISafeOutputStream> safeStream = do_QueryInterface(bufferedOutputStream); > + safeStream->Finish(); What currently guarantees that safeStream would not be null here (i.e. the object does implement nsISafeOutputStream)? It seems to me like a null check is needed here. Also, this will run the risk of somebody adding an early return statement later on in this code which unintentionally suppresses the call to Finish(). Can you please use an RAII class here to prevent against that? ::: modules/libpref/src/Preferences.cpp @@ +903,2 @@ > rv = NS_NewBufferedOutputStream(getter_AddRefs(outStream), outStreamSink, 4096); > + if (NS_FAILED(rv)) return rv; Please don't do this!

Thank you for your comments. They make sense to me.

@Ehsan: regarding RAII: on destruction of the temporary stream, nsSafeFileOutputStream calls Close(). This closes the temporary file and removes it. So it should be handled nicely already.

I use the same finish() everywhere now.

I meant to say, I use the same finish() construct everywhere now. Please review again.

Comment on attachment 826555 [details] [diff] [review] safewrite_comm-central.patch I am not interested in reviewing tbird code.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #22) > Comment on attachment 826555 [details] [diff] [review] > safewrite_comm-central.patch > > I am not interested in reviewing tbird code. My apologies, you were meant for the the other patch, which concerns mozilla-central.

Comment on attachment 826556 [details] [diff] [review] safewrite_mozilla-central.patch Review of attachment 826556 [details] [diff] [review]: ----------------------------------------------------------------- I know nothing about this code.

Comment on attachment 826556 [details] [diff] [review] safewrite_mozilla-central.patch This is still a mailnews patch...

I thoroughly messed this up and uploaded the same patch twice. I'm so sorry for wasting your time!

Comment on attachment 832147 [details] [diff] [review] safewrite_mozilla-central.patch Review of attachment 832147 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below fixed. ::: extensions/spellcheck/src/mozPersonalDictionary.cpp @@ +157,3 @@ > > // get a buffered output stream 4096 bytes big, to optimize writes > + // similar to mailnews/base/util/nsMsgUtils.cpp I don't think this comment is needed. ::: modules/libpref/src/Preferences.cpp @@ +941,3 @@ > return rv; > + } else { > + gDirty = false; Please don't use else after return. You can revert this hunk completely.

Like so?

Comment on attachment 832357 [details] [diff] [review] safewrite_mozilla-central.patch [checkin: comment 33] Review of attachment 832357 [details] [diff] [review]: ----------------------------------------------------------------- Yes, thanks!

Irving, please accept again -- I had a unnecessary return in there which throws a error.

Can all this land at the same time or does the m-c patch need to land first?

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #31) > Can all this land at the same time or does the m-c patch need to land first? The patches are independent.

Backed out from c-c for xpcshell crashes. https://hg.mozilla.org/comm-central/rev/933b83459851 https://tbpl.mozilla.org/php/getParsedLog.php?id=30770789&tree=Thunderbird-Trunk

So this seems to be crashing in http://hg.mozilla.org/comm-central/file/ffe30ba8cd1b/mailnews/base/util/nsMsgMailNewsUrl.cpp#l871 after your change to nsMsgSaveAsListener::SetupMsgWriteStream . Can you please check it? I couldn't see an obvious reason for the problem. Maybe something with the aFile already existing? Is the comment in nsMsgSaveAsListener::SetupMsgWriteStream still valid?

This patch introduces a problem.

I split up the patch into 4 hunks according to the files they modify (the hunks are independent). 3 hunks, nsMsgFilterService.patch, nsMsgAccountManager.patch, nsPop3Protocol.patch have not changed at all, and can be re-applied. The last hunk is in nsMsgMailNewsUrl-wip.patch, and introduces a problem, namely the tests _tests/xpcshell/mailnews/imap/test/unit/test_bug460636.js _tests/xpcshell/mailnews/imap/test/unit/test_imapChunks.js _tests/xpcshell/mailnews/imap/test/unit/test_imapChunks.js _tests/xpcshell/mailnews/imap/test/unit/test_saveTemplate.js fail. I put in a bit of debugging output to see what's going on: Running the test with TEST_PATH=mailnews/imap/test/unit/test_bug460636.js make xpcshell-tests -C ../objdir-tb/ Yields the debugging output: pre-setup IMAP Pump adding message updating TEST-INFO | (xpcshell/head.js) | test _async_driver finished (2) TEST-INFO | (xpcshell/head.js) | test _async_driver pending (2) saving to disk /tmp/xpcshell/xpcshellprofile/ImapMail/bug460636.eml false saving to disk done TEST-INFO | (xpcshell/head.js) | test _async_driver finished (2) !! nsMsgSaveAsListener::SetupMsgWriteStream !! file exists = 0 !! file exists = 0 !! writing to /tmp/xpcshell/xpcshellprofile/ImapMail/bug460636.eml !! opening was successful, 0 !! file exists = 1 !! nsMsgSaveAsListener::OnStopRequest: closing file !! file closed !! getting safeStream successful !! finish successful TEST-INFO | (xpcshell/head.js) | test _async_driver pending (2) 2013-11-20 22:26:48 test.test INFO [Context: test.test:1 state: finished] Finished test: setup 2013-11-20 22:26:48 test.test INFO [Context: test.test:2 state: started] Starting test: checkSavedMessage loading test string ... /mnt/data/daten/Master/home/Downloads/mozilla/objdir-tb/mozilla/_tests/xpcshell/mailnews/data/bug460636 loading written string ... /tmp/xpcshell/xpcshellprofile/ImapMail/bug460636.eml false In the javascript setup function of test_bug460636.js, the file does not exist, but is commanded to be created. At the end of it, it does not exist yet (because it is asynchronous, I think). Then nsMsgSaveAsListener::SetupMsgWriteStream is finally called, the file at first does not exist, then it is successfully created on the disk. But when checkSavedMessage of test_bug460636.js is run, the file has vanished!

I think I solved it. The stream was closed, which causes Flush() in Finish() to fail, which deletes the temporary file and aborts. Ryan, please apply the patches nsMsgFilterService.patch, nsMsgAccountManager.patch, nsPop3Protocol.patch (they have not changed, so I don't think review is necessary). Irving, please review my new patch (nsMsgMailNewsUrl.patch). I am trying without the aFile->Remove() call, I think it should work.

https://hg.mozilla.org/comm-central/rev/9b1106f87c8c https://hg.mozilla.org/comm-central/rev/7b725655e48f https://hg.mozilla.org/comm-central/rev/25b09e25e7de Please make sure that future patches have proper commit information included. https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment on attachment 8335617 [details] [diff] [review] nsMsgMailNewsUrl.patch Review of attachment 8335617 [details] [diff] [review]: ----------------------------------------------------------------- This regresses bug 328027 - loading the example message attached to that bug, and attempting to save the 0-length attachment, does not create an empty file.

Johannes, you may be interested in the new NS_NewAtomicFileOutputStream from bug 928321. Maybe it could be used for some of the cases here.

(In reply to :Irving Reid from comment #45) > Comment on attachment 8335617 [details] [diff] [review] > nsMsgMailNewsUrl.patch > > Review of attachment 8335617 [details] [diff] [review]: > ----------------------------------------------------------------- > > This regresses bug 328027 - loading the example message attached to that > bug, and attempting to save the 0-length attachment, does not create an > empty file. Irving, is the warning "failed to flush buffered data! possible dataloss" being triggered in the empty file case? In that case, flush after not writing anything returning a failure would be the cause of the problem. http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsBufferedStreams.cpp#653 I can't see anything particular in nsSafeFileOutputStream code that prohibits the temporary file from being empty.

Comment on attachment 8335617 [details] [diff] [review] nsMsgMailNewsUrl.patch Review of attachment 8335617 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/util/nsMsgMailNewsUrl.cpp @@ +925,1 @@ > aFile, -1, 0666); I spent some time tracing through the code while I was reviewing, now I'm trying to remember what I saw. I think the problem is that we never get to this code for 0-length files, because we don't send a "data received" event to this listener if there is nothing in the file. The old fix for bug 328027 was kind of a hack - as far as I can tell, we create a file in nsMessenger::SaveAs to handle the 0-length case, then remove it and create a new file here if there really was data. It would make me happy if you could figure out a better way...

Sorry, I am currently not developing. I think the patch is ok, but it unearthed another (hopefully rare) problem. This requires more digging for its origin, and could be resolved in another bug?

Yes, if you wish we can split off the last file nsMsgMailNewsUrl.patch to a new bug to not drag this bug indefinitelly. We just need to copy enough context. I'll do it if you wish. It is a pity we loose a developer again.

Good luck and thanks for all the fish (I'll linger around)

Thanks. Assigning back to Johannes to leave the credit where it is due. I close this bug and the remaining issue can be finished in bug 1169252. Assuming from the date the patches here were landed (comment 44), this was fixed in TB28.