Closed
Bug 243473
Opened 20 years ago
Closed 20 years ago
nsLocalFileWin shortcut resolver does more work than necessary
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: bmo, Assigned: bmo)
References
Details
Attachments
(3 files, 4 obsolete files)
27.99 KB,
text/plain
|
Details | |
51.56 KB,
patch
|
darin.moz
:
review+
dougt
:
superreview+
|
Details | Diff | Splinter Review |
3.28 KB,
patch
|
darin.moz
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040421 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040421 In xpcom/io/nsLocalFileWin.cpp there is some code to do shortcut resolution for paths like: c:\foo\shortcut.lnk\bar\file.txt This appears to be designed to resolve to 'c:\hello\bar\file.txt' for a tree like: c:\ foo\ shortcut.lnk -> c:\hello\ hello\ bar\ file.txt However calling the Win32 function CreateFile() on the same path will fail. Since there is no reason to support functionality which the OS doesn't support itself, this functionality should be removed from nsLocalFileWin.cpp which would save a few bytes. Reproducible: Always Steps to Reproduce: 1. 2. 3. See nsLocalFile::ResolvePath, <http://lxr.mozilla.org/seamonkey/source/xpcom/io/nsLocalFileWin.cpp#473>
we've talked about this before. someone wanted to be able to do this, so things could work like they do on unix. but i can certainly see removing it as i doubt anyone uses it or knows that they can.
Comment 2•20 years ago
|
||
Yeah, I think it will probably be sufficient to only resolve the terminal node when followLinks is set to true. I may have argued against that in the past for XP consistency, but the more I think about it the more I feel that that's not so important.
This is the first cut at removing the shortcut stuff and a general cleanup of the nsLocalFileWin implementation. The general cleanup occurred because this bug touches so many things that it seemed easier to do it all at once. Things this addresses (or tries to address) are: * removes resolving of .lnk files in paths * fixes bug 134796 (correct handling of c:\ etc) * correctly handles the new drive enumerator in all functions * checks paths for validity more strictly (at creation time) * reduces code size by combining similar functions * fixes unreported bug with SetFileSize using > 32 bit size * implements permissions better * removes unused member variable 'mFSCharsetIsUTF8' * removes (newly) unnecessary member variable 'mLastResolution' * implements *OfLink correctly (as far as I understand from the spec) so that we return the shortcut file properties As this patch includes a fix for bug 134796 I would prefer to just make that bug depend on this and fix it together.
Comment 4•20 years ago
|
||
we should land this early in 1.8 to get as much feedback on the trunk as possible. thanks for taking the time to do this work! i'll try to review this next week.
Updated•20 years ago
|
Attachment #148572 -
Flags: review?(darin)
New automated test harness for nsIFile/nsILocalFile as described at bug 134796 (updated to include a few more tests)
Comment 6•20 years ago
|
||
Brodie: this patch does not apply cleanly to the trunk. perhaps someone else has checked in something since you wrote this patch. would you mind merging your patch to the trunk and posting an up-to-date version for review? thanks!
Attachment #148798 -
Attachment is obsolete: true
I'm busy with work at the moment. I might get back to this in a week or two. I know that everyone is busy, but I would really appreciate if someone would at least try to compile and run the "test cases" file on Linux (my build server is dead at the moment). It is a complete replacement for nsIFileTest.cpp and so should be easy (in theory): copy to mozilla/xpcom/tests make (in mozilla/xpcom/tests), [if no build errors then] ./nsIFileTest > test.log (in mozilla/dist/bin) check to see how many test failures there were (near line of file) It has a lot of test case failures with the current implementation of nsLocalFileWin because the current nsLocalFileWin doesn't implement nsILocalFile according to the spec (e.g. GetParent doesn't return null/NS_OK when there is no parent).
This is a respin of the first patch for the current head. I cut the scope of the patch down a bit, but it still does more than just remove the shortcut resolver. List of things touched are: * removes resolving of .lnk files in paths * fixes unreported bug with SetFileSize using > 32 bit size * implements get/set permissions better (bug 140224) * removes unused member variable 'mFSCharsetIsUTF8' * removes (newly) unnecessary member variable 'mLastResolution' * implements *OfLink correctly (as far as I understand from the spec) so that we return the shortcut file properties * fixes a few spelling errors * ensure that append doesn't permit the ".." directory (bug 217580) It doesn't touch any of the path processing stuff that the previous patch did (i.e. nothing to do with bug 134796)
Attachment #148572 -
Attachment is obsolete: true
Attachment #148572 -
Flags: review?(darin)
Attachment #149888 -
Flags: review?(darin)
Assignee | ||
Comment 10•20 years ago
|
||
Comment on attachment 149888 [details] [diff] [review] patch 1 Doug, this patch rips out the .lnk resolution inside of paths (e.g. c:/foo.lnk/blah.txt) and fixes a number of bugs in nsLocalFileWin (see previous comment).
Attachment #149888 -
Flags: superreview?(dougt)
Comment 11•20 years ago
|
||
I'm getting a compilation error using VC6: c:/builds/moz-trunk/mozilla/xpcom/io/nsLocalFileWin.cpp(271) : error C2065: 'INV ALID_SET_FILE_POINTER' : undeclared identifier make[1]: *** [nsLocalFileWin.obj] Error 2 make[1]: Leaving directory `/cygdrive/c/builds/moz-trunk/sea-debug-build/xpcom/i o' I checked MSDN, and it seems to suggest that this should work. Then, I grepped the header files included with VC6 and came up with no hits. I guess Microsoft only added this identifier more recently. From reading the documentation it looks like it should be sufficient to simply test GetLastError() in all cases. Or, perhaps we should just hard code the value of INVALID_SET_FILE_POINTER?
Comment 12•20 years ago
|
||
Comment on attachment 149888 [details] [diff] [review] patch 1 >Index: xpcom/io/nsLocalFileWin.h >+ nsresult SetModDate(PRInt64 aLastModifiedTime, const char * filePath); >+ nsresult HasFileAttribute(DWORD fileAttrib, PRBool *_retval); nit: try to be consistent with whitespace in parameter lists. >Index: xpcom/io/nsLocalFileWin.cpp >+MyFileSeek64(HANDLE aHandle, __int64 aDistance, DWORD aMoveMethod) >+{ >+ LARGE_INTEGER li; style nit: indentation is 4 spaces in this file.. please try to keep all the code consistent. thanks! >+IsShortcutPath(const char * path) whitespace nit in parameter list. >+ const char * ext = (const char *) _mbsrchr((const unsigned char *)path, '.'); >+ if (!ext || 0 != stricmp(ext, ".lnk")) > return PR_FALSE; nit: you already know if ext starts with a '.', so how about this instead: if (!ext || 0 != stricmp(ext + 1, "lnk")) >+// Resolve the shortcut file from mWorkingPath and write the path >+// it points to into mResolvedPath. > nsresult >+nsLocalFile::ResolveShortcut() > { >+ // we can't do anything without the resolver >+ if (!gResolver) >+ return NS_ERROR_FAILURE; > >+ // allocate the memory for the result of the resolution >+ nsAutoString ucsBuf; >+ NS_CopyNativeToUnicode(mWorkingPath, ucsBuf); >+ char *resolvedPath = (char*) nsMemory::Alloc( MAX_PATH ); >+ if (!resolvedPath) > return NS_ERROR_NULL_POINTER; > >+ // resolve this shortcut >+ nsresult rv = gResolver->Resolve(ucsBuf.get(), resolvedPath); >+ if (NS_FAILED(rv)) > { >+ nsMemory::Free(resolvedPath); >+ return rv; > } > >+ mResolvedPath.Adopt(resolvedPath); >+ return NS_OK; > } Adopting the buffer into mResolvedPath means that the string buffer will not be sharable. This means that if the code ever assigns mResolvedPath into another nsACString that it will have to copy. You can avoid that by writing this function like this instead: nsresult nsLocalFile::ResolveShortcut() { // we can't do anything without the resolver if (!gResolver) return NS_ERROR_FAILURE; // allocate the memory for the result of the resolution nsAutoString ucsBuf; NS_CopyNativeToUnicode(mWorkingPath, ucsBuf); mResolvedPath.SetLength(MAX_PATH); char *resolvedPath = mResolvedPath.BeginWriting(); // resolve this shortcut nsresult rv = gResolver->Resolve(ucsBuf.get(), resolvedPath); size_t len = NS_FAILED(rv) ? 0 : strlen(resolvedPath); mResolvedPath.SetLength(len); return rv; } >+nsLocalFile::AppendNativeInternal(const nsAFlatCString &node, PRBool multipleComponents) >+ unsigned char * doubleDot = _mbsstr(nodePath, (unsigned char *)"\\.."); >+ while (doubleDot) >+ { >+ doubleDot += 2; >+ if (*doubleDot == '\0' || *doubleDot == '\\') >+ return NS_ERROR_FILE_UNRECOGNIZED_PATH; >+ doubleDot = _mbsstr(doubleDot, (unsigned char *)"\\.."); >+ } maybe i haven't had enough coffee yet, but i don't get how the inner condition of the loop is supposed to do anything. if we enter the loop body, then it means that doubleDot starts with \.., so it seems like "doubleDot += 2" should be "doubleDot += 3", no? surely, after incrementing by two we know that doubleDot is pointing at '.' and never anything else. so why test if it equals '\0' or '\\'? >+ // Since shortcut files are no longer permitted to be used as unix-like >+ // symlinks interspersed in the path (e.g. "c:/file.lnk/foo/bar.txt") >+ // this processing is a lot simpler. Even if the shortcut file is >+ // pointing to a directory, only the mWorkingPath value is used and so >+ // only the shortcut file file will be deleted. "shortcut file file" -- do you mean "shortcut file" ? >+nsLocalFile::GetPermissionsOfLink(PRUint32 *aPermissions) > { >+ NS_ENSURE_ARG(aPermissions); >+ >+ nsresult rv = ResolveAndStat(); >+ if (NS_FAILED(rv)) >+ return rv; >+ >+ DWORD word = GetFileAttributes(mWorkingPath.get()); >+ PRBool isWritable = !(word & FILE_ATTRIBUTE_READONLY); >+ >+ *aPermissions = PR_IRUSR|PR_IRGRP|PR_IROTH; // all read >+ if (isWritable) >+ *aPermissions |= PR_IWUSR|PR_IWGRP|PR_IWOTH; // all write >+ >+ return NS_OK; > } why does this function call ResolveAndStat if its result is only a function of mWorkingPath? > nsLocalFile::SetPermissionsOfLink(PRUint32 aPermissions) ... >+ nsresult rv = ResolveAndStat(); same question here.
Attachment #149888 -
Flags: review?(darin) → review-
Assignee | ||
Comment 13•20 years ago
|
||
This patch fixes the nits and takes Darin's suggestions (re: IsShortcutPath and ResolveShortcut). If INVALID_SET_FILE_POINTER isn't defined then I now define it using the same definition as appears in VC.NET headers. AppendNativeInternal has been fixed to do what it should've been doing in the first place (I've written a heap of testcases for it and tested it in the new nsIFileTest.cpp regression test harness). *OfLink has been fixed so that we don't call ResolveAndStat but just go straight to the file system, I have commented that we expect that the user already knows that this is a symlink otherwise the results may be unexpected. The spec for the *OfLink functions is a bit vague - I wasn't sure what it was supposed to do myself. Fixed the function SetModDate to test for INVALID_HANDLE_VALUE as the return value of CreateFile instead of NULL. SetFileSize only makes the file dirty if we succeed. GetDiskSpaceAvailable has been rewritten with the same logic but a simpler body. I've documented why we are looking up the GetDiskFreeSpaceExA function (copy and paste from MSDN) and used the MSDN recommended code. I've also got rid of the unused variables and unnecessary use of a double.
Attachment #149888 -
Attachment is obsolete: true
Attachment #149888 -
Flags: superreview?(dougt)
Assignee | ||
Comment 14•20 years ago
|
||
Comment on attachment 150320 [details] [diff] [review] patch 2 Darin: I don't have bugzilla perms at the moment. Would you please assign this bug to me?
Attachment #150320 -
Flags: review?(darin)
Comment 15•20 years ago
|
||
Comment on attachment 150320 [details] [diff] [review] patch 2 >+nsLocalFile::AppendNativeInternal(const nsAFlatCString &node, PRBool multipleComponents) ... >- mWorkingPath.Append(NS_LITERAL_CSTRING("\\") + node); >+ >+ mWorkingPath.Append('\\'); >+ mWorkingPath.Append(node); any good reason for this change? the single Append call is more efficient. operator+ actually creates a stack object that simply holds pointers to both nsCSubstring objects. when Append is called on a concatenation, it efficiently allocates enough space for the result of the concatenation and then copies each element of the concatenation into the string buffer. it never actually forms the string corresponding to the concatenation. so, i think you should revert this change. r=me with that change.
Attachment #150320 -
Flags: review?(darin) → review+
Assignee | ||
Comment 16•20 years ago
|
||
No good reason for the change. I didn't realize that the single append would be faster (and after testing, what do you know, it is! :-) This patch has that line reverted. Nothing else changed.
Attachment #150320 -
Attachment is obsolete: true
Attachment #150396 -
Flags: superreview?(dougt)
Attachment #150396 -
Flags: review?(darin)
Updated•20 years ago
|
Attachment #150396 -
Flags: review?(darin) → review+
Updated•20 years ago
|
Attachment #150396 -
Flags: superreview?(dougt) → superreview+
Assignee | ||
Comment 17•20 years ago
|
||
Darin: I currently don't have a CVS account (in process of requesting one, bug 247241) so I can't check this in myself yet. Doug was going to but is going to be away for a while now. Would you be so kind as to oblige?
Comment 18•20 years ago
|
||
hmm... same compilation problem with INVALID_FILE_ATTRIBUTES. what is the value of that define in the VC7 headers?
Comment 19•20 years ago
|
||
on irc.mozilla.org#developers: <Silver> #define INVALID_FILE_ATTRIBUTES ((DWORD)-1) so, i patched the source accordingly, and the patch compiles fine. i'll be checking in the patch shortly.
Comment 20•20 years ago
|
||
fixed-on-trunk
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Comment 21•20 years ago
|
||
This may have caused bug 247637.
Comment 22•20 years ago
|
||
This patch seems to have serious problems. It causes "multiline comment" warnings for the following lines: if (*nodePath == '\\' // can't start with an \ else if (_mbschr(nodePath, '\\')) // single components can't contain \ I don't see that it can be working as designed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 23•20 years ago
|
||
Changing those comments to end in '\' instead of \ fixes bug 246988 for me, so this is causing serious dataloss.
Severity: trivial → blocker
Assignee | ||
Comment 24•20 years ago
|
||
This patch fixes the regression bugs in bug 246988 and bug 247637.
Attachment #151496 -
Flags: superreview?(dougt)
Attachment #151496 -
Flags: review?(darin)
Assignee | ||
Comment 25•20 years ago
|
||
A note on the patch: * The fix for bug 246988 is just Simon's suggestion of quoting the backslashes to stop the compiler from thinking they are continuations * The slutty hack for bug 247637 is some of the code from the original ResolveAndStat function
Comment 26•20 years ago
|
||
Comment on attachment 151496 [details] [diff] [review] regression bug fix r+sr=darin let's get this in quick to fix the dataloss bug on the trunk.
Attachment #151496 -
Flags: superreview?(dougt)
Attachment #151496 -
Flags: superreview+
Attachment #151496 -
Flags: review?(darin)
Attachment #151496 -
Flags: review+
Comment 27•20 years ago
|
||
regression fix checked in on trunk. marking FIXED again.
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Comment 28•20 years ago
|
||
Brodie: please make sure that the "slutty hack" gets cleaned up sometime, thanks ;-)
Comment 29•20 years ago
|
||
>// It is not necessary to call LoadLibrary on Kernel32.dll
>because it is already loaded into every process address space.
Particularly as LoadLibrary is linked from Kernel32.dll :-)
You need to log in
before you can comment on or make changes to this bug.
Description
•