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

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

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.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proposed patch

Check the return value of InternalFlush and cleanup buffer if InternalFlush is failed.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

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

Comment 3

[Benjamin Smedberg]

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.

Comment 4

[Daniel Veditz [:dveditz]]

David, Dan: does Thunderbird use this code?

Comment 5

[David :Bienvenu]

2.0x most likely does, 3.0x does not.

Comment 6

[Dan Mosedale (:dmosedale, :dmose)]

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?

Comment 7

[Samuel Sidler (old account; do not CC)]

Comment on attachment 334443 [details] [diff] [review]
Proposed patch

Clearing approval request.

Please renominate when the questions from comment 6 are addressed.

Comment 8

[Kent James (:rkent)]

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)

Comment 9

[Joshua Mitchell (Inactive)]

Issue is Resolved - removing QA-Wanted Keywords - QA-Wanted query clean-up task