Opening files for writing can destroy data on full disk in nsMsgMailNewsUrl.cpp

NEW
Unassigned

Status

defect
--
critical
4 years ago
2 months ago

People

(Reporter: aceman, Unassigned, NeedInfo)

Tracking

({dataloss})

Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird_esr60 affected, thunderbird67 affected, thunderbird68 affected)

Details

(Whiteboard: [patchlove][needs new assignee])

Attachments

(1 attachment)

Reporter

Description

4 years ago
+++ 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, 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 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
Reporter

Comment 1

4 years ago
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...
Reporter

Updated

4 years ago
Whiteboard: [patchlove][needs new assignee]

Updated

2 months ago
Duplicate of this bug: 1545725

Good time to revisit?

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

Flags: needinfo?(ishikawa)
Flags: needinfo?(acelists)
See Also: → 328027

Comment 4

2 months ago

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

Comment 5

2 months ago

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

Comment 6

2 months ago

(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

Flags: needinfo?(ishikawa)

Updated

2 months ago
See Also: → 1545725
You need to log in before you can comment on or make changes to this bug.