243473 - nsLocalFileWin shortcut resolver does more work than necessary

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Brodie]

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>

Comment 1

[timeless]

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

[Darin Fisher]

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.

Comment 3

[Brodie]

Attachment: first cut

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

[Darin Fisher]

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.

Comment 5

[Brodie]

Attachment: complete rewritten test nsIFile testcases

New automated test harness for nsIFile/nsILocalFile as described at bug 134796
(updated to include a few more tests)

Comment 6

[Darin Fisher]

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!

Comment 7

[Brodie]

Attachment: test cases

Comment 8

[Brodie]

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). 

Comment 9

[Brodie]

Attachment: patch 1

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)

Comment 10

[Brodie]

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).

Comment 11

[Darin Fisher]

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

[Darin Fisher]

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.

Comment 13

[Brodie]

Attachment: patch 2

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.

Comment 14

[Brodie]

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?

Comment 15

[Darin Fisher]

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.

Comment 16

[Brodie]

Attachment: patch 3

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.

Comment 17

[Brodie]

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

[Darin Fisher]

hmm... same compilation problem with INVALID_FILE_ATTRIBUTES.  what is the value
of that define in the VC7 headers?

Comment 19

[Darin Fisher]

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

[Darin Fisher]

fixed-on-trunk

Comment 21

[Isaac Hwak Han]

This may have caused bug 247637.

Comment 22

[Simon Montagu :smontagu]

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.

Comment 23

[Simon Montagu :smontagu]

Changing those comments to end in '\' instead of \ fixes bug 246988 for me, so
this is causing serious dataloss.

Comment 24

[Brodie]

Attachment: regression bug fix

This patch fixes the regression bugs in bug 246988 and bug 247637.

Comment 25

[Brodie]

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

[Darin Fisher]

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.

Comment 27

[Darin Fisher]

regression fix checked in on trunk.

marking FIXED again.

Comment 28

[Darin Fisher]

Brodie: please make sure that the "slutty hack" gets cleaned up
sometime, thanks ;-)

Comment 29

[[e-mail]]

>// 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 :-)