939535 - Failure to check the return value of PR_Close() in CloseFile() (mozilla/ipc/glue/FileDescriptorUtils.cpp)

Status: NEW

(Core :: IPC, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: (Work-inProgress) Checking PR_Close() return value.

This is part of an effort to make mozilla software rock-solid.
(See bug 936990 (META) The return value of PR_Close() should be checked (like any other system calls).

In CloseFile(), the return value of PR_Close() is not checked.
It is not possible to return the error value since the function CloseFile() is void.
However, a failure to close a file properly usually indicates that the file system
is in a serious error condition such as
 - transient and/or semi-permanent network error during a remote file mount (CIFS, NFS, etc.), or
 - malfunctioning storage device (such as worn out SD, etc.)

So at least the error should be caught and error/warning be issued.

As for warning, I am not sure if fprintf(stderr,"ERROR: PR_Close() returned 0x%08x\n", prrc) or some such is better than MOZ_ASSERT() which does not show the error value. 
The proper error/warning is most useful for real-world user who experience such
errors during TB/FF operation.
So I wonder if using fprintf() is better than MOZ_ASSERT(). Suggestions are welcome.

I am attaching a patch (against the file in comm-central.)

TIA

PS: I can't help noticing the similarity to the code being fixed in
bug 938687

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 833467 [details] [diff] [review]
(Work-inProgress) Checking PR_Close() return value.

Review of attachment 833467 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking into this!

::: ipc/glue/FileDescriptorUtils.cpp
@@ +1,2 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> + * vim: sw=2 ts=8 et :

If you're adding a mode line it needs to be the one defined here:

https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line

@@ +71,5 @@
>    if (fd) {
> +    PRStatus prrc;
> +    prrc = PR_Close(fd);
> +    if (prrc != PR_SUCCESS) {
> +      MOZ_ASSERT(false, "PR_Close() failed.");

In general I'd much prefer that we follow this pattern:

  DebugOnly<PRStatus> status = PR_Close(fd);
  MOZ_ASSERT(status == PR_SUCCESS);

However, with this case in particular I'm not very confident that it will help find real bugs. The cases where this could fail are pretty rare and most people who experience that failure will not be running debug builds (our MOZ_ASSERT compiles to nothing on release builds). You could print to stderr but most users probably have no idea what that means. You could print to the Web Console but a) most folks will not ever see it, and b) no one is going to have any idea what to do with such a message. I don't even think I would know what to do if I saw such a message.

My only thought is that you could MOZ_CRASH if this fails. That way at least we'd see it in our crash stats. But I don't know if that would provide enough information to us to figure out what's going wrong and I still don't know how we'd handle it...

Comment 2

[ISHIKAWA, Chiaki]

(In reply to ben turner [:bent] (needinfo? encouraged) from comment #1)
> Comment on attachment 833467 [details] [diff] [review]
> (Work-inProgress) Checking PR_Close() return value.
> 
> Review of attachment 833467 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for looking into this!
> 
> ::: ipc/glue/FileDescriptorUtils.cpp
> @@ +1,2 @@
> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> > + * vim: sw=2 ts=8 et :
> 
> If you're adding a mode line it needs to be the one defined here:
> 
> https://developer.mozilla.org/en-US/docs/Developer_Guide/
> Coding_Style#Mode_Line
> 

I will use this. I am afraid that I copied the said mode line from an existing file in
comm-central tree, and so if I can find the one, I would file a bugzilla to fix this, but
as a matter of fact, there are MANY files that don't seem to have such mode line in the first place.
Hmm... I wonder what I should do. But that is another issue. I digress.


> @@ +71,5 @@
> >    if (fd) {
> > +    PRStatus prrc;
> > +    prrc = PR_Close(fd);
> > +    if (prrc != PR_SUCCESS) {
> > +      MOZ_ASSERT(false, "PR_Close() failed.");
> 
> In general I'd much prefer that we follow this pattern:
> 
>   DebugOnly<PRStatus> status = PR_Close(fd);
>   MOZ_ASSERT(status == PR_SUCCESS);
> 
> However, with this case in particular I'm not very confident that it will
> help find real bugs. The cases where this could fail are pretty rare and
> most people who experience that failure will not be running debug builds
> (our MOZ_ASSERT compiles to nothing on release builds). You could print to
> stderr but most users probably have no idea what that means. You could print
> to the Web Console but a) most folks will not ever see it, and b) no one is
> going to have any idea what to do with such a message. I don't even think I
> would know what to do if I saw such a message.
> 
> My only thought is that you could MOZ_CRASH if this fails. That way at least
> we'd see it in our crash stats. But I don't know if that would provide
> enough information to us to figure out what's going wrong and I still don't
> know how we'd handle it...

Thank you for your comment.

In bug 936
You have brought up a valid point.

The people who may see these errors are possibly experiencing network errors (causing
network file mount fail, e.g., or real storage device errors (such as worn out SD, USB memory stick, etc.). The errors *DO* happen.

When I looked for proper macros, there don't seem to be appropriate one.
MOZ_ASSERT() turn out to be noop in optimized build which people use.
So that does not help to catch these rare but real bugs in the field.
(But a good indicator of a mail client (I am debugging some issues related to TB)
is that one handles error conditions gracefully. I think TB leaves much room for improvement in this regard.)

In bug 936990 (META) The return value of PR_Close() should be checked (like any other system calls),
I would like to summarize the basic guideline for handling these hard-to-handle errors based
on the fixing of a dozen bug. This bugzilla entry is one of the first 8 or so bugzilla entries blocking 93690.
So your suggestion of the template is a valuable one, but at the same time
as you suggest MOZ_ASSERT() leaves a lot to be desired.

Maybe fprintf() even in the release build might be a way to go. There was a suggestion of
putting this into console. 
As you say, "PR_Close() failed" does not mean much to ordinary users.
*BUT*, at least, after their report, developers of mozilla software can learn the failure of such
basic routines and then figure out something is wrong with users network or storage media, or filesystem that needs fixing, etc.
There are about hundred or so places where the return value of close, PR_Close(), etc. are not checked.
If I add a dozen or so bytes (say 40 bytes including the short string), the fix would not amount to 4K or 8K. I would opt for a rock-solid client by using 4K - 8K bytes.

But let me think it over about how best to
- what kind of error actions should be taken;
  a) simply return an error value to the upper layer. (this is impossible here. And there are many
  places where PR_Close() is called in void function.
  b) show the error somehow to the user console, built-in debug console (which may add another 4K or so), etc. in a manner USEFUL BOTH to the developer during testing AND end users who are using release build.

If we can simulate all the I/O error conditions and create test programs for covering ALL the error handling paths, we can forget about the showing of such errors to the end user. But this is not the case.
There will bound to be error conditions unforeseen, and creating test paths to cover all the
error handling paths in mozilla source code is close to impossible. I notice that
|make mozmill| do not report errors even when some routines during tests throw javascript EXCEPTION error all the way to the top-level: still |make mozmill| checking only the final condition of tests
report success !

It is indeed a hard to deal with issue, but I think we at least should report the error even for the purpose of letting the user that their network, hardware, and/or file system is in serious trouble ASAP.

Again, thank you for your comment, and let me think over what should be done in conjunction with
the feedback from the early dozen bugs that block 936990.

TIA

Comment 3

[Benjamin Smedberg]

I'm not sure that *in general* we're fixing a problem that needs to be fixed. If close fails on a pipe or a readonly file, there isn't any real problem.

The only real problem is if close fails on a file that we're writing, and then we make some important decision afterwards, such as marking a "we cleanly wrote to disk" flag somewhere. These are pretty unusual. For most code that needs to reliably write to disk, we're either using something like sqlite with journaling/recovery, or use the safe file output stream which only renames the file if it was written successfully.

So we should make sure that the safe streams operate correctly, but most of these other cases should not issue any warnings.

Also, we should not write to stdout/stderr in release builds in pretty much any circumstance.

Comment 4

[ISHIKAWA, Chiaki]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #3)
> I'm not sure that *in general* we're fixing a problem that needs to be
> fixed. If close fails on a pipe or a readonly file, there isn't any real
> problem.

I agree on this readonly file angle.
(The problem in existing code, though, is the lack of proper comment. Someone who has worked on the code base for prolonged time may know already that the particular code is for readonly file,
and failure on close really doesn't matter, but newcomer who is looking for problems may not know this
and is puzzled at the lack of checking. *AT LEAST*, we should comment such code IMHO.
For example, as in "Bug 939531 - Failure to the check the return value of PR_Close() in ~FileAutoCloser() (js/xpconnect/loader/mozJSComponentLoader.cpp)").
If we put "We can ignore the return value. The file is opened for readonly." or some such,
it will help future debuggers IMHO.

> 
> The only real problem is if close fails on a file that we're writing, and
> then we make some important decision afterwards, such as marking a "we
> cleanly wrote to disk" flag somewhere. These are pretty unusual. For most
> code that needs to reliably write to disk, we're either using something like
> sqlite with journaling/recovery, or use the safe file output stream which
> only renames the file if it was written successfully.

Yes, I noticed sqlite seems to handle I/O very carefully (and it has built-in test harness to
simulate I/O errors). There do seem to be places where safe file output stream is used.
But a few places where I noticed problems didn't use such facility and
was returning a bad error (or failed to return error values at all.).

> So we should make sure that the safe streams operate correctly, but most of
> these other cases should not issue any warnings.

OK. 
What I want is an output for the developers to catch low-level errors in case
users experience dataloss like situations repeatedly in user's enviornment.
Such clue helps diagnose the problem.
Basically debugging help and to catch improperly unhandled cases during testing.

> Also, we should not write to stdout/stderr in release builds in pretty much
> any circumstance.

Right, I am looking for a method that works reliably and usefully for developers and users alike.
Also, now I recall it seems stdout/stderr is not useful at all for Windows build and typical windows user usage. (I am using linux version mostly but I do use windows version occasionally. Somehow I experience more problems under linux environment and so my bugzilla entries are mostly found on linux first. Under linux, stdout, stderr are used extensively for DEBUG BUGILD.)

Thank you again for your comment.

Comment 5

[ISHIKAWA, Chiaki]

Attachment: (Work-in-Progress. Take 2) Checking PR_Close() return value.

I have updated my patch.
I added fprintf() statement within #if DEBUG/#endif so that we get something shown for DEBUG BUILD (for test run)
Maybe I should use something like 
MOZ_MTLOG as suggested in https://bugzilla.mozilla.org/show_bug.cgi?id=939530#c2
(But that may need to include additional header file, etc.)

Here my standpoint is to make sure that PR_Close() is succeeding even under
dire conditions (this seems to be the assumption of the original code since
no checking was done at all, and I hope this code is for file or stream that was opened for read-only. Such a comment, if it is the case, would have been welcome.)

TIA

Comment 6

[ISHIKAWA, Chiaki]

I am afraid that I hit [SAVE Changes] earlier than I intended.
The code as it stands doesn't compile.
So back to the drawing board.

Comment 7

[ISHIKAWA, Chiaki]

Attachment: (Work-in-Progress Patch Take 3) Checking PR_Close() return value.

DebugOnly<PRStatus> caused the following error when I tried to print it
to stderr inside: #if DEBUG/#endif
/REF-COMM-CENTRAL/comm-central/mozilla/ipc/glue/FileDescriptorUtils.cpp: In member function 'void mozilla::ipc::CloseFileRunnable::CloseFile()':
/REF-COMM-CENTRAL/comm-central/mozilla/ipc/glue/FileDescriptorUtils.cpp:74:64: error: cannot pass objects of non-trivially-copyable type 'class mozilla::DebugOnly<PRStatus>' through '...'
     fprintf(stderr,"PR_Close failed. status = 0x%08x\n", status);
                                                                ^
/REF-COMM-CENTRAL/comm-central/mozilla/ipc/glue/FileDescriptorUtils.cpp:74:64: warning: format '%x' expects argument of type 'unsigned int', but argument 3 has type 'mozilla::DebugOnly<PRStatus>' [-Wformat=]

So I had to use the original bare PRStatus.

Maybe one of those logging macros *may* be a good idea.
However, they also output to files, I wonder if that fits the idea of
warning/errors in release build. Maybe it is OK when the user explicitly requests such error/warning to a specified file?

*In general* (not specific to this place),
it is rather strange that there do not seem to be handy macros that
can be useful both to developers and users to track down the issues
when a user is experiencing serious I/O errors.
(Not that I am saying that this part is causing miserable failures by failing to check the I/O errors. I am doing a cleaning sweep based on my observation in
bug 936990. I am glad I am doing it since now the lack of proper macros or widely accepted idiom may be one reason why TB (maybe FF, too) fails to produce
user-friendly useful error/warning in a manner useful both to the developer and users.

TIA

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8337260 [details] [diff] [review]
(Work-in-Progress Patch Take 3) Checking PR_Close() return value.

Review of attachment 8337260 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/FileDescriptorUtils.cpp
@@ +70,5 @@
>    if (fd) {
> +    PRStatus status = PR_Close(fd);
> +#if DEBUG
> +    // Print out the value for debugging just in case.
> +    fprintf(stderr,"PR_Close failed. status = 0x%08x\n", status);

It looks to me like this will always be called, even if it succeeds...

@@ +72,5 @@
> +#if DEBUG
> +    // Print out the value for debugging just in case.
> +    fprintf(stderr,"PR_Close failed. status = 0x%08x\n", status);
> +#endif
> +    MOZ_ASSERT(status == PR_SUCCESS);

I think you can basically do this with a simple helper macro that we have:

  MOZ_ALWAYS_TRUE(PR_Close(fd) == PR_SUCCESS);

That doesn't require any additional DEBUG guards or anything.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 10

[Suhaib Mujahid [:suhaib]]

Sorry, there was a problem with the detection of inactive users. I'm reverting the change.