438316 - Safe File Outputstreams conflict with file monitoring tools

Status: NEW

(Core :: XPCOM, defect)

Description

[Dave Townsend [:mossop]]

The way a safe file output stream works causes problems when there are external processes monitoring file accesses, like the majority of resident AV tools and something like TortoiseSVN. In bug 431065 I have isolated faults with the registration of add-ons during startup down to the use of safe file outputstreams on the same file repeatedly in a short space of time.

The basic problem is that when the stream is finished and attempts to move the temporary copy to the real file, the real file may be open by an external process. Bug 375778 appears to have made matters worse here though this issue still occurs on the 1.8 branch.

Using process monitor it seems that these monitoring tools open the files with shared read, write and delete access which would allow us to open and write to the file directly, but apparently not copy a file over the top.

A couple of ideas. When the stream is opened should we obtain some kind of lock on the real file so that external processes can't start accessing it afterwards? Is there a better way to move the temporary copy into place that doesn't hit this problem?

Jim, do you have any thoughts?

Comment 1

[Jim Mathies [:jimm]]

Are you positive Norton is part of the problem or could it just be TortoiseSVN? The reason I ask is I've had stability problems in explorer due to TortoiseSVN on my system, I finally had to uninstall it. After that a lot of misc. problems I was having went away.

Comment 2

[Dave Townsend [:mossop]]

(In reply to comment #1)
> Are you positive Norton is part of the problem or could it just be TortoiseSVN?
> The reason I ask is I've had stability problems in explorer due to TortoiseSVN
> on my system, I finally had to uninstall it. After that a lot of misc. problems
> I was having went away.

I never mentioned Norton. I don't think it is just one application that causes the issue. I can reproduce reliably with Tortoise SVN, bug 430579 was caused by Avira, bug 437914 shows multiple AV tools, but not Tortoise SVN.

Comment 3

[Jim Mathies [:jimm]]

So basically, in a short period of time, we are doing something like this - 

open temp file
write to it
close it
move it to destination
open temp file
write to it
close it
move it to destination

and on that last move, we get an access denied error when we call MoveFileEx? If that's the case, I don't see a way to lock the destination, since that would just get in the way of the move. Maybe going back to the original method of moving the destination to a backup in the same directory, and then calling MoveFile on the source. (Apparently there were no issues with a missing file for a brief instant in 2.0?) You posted that the problem was in 2.0 as well but less common, so this isn't a 100% fix, but it sounds like it will get us back to 2.0 level on the issue, which is better than leaving it as is.

We could also relnote this, I believe anti-virus vendors usually provide directory exclusion lists. Again though, not a pretty fix at all.

I think you suggested migrating to sqlite? I like that solution the best. :)

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> So basically, in a short period of time, we are doing something like this - 
> 
> open temp file
> write to it
> close it
> move it to destination
> open temp file
> write to it
> close it
> move it to destination

Yep exactly

> We could also relnote this, I believe anti-virus vendors usually provide
> directory exclusion lists. Again though, not a pretty fix at all.

That might be what Avira have decided to do, maybe we ask AV vendors to exclude profile folders from scans but I guess that isn't a good idea from the point of view of malicious extensions

> I think you suggested migrating to sqlite? I like that solution the best. :)

Well the extensions.rdf file is going that way, but not soon enough. But this also hits other files, extensions.ini (which might be going away altogether) and extensions.cache (needs to stay) are other cases. I opened this bug to be less specific than the EM cases in the hope there might be a solution that helps anywhere we might hit this.