163941 - nsLocalFileWin.cpp's nsLocalFile::ResolvePath code for .lnk has been broken since rev 1.70 by dougt [1/7/02]

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[timeless]

bug 116299 introduced unreachable code paths and probably would have totally
broken .lnk handling if not for the minor detail that it had already been broken
for ages.

anyway, we've known .lnk resolution has been broken, but normally no one really
notices it anyway, so it's not a big deal.  I found one of the reasons it's
broken (and this means it's time for me to stop waiting and actually rebuild my
w2k buildenv).

in 1.70, dougt changed code from:
if (slash == nsnull) {
  if (filePath[0] != nsnull && filePath[1] == ':' && filePath[2] == '\0') {
...
    return;
  } else {
...
    return;
  }
}
doImportantWork;
to:
if (!slash && filePath[0] != nsnull && filePath[1] == ':' && filePath[2] == 
'\0') {
...
  return;
} else {
...
  return;
}
unreachable:
doImportantWork;

--

Well, i suppose people want testcases so that they can find out that .lnk
resolution actually works.
The following should work:
A. dropping a shortcut (.lnk) from explorer into mozilla's content panel.
B. entering a file:/// url that ends in the .lnk or perhaps .lnk/ (i haven't
played with this much)

fwiw, 094,095,096 can't actually resolve a shortcut in case A or B, so while
this is definitely one of the many bugs in .lnk resolution, it isn't the only
one. Mozilla0.6/0.8/0.9/0.9.2/0.9.3 actually handle A. (0.9/0.9.2 handles both
variants of B -- i didn't spend time checking B earlier)

Ok. so .lnk worked up to 0.9.3 and was broken before 0.9.4, that's nice to know.
That means that I have all that space to cover while i search for the first
straw (this bug is the last straw).

Comment 1

[Doug Turner (:dougt)]

if no one but timeless has bitch about lnk resolution, maybe we should just rip
it out.  Most window application do nothing like this when handling file paths.

did you have to cc the whole world? 

Comment 2

[Doug Turner (:dougt)]

timeless, care to provide a patch which either rips out all of this shortcut
code or fixes the problem?

Comment 3

[Doug Turner (:dougt)]

neeti is going to fix this.

Comment 4

[neeti]

Attachment: patch for resolving shortcuts

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 103748 [details] [diff] [review]
patch for resolving shortcuts

the widget code should be ifdef windows only - passing out the target of a
symlink isn't a good idea.

You have some tabs in the xpcom/io/nsLocalFileWin.cpp diff. 

explain why exactly you are doing this:

 {
-    if (mDirty)
+    if ((strstr(mWorkingPath.get(), ".lnk") == nsnull) && mDirty)

Maybe we should be creating a IsDirty method that does this logic.  are there
other places that need to be checked in a similar way??

lets get a new patch together.

Comment 6

[neeti]

>the widget code should be ifdef windows only - passing out the target of a
>symlink isn't a good idea.
No need to ifdef windows because the changes are already in a windows specific
file - widget/src/windows/nsClipboard.cpp

>You have some tabs in the xpcom/io/nsLocalFileWin.cpp diff. 
removed the tabs

>{
>-    if (mDirty)
>+    if ((strstr(mWorkingPath.get(), ".lnk") == nsnull) && mDirty)
Added explanation here. This is the only place we check this logic because we
have an optimization. Other places call ResolveAndSatat()

Comment 7

[neeti]

Attachment: revised patch

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 105119 [details] [diff] [review]
revised patch

r=dougt

Comment 9

[Darin Fisher]

Comment on attachment 105119 [details] [diff] [review]
revised patch

some review comments...  a couple leaks... some i18n problems...
and a few nits... needs work.


>Index: netwerk/base/src/nsURLHelperWin.cpp

>+    localFile->GetFollowLinks(&followSymlinks);
>+    if (followSymlinks)
>+    {
>+        rv = localFile->GetNativeTarget(ePath);
>+        if (ePath.IsEmpty())
>+            rv = localFile->GetNativePath(ePath);

why should URLs use the resolved path?	why not let URLs reference a
shortcut?  this seems wrong to me.  if this is truly desired behavior
then why not make GetNativePath return the resolved path?


>Index: xpcom/io/nsLocalFileWin.cpp

>     char* slash = strchr(filePath, '\\');

isn't filePath using the native charset?  i think it is, since it is in 
effect a copy of mWorkingPath.	this means that strchr is wrong.  you need
to replace strchr w/ _mbschr.  see other examples in nsLocalFileWin.cpp.


>+    int slashIndex = slash - filePath;

no point in computing slashIndex here.	the result of this calculation
is never used.


>+        if (filePath[0] != nsnull && filePath[1] == ':' && filePath[2] == '\0')

filePath[0] is of type char, so you should not compare its value against
nsnull.
compare it against '\0' or just 0, but nsnull should only be used with
pointers.


>+            int filePathLen = strlen(filePath);

don't compute strlen twice... it has already been done once when filePath
was allocated.


>+            char* rp = (char*) nsMemory::Alloc( filePathLen + 2 );
>+            if (!rp)
>+                return NS_ERROR_OUT_OF_MEMORY;

if we return here, then we will have leaked filePath, right?


>+            memcpy( rp, filePath, filePathLen );
>+            rp[filePathLen] = '\\';
>+            rp[filePathLen+1] = 0;
>+            *resolvedPath = rp;
>+    
>+            nsMemory::Free(filePath);

would it be worth it to allocate filePath large enough to hold an extra
character just so as to optimize this case?

also, perhaps it would probably greatly simplify the memory allocation
stuff if this function used |nsACString&| instead of |char*|.  buf, of
course that'd just be extra goodness... most important thing is to get
this function working again.

>     slash = strchr(++slash, '\\');
>+    slashIndex = slash - filePath;

i don't think it is wise to compute slashIndex before checking if
slash is null.	slashIndex will have some garbage value potentially,
and even though you are checking slash before using slashIndex, it
seems risky to ever give slashIndex an invalid value.

>     while (slash || resolveTerminal)
>     {
>         if (slash)
>         {
>+            filePath[slashIndex] = '\0';

perhaps you don't even need to compute slashIndex.  why not just
replace this line w/ |*slash = '\0'|, which is what i think it
used to say.  then down below you don't have to recompute
slashIndex.

>@@ -513,74 +514,67 @@
>         if ((offset > 0) && (strncmp( (filePath + offset), ".lnk", 4) == 0))
>         {
>             MultiByteToWideChar(CP_ACP, 0, filePath, -1, wsz, MAX_PATH); 

this code would probably benefit from calling NS_CopyNativeToUnicode
since doing so would solve the "XXX make this dynamically allocated"
comment.  for example,

  nsAutoString ucsBuf;
  NS_CopyNativeToUnicode(nsDependentCString(filePath, filePathLen), ucsBuf);
  ...
  if (gResolver)
      gResolver->Resolve(uscBuf.get(), temp);

>+            else
>+            {
>+                // could not resolve shortcut.  Return error;
>+                nsMemory::Free(filePath);
>+                return NS_ERROR_FILE_INVALID_PATH;

and here, it looks like you'd leak |temp|.

Comment 10

[neeti]

I have made all the changes mentioned above, except for the changes mentioned in
netwerk/base/src/nsURLHelperWin.cpp

In the case of dragging a shortcut to a directory to the browser, if we just use
the workingPath for URLs as suggested above, we can still correctly display the
directory listing, however when one of the links in the listing is clicked on,
the resulting URL is wrong and we cannot access the links.

e.g. I have shortcut directory link 
file:///C:/Documents%20and%20Settings/neeti/Desktop/Shortcut%20to%20testdir.lnk/

In this directory I have a file test.html.
When I click on test1.html in the directory listing of the above shortcut I get
get an error message

The file /C:/Documents%20and%20Settings/neeti/Desktop/test1.html not found.

Comment 11

[neeti]

Attachment: revised patch

Comment 12

[Darin Fisher]

> In the case of dragging a shortcut to a directory to the browser, if we just use
> the workingPath for URLs as suggested above, we can still correctly display the
> directory listing, however when one of the links in the listing is clicked on,
> the resulting URL is wrong and we cannot access the links.

hmm.. what makes the resulting URL wrong?

if i have the following directory and files

  c:\dir
  c:\dir\a.html
  c:\shortcut.lnk  ->  c:\dir

then

  file:///c:/shortcut.lnk/a.html

should be equivalent to
 
  file:///c:/dir/a.html

perhaps someone is creating a local file w/ the followLinks attribute set to
false when it should be really be set to true?  should the default value be
true?  perhaps there is a performance reason why we would not want to do this?!?

Comment 13

[neeti]

>hmm.. what makes the resulting URL wrong?
>file:///c:/shortcut.lnk/a.html

> should be equivalent to
 
>file:///c:/dir/a.html

When we click on a.html, then the result is

file:///c:/a.html
instead of
file:///c:/shortcut.lnk/a.html

Comment 14

[Darin Fisher]

perhaps the problem is that the document's base URI is

  file:///c:/shortcut.lnk

instead of

  file:///c:/shortcut.lnk/

because resolving a.html against the first href would result in
file:///c:/a.html instead of file:///c:/shortcut.lnk/a.html.  so, this bug is
probably caused by nsLocalFileWin not appending a '/' at the end of the working
path for shortcuts.  of course it has to distinguish a directory shortcut from a
file shortcut in order to do this correctly.

Comment 15

[neeti]

Yes, the document's base URI is

file:///c:/shortcut.lnk

instead of

file:///c:/shortcut.lnk/

Comment 16

[Darin Fisher]

neeti: when you load file:///c:/dir, and you see a file directory listing, i bet
the base URI is file:///c:/dir/ instead of file:///c:/dir... otherwise, things
wouldn't work.  please check this case out.  i bet someone is doing some sort of
special check to see if file:///c:/dir is a directory, and if so, they are
probably appending a '/'.  my guess is that we need to do something similar if
the leaf is a shortcut to a directory.

Comment 17

[neeti]

yes, the  base URI is indeed file:///c:/dir/ instead of file:///c:/dir...
Need to figure out where this slash is being appended.

Comment 18

[neeti]

Attachment: revised patch

Attaching a new patch with all the changes to
netwerk/base/src/nsURLHelperWin.cpp removed.

Comment 19

[Doug Turner (:dougt)]

Comment on attachment 107050 [details] [diff] [review]
revised patch

add a comment as to why you need three extra bytes here:

     // Get the native path for |this|
-    char* filePath = (char*) nsMemory::Clone( workingPath,
strlen(workingPath)+1 );
+    int filePathLen = strlen(workingPath);
+    char* filePath = (char*) nsMemory::Clone( workingPath, filePathLen+3 );

I am kindof confused.  In some places you use strstr but in other you use
_mbsstr.  Can we make this consistent?

Comment 20

[neeti]

Attachment: revised patch

Comment 21

[Darin Fisher]

Comment on attachment 107072 [details] [diff] [review]
revised patch

>Index: netwerk/streamconv/converters/nsIndexedToHTML.cpp

>+        nsCOMPtr<nsILocalFile> lfile = do_QueryInterface(file);
>+        lfile->SetFollowLinks(PR_TRUE);

you should null check lfile in case the QI fails for some reason.


>Index: xpcom/io/nsLocalFileWin.cpp

>+    if (_mbsstr((unsigned char*)workingPath, (unsigned char*)".lnk") == nsnull)
>         return NS_ERROR_FILE_INVALID_PATH;

so, this concerns me since it would also match .lnkx, which might exist.
seems like we need to use a more sophisticate search algorithm.


>+    int filePathLen = strlen(workingPath);
>+    // allocate extra bytes incase we need to append '//' and '\0' to the 
>+    // workingPath, if it is just a drive letter and a colon  
>+    char* filePath = (char*) nsMemory::Clone( workingPath, filePathLen+3 );

don't you mean '\\' instead of '//' because '\\' is one byte whereas '//' is
two bytes.  should 3 be 2 instead?


>+        if (filePath[0] != '\0' && filePath[1] == ':' && filePath[2] == '\0')

how about using nsCRT::IsAsciiAlpha(filePath[0]) instead of (filePath[0] !=
'\0')?


>+            NS_CopyNativeToUnicode(nsDependentCString(filePath, strlen(filePath)), ucsBuf);
nit:	      NS_CopyNativeToUnicode(nsDependentCString(filePath), ucsBuf); //
equivalent

>+            }
>+		}
>+		if (slash)
>+		{
>+			*slash = '\\';

nit: wierd indentation here.

> nsLocalFile::OpenNSPRFileDesc(PRInt32 flags, PRInt32 mode, PRFileDesc **_retval)
> {
>+    if ((_mbsstr((unsigned char*)mWorkingPath.get(), (unsigned char*)".lnk") == nsnull) && mDirty)

same problem as above.	this will mistakenly match "c:\temp\foo.lnk.html" which
is admittedly wacky but totally valid on windows.


>-    const char *workingFilePath = mWorkingPath.get();
>-    DWORD word = GetFileAttributes(workingFilePath);
>+
>+    const char *resolvedFilePath = mResolvedPath.get();
>+    DWORD word = GetFileAttributes(resolvedFilePath);

should this be based on the value of mFollowLinks?

Comment 22

[neeti]

Attachment: revised patch

Attached a new patch with changes mentioned above.

Comment 23

[Darin Fisher]

Comment on attachment 107178 [details] [diff] [review]
revised patch

>Index: xpcom/io/nsLocalFileWin.cpp

>+    // check to see if it is shortcut, i.e., it has ".lnk" in it
>+    unsigned char* dest = _mbsstr((unsigned char*)workingPath,
>+                                  (unsigned char*)".lnk");
>+    if (!dest)
>         return NS_ERROR_FILE_INVALID_PATH;
> 
>+    int filePathLen = strlen(workingPath);
>+    // find index of ".lnk"
>+    int result = (int)(dest - (unsigned char*)workingPath);
>+
>+    // if ".lnk" is not at the leaf of this path, we need to make sure the
>+    // next char after ".lnk" is a '\\'. e.g. "c:\\foo.lnk\\a.html" is valid,
>+    // whereas "c:\\foo.lnkx" is not.
>+    if ((result+3) != (filePathLen-1))
>+    {
>+         if (workingPath[result+4] != '\\')
>+             return NS_ERROR_FILE_INVALID_PATH;
>+    }

this should be moved to a static helper function since it is
essentially the same code needed below.  also, i'd recommend
writing it like this instead:

    if (result + 4 < filePathLen) {
	if (workingPath[result + 4] != '\\')
	    return NS_ERROR_FILE_INVALID_PATH;
    }

otherwise, looks good.

Comment 24

[neeti]

Attachment: revised patch

Created new patch with changes mentioned above.

Comment 25

[Darin Fisher]

Comment on attachment 107378 [details] [diff] [review]
revised patch

>Index: xpcom/io/nsLocalFileWin.cpp

>+    {
>+	    if (workingPath[result + 4] != '\\')
>+	        return PR_FALSE;
>+    }

nit: eliminate tabs

>+    // check to see if it is shortcut, i.e., it has ".lnk" in it
>+    PRBool isShortcut = IsShortcut(workingPath);
>+    if (!isShortcut)
>         return NS_ERROR_FILE_INVALID_PATH;
> 
> #ifdef DEBUG_dougt
>     printf("localfile - resolving symlink\n");
> #endif
> 
>+    int filePathLen = strlen(workingPath);

sucks to compute length of workingPath twice.  why don't you compute the
length of workingPath first, and then pass it as a parameter to IsShortcut?

>+    PRBool isShortcut = IsShortcut(mWorkingPath.get());

      PRBool isShortcut = IsShortcut(mWorkingPath.get(),
mWorkingPath.Length());

fix that, and sr=darin

Comment 26

[neeti]

Attachment: revised patch

removed tabs and passing in strlen of workingPath to IsShortcut()

Comment 27

[Darin Fisher]

Comment on attachment 107383 [details] [diff] [review]
revised patch

>Index: xpcom/io/nsLocalFileWin.cpp

>+    int filePathLen = strlen(workingPath);
...
>+    char* filePath = (char*) nsMemory::Clone( workingPath, filePathLen+2 );

i was about to mark this patch sr=darin, but then this caught my attention.
this is wrong!	we'll read one byte beyond the length of workingPath, and
that might cause a crash.

allocating an extra byte is good as an optimization, but it requires that
you manually memcpy the string into the new buffer.

      char *filePath = (char *) nsMemory::Alloc(filePathLen+2);
      if (!filePath)
	 // ... blah, blah
      memcpy(filePath, workingPath, filePathLen + 1);

sorry for not catching this problem in the earlier reviews!

Comment 28

[neeti]

Attachment: revised patch

made the changes mentioned above.

Comment 29

[Darin Fisher]

Comment on attachment 107394 [details] [diff] [review]
revised patch

sr=darin

Comment 30

[rpotts (gone)]

Comment on attachment 107394 [details] [diff] [review]
revised patch

r=[e-mail]

Looks good to me.  Do we care about the situation where a link does *not* end
in '.lnk'?
see:
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platfo
rm/shell/programmersguide/shell_int/shell_int_programming/shortcuts/shortcut.as
p

apparently it is possible to register any file extension as a shortcut... 
However, i don't know if we care about this :-)

-- rick

Comment 31

[Darin Fisher]

wonderful!  let's not worry about _that_ for the moment.  this patch will cover
the 99.99% case.  though, perhaps we should file a bug about fixing the
non-standard shortcut extension case.  hopefully, windows provides an API to
discover all extensions that mean shortcut :)

neeti: can you file a followup bug on this issue?  thx!

Comment 32

[neeti]

Created bug 182085 for resolving non-standard shortcut extension cases.

Comment 33

[neeti]

fix checked in.