402460 - CreateUnique is very slow and returns bogus error for write-protected directory

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[[e-mail]]

Trying to CreateUnique into a write-protected directory fails very slowly, as it attempts to create 9999 files before giving up with NS_ERROR_FILE_TOO_BIG. This is because Create always fails with NS_ERROR_FILE_ALREADY_EXISTS although NS_ERROR_FILE_ACCESS_DENIED is the correct error to use in this case.

This bites SeaMonkey branch builds because bug 375102 changed the behaviour of rdfXMLFlush to use a safe output stream which tries to CreateUnique the file. It also causes error returns when there weren't before but we work around that.

Comment 1

[[e-mail]]

Attachment: Proposed patch

Simply propagating the return value from Open fixes the bug.

I also noticed the same error in OS/2, so I copied code from OpenNSPRFileDesc.

Comment 2

[Benjamin Smedberg]

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

This looks like it is re-introducing the insecurity fixed by bug 43314, where the leak was introduced intentionally (at least as far as I can tell). Please re-request review if this is not the case.

Comment 3

[[e-mail]]

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

I'm not sure what you mean by the leak:

>-        if (!file) return NS_ERROR_FILE_ALREADY_EXISTS;
The error code may not correctly describe the actual failure.
>+        if (!file)
>+            return NS_ErrorAccordingToNSPR();
The fix is to return the correct error code.
>         PR_Close(file);
>         return NS_OK;
Either way the file is closed.

>-        OpenFile(mResolvedPath,
>-                 PR_RDONLY | PR_CREATE_FILE | PR_APPEND | PR_EXCL, attributes,
>-                 &file);
>-        if (!file) return NS_ERROR_FILE_ALREADY_EXISTS;
>-
>-        PR_Close(file);
>-        return NS_OK;
This closes the file on success but again the error code might be bogus.
>+        rv = OpenFile(mResolvedPath,
>+                      PR_RDONLY | PR_CREATE_FILE | PR_APPEND | PR_EXCL, attributes,
>+                      &file);
>+        if (file)
>+            PR_Close(file);
>+        return rv;
The fix is to preseve the return code while closing the file on success.

Comment 4

[Benjamin Smedberg]

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

Oh, I misread the diff.

Comment 5

[[e-mail]]

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

Looking for this on branch as well to resolve the dependent bug.

Comment 6

[[e-mail]]

Fix checked in.

Comment 7

[Daniel Veditz [:dveditz]]

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

approved for 1.8.1.10, a=dveditz for release-drivers

must land _today_ or wait for 1.8.1.11

Comment 8

[[e-mail]]

Fix checked in to the branch.

Comment 9

[Al Billings [:abillings - ex-MoCo]]

Can the reporter check with the Firefox 2.0.0.10 release candidate (http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2.0.0.10-candidates/rc1/) and verify if this is fixed or does it only express in Seamonkey?

Comment 10

[[e-mail]]

(In reply to comment #9)
>Can the reporter check with the Firefox 2.0.0.10 release candidate
>(http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2.0.0.10-candidates/rc1/)
>and verify if this is fixed or does it only express in Seamonkey?
The problem is only readily reproducible in SeaMonkey, but I have tried the latest SeaMonkey 1.1.7pre nightly and the problem does not exhibit itself.

Comment 11

[Al Billings [:abillings - ex-MoCo]]

dveditz, is that enough for us to mark this as verified for branch?

Comment 12

[Boris Zbarsky [:bzbarsky]]

This seems to have caused bug 420310.