Last Comment Bug 300692 - Win32 nsLocalFile::Create returns not found if no access and a directory hierarchy is needed
: Win32 nsLocalFile::Create returns not found if no access and a directory hier...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: x86 Windows XP
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
Depends on: 429484
Blocks: 682571
  Show dependency treegraph
 
Reported: 2005-07-13 15:54 PDT by Robert Strong [:rstrong] (use needinfo to contact me)
Modified: 2011-09-22 17:45 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for wrong error code when access denied on Create operations (3.28 KB, patch)
2011-08-27 08:55 PDT, Brian R. Bondy [:bbondy]
benjamin: review+
Details | Diff | Splinter Review
Patch for wrong error code when access denied on Create operations v2. (3.28 KB, patch)
2011-08-30 14:58 PDT, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review

Description Robert Strong [:rstrong] (use needinfo to contact me) 2005-07-13 15:54:41 PDT
Here it is:
http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileWin.cpp#762

This appears to affect nsIFile.createUnique in that if access is denied it
thinks a file already exists and then tries to create the file 10,000 times
before giving up.
Comment 1 Robert Strong [:rstrong] (use needinfo to contact me) 2005-07-13 15:57:20 PDT
This came to light in bug 300265
Comment 2 James Ross 2005-07-13 16:14:28 PDT
FWIW, I changed the return to use ConvertWinError, and createUnique started
behaving much more sensibly (though I found one odd side-effect[1]), but
obviously all call sites would need to be carefully checked for how they handle
the return code.

[1] If the "Access Privileges Test" file exists when createUnique tries to test
it, and the user has permission to delete it... it deletes it! I assume this is
because it got an unexpected error, and assumed it had worked, and that it was
ok to delete the file. Oops.
Comment 3 Doug Turner (:dougt) 2005-07-14 10:16:11 PDT
line 763 seams very broken.  Don't we want something like:

PRFileDesc* file = PR_Open(mResolvedPath.get(), PR_RDONLY | PR_CREATE_FILE |
PR_APPEND | PR_EXCL, attributes);

if (file) {
 PR_Close(file);
 return NS_ERROR_FILE_ALREADY_EXISTS;
}
Comment 4 Robert Strong [:rstrong] (use needinfo to contact me) 2005-07-14 17:28:18 PDT
I would think we'd want the return value from ConvertWinError as is used when
creating a directory since this won't properly handle when access is denied. As
was pointed out it appears that not all call sites properly handle a return of a
value other than NS_ERROR_FILE_ALREADY_EXISTS.
Comment 5 Doug Turner (:dougt) 2007-10-08 16:14:02 PDT
mass reassigning to nobody.
Comment 6 Brian R. Bondy [:bbondy] 2011-08-26 21:51:53 PDT
I tested this and the only time it returns that value is if the file does already exist. I think this is expected behaviour and this bug was fixed somewhere along the line. I'd like to nominate that we resolve this as invalid.
Comment 7 Robert Strong [:rstrong] (use needinfo to contact me) 2011-08-26 22:02:07 PDT
It was returning NS_ERROR_FILE_ALREADY_EXISTS when failing to create the file. For example, in bug 300265 (see comment #1) this was happening when trying to write to a read only share. If this has been fixed since this bug was filed then YAY!
Comment 8 Brian R. Bondy [:bbondy] 2011-08-27 04:51:54 PDT
ah ok I'll check, thanks for the info.
Comment 9 Brian R. Bondy [:bbondy] 2011-08-27 06:01:12 PDT
For writing a file to a read only share we return:
NS_ERROR_FILE_ACCESS_DENIED which is correct.

When we create a file with a directory hierarchy that doesn't exist though, we usually create the whole directory hierarchy.

For writing a file to a read only share that has a directory that doesn't exist, this returns NS_ERROR_FILE_NOT_FOUND.  So in that case NS_ERROR_FILE_ACCESS_DENIED should be used.
Comment 10 Brian R. Bondy [:bbondy] 2011-08-27 08:55:07 PDT
Created attachment 556265 [details] [diff] [review]
Patch for wrong error code when access denied on Create operations

So the problem is, if you have an empty read only share:
\\computername\myshare

And you try to do something like create a file inside a subfolder:
\\computername\myshare\1\2\3\file.txt

Then the first create directory returns access denied, which keeps going trying to create the other directories.
But the others return file not found and return at that point.
The real error is that access is denied though not that the directory doesn't exist.
Comment 11 Brian R. Bondy [:bbondy] 2011-08-27 08:56:49 PDT
Example code you can run to reproduce:

var file = Components.classes['@mozilla.org/file/local;1'].createInstance(Components.interfaces.nsILocalFile);
file.initWithPath("\\\\rhesus\\readonlyshare\\1\\2\\3\\test.txt"); 
file.create(Components.interfaces.nsIFile.NORMAL_FILE_TYPE, 0777);
Comment 12 Benjamin Smedberg [:bsmedberg] 2011-08-30 14:18:46 PDT
Comment on attachment 556265 [details] [diff] [review]
Patch for wrong error code when access denied on Create operations

>diff --git a/xpcom/io/nsLocalFileWin.cpp b/xpcom/io/nsLocalFileWin.cpp

>+    nsresult directoryCreateError = 0;

This should be NS_OK;

>     if (slash)
>     {
>         // skip the first '\\'
>         ++slash;
>         slash = wcschr(slash, L'\\');
> 
>         while (slash)
>         {
>             *slash = L'\0';
> 
>             if (!::CreateDirectoryW(mResolvedPath.get(), NULL)) {
>                 rv = ConvertWinError(GetLastError());
>+                if (rv == NS_ERROR_FILE_NOT_FOUND && 
>+                    directoryCreateError == NS_ERROR_FILE_ACCESS_DENIED) {
>+                    // If a previous CreateDirectory failed due to access, return that.
>+                    return NS_ERROR_FILE_ACCESS_DENIED;
>+                }

As a style nit, it would be better to put the constant on the left side of the ==, i.e.:

if (NS_ERROR_FILE_NOT_FOUND == rv &&
    NS_ERROR_FILE_ACCESS_DENIED == directoryCreateError)

That way any accidentally mistyped "=" become compile errors. But since the surrounding code doesn't use that style currently, I won't insist on it.
Comment 13 Brian R. Bondy [:bbondy] 2011-08-30 14:58:11 PDT
Created attachment 557006 [details] [diff] [review]
Patch for wrong error code when access denied on Create operations v2.

Fixed coding style on if conditions and NS_OK instead of 0.
Comment 14 Brian R. Bondy [:bbondy] 2011-09-21 10:40:00 PDT
Pushed to try: 
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=ed47da6511a5
Comment 15 Brian R. Bondy [:bbondy] 2011-09-22 06:23:43 PDT
Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/65f0640bf316
Comment 16 Ed Morley [:emorley] 2011-09-22 17:45:43 PDT
https://hg.mozilla.org/mozilla-central/rev/65f0640bf316

Note You need to log in before you can comment on or make changes to this bug.