1169252 - Opening (non-msf/non-email) files for writing can destroy data on full disk in nsMsgMailNewsUrl.cpp

Status: NEW

(MailNews Core :: Database, defect)

Description

[:aceman]

+++ This bug was initially created as a clone of Bug #912465 +++

(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, but 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 sqlite or mork (msf) databases are written, the case might be different.
This bug should identify and fix code not safe against a full disk.

------------------------------------------
In this bug we focus on the remaining problem in the file nsMsgMailNewsUrl.cpp

Comment 1

[:aceman]

Attachment: nsMsgMailNewsUrl-WIP.patch

Carrying over remaining patch from bug 912465.

It has accumulated these review comments that need to be fixed:

 :Irving Reid (No longer working on Firefox) 2013-12-03 19:50:38 CET

Comment on attachment 8335617 [details] [diff] [review] [diff] [review]
nsMsgMailNewsUrl.patch

Review of attachment 8335617 [details] [diff] [review] [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 Buchner 2013-12-24 01:25:49 CET

(In reply to :Irving Reid from comment #45)
> Comment on attachment 8335617 [details] [diff] [review] [diff] [review]
> nsMsgMailNewsUrl.patch
> 
> Review of attachment 8335617 [details] [diff] [review] [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.

 :Irving Reid (No longer working on Firefox) 2013-12-30 04:28:20 CET

Comment on attachment 8335617 [details] [diff] [review] [diff] [review]
nsMsgMailNewsUrl.patch

Review of attachment 8335617 [details] [diff] [review] [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...

Comment 3

[Wayne Mery (:wsmwk)]

Good time to revisit?

Other disk full reports https://mzl.la/2UnKcY9 - a couple against import

Comment 4

[klaus]

it seems the safe writing is not implemented for inbox.msf - see also https://bugzilla.mozilla.org/show_bug.cgi?id=1545725

Comment 5

[klaus]

comment 4 was for an inbox on imap, if this should be of relevance

Comment 6

[ISHIKAWA, Chiaki]

(In reply to Wayne Mery (:wsmwk) from comment #3)

Good time to revisit?

Other disk full reports https://mzl.la/2UnKcY9 - a couple against import

I thought this was fixed years ago, but it was not???

I will look into this starting on Saturday. We have 10 straight holidays in Japan, and I have returned from
an early trip and would stay home more or less and do some serious catching up.

TIA

Comment 7

[:aceman]

This bug is specifically not for msf files (mork) and also not the email data (e.g. Inbox) as those can't be handled by the MsgNewSafeBufferedFileOutputStream code as it duplicates the files for a moment and that can't be done for these multi-gigabyte files.
Also I wouldn't cover import problems here as it is handled in a different module. Good that those have their own bugs.