The return value of FileImpl::InternalFlush() should be checked or else file corruption will occur

RESOLVED FIXED

Status

()

Core
XPCOM
--
critical
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

({dataloss})

1.8 Branch
dataloss
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

3.64 KB, patch
Benjamin Smedberg
: review+
Benjamin Smedberg
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.15) Gecko/20080702 Ubuntu/8.04 (hardy) Firefox/2.0.0.15 Kazehakase/0.5.4
Build Identifier: 

The return value of FileImpl::InternalFlush() should be checked or else file corruption will occur.

Possible scenario

1. some data wrote by FileImpl::Write().
2. called FileImpl:Seek()
3. FileImpl::InternalFlush() was called from FileImpl:Seek().
4. some buffer were written and some were not written since PR_Write() was failed.
5. called FileImpl::Write()
6. file data became corruption since all buffer remained even if InternalFlush() was failed.


Reproducible: Always

Steps to Reproduce:
1.
2.
3.
(Assignee)

Comment 1

10 years ago
Created attachment 334443 [details] [diff] [review]
Proposed patch

Check the return value of InternalFlush and cleanup buffer if InternalFlush is failed.
Attachment #334443 - Flags: superreview?(benjamin)
Attachment #334443 - Flags: review?(brendan)
(Assignee)

Comment 2

10 years ago
I think the return value of seek() should be checked as well but seek is invoked here and there...
(Assignee)

Updated

9 years ago
Severity: normal → critical
Keywords: dataloss
Version: unspecified → 1.8 Branch

Comment 3

9 years ago
Comment on attachment 334443 [details] [diff] [review]
Proposed patch

I'd feel happier if you just removed the clients of this code so we could stop building it in all configurations.
Attachment #334443 - Flags: superreview?(benjamin)
Attachment #334443 - Flags: superreview+
Attachment #334443 - Flags: review?(brendan)
Attachment #334443 - Flags: review+
(Assignee)

Updated

9 years ago
Attachment #334443 - Flags: approval1.8.1.next?
David, Dan: does Thunderbird use this code?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → poincare

Comment 5

9 years ago
2.0x most likely does, 3.0x does not.
I'm somewhat inclined to take this, because it would appear to be a serious bug.  However, I'm a bit leery of the risk that are other callers of this code that aren't expecting errors in these circumstances and might behave poorly (eg crash; stomp memory) if they crop up.  It's hard to figure out how to balance these risks without knowing more:

* has this ever been sighted in the wild in Thunderbird?
* can we get steps to reproduce?
* what error-handling code paths are taken by other in-tree callers?
Keywords: qawanted
Comment on attachment 334443 [details] [diff] [review]
Proposed patch

Clearing approval request.

Please renominate when the questions from comment 6 are addressed.
Attachment #334443 - Flags: approval1.8.1.next?

Comment 8

5 years ago
I assume Wayne cc'd me for a comment.

Based on comment 5, this fix is for versions of the code that are no longer shipping, so I assume that means the bug is now either WONTFIX or INVALID.

(It modifies files in mozilla/xpcom/obsolete)
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.