156422 - Win32's nsILocalFile.reveal() (used by Reveal Location) doesn't select file

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[[e-mail]]

Possibly a Windows 95 thing because I noticed that the code looked very
complicated especially in comparison with the Perl that I use:
sub reveal {
  system "$ENV{'windir'}\\explorer.exe /n,/select,$_[0]"
}

Comment 1

[:deanis74]

Ummm... wow, that's some complicated code!  What Neil suggests works great for
me on Win2K, and is a lot faster than what we currently do, because of the up to
2 second wait.  Running explorer directly with "/select" works for folders as
well.  Both of these work fine for me:

  explorer /n,/select,c:\winnt\explorer.exe
  explorer /n,/select,c:\winnt

Comment 2

[:deanis74]

This isn't just Win95, it's not working for me on Win2K either.

What I meant in my last comment is that Neil's suggestion works fine for me on
Win2K.  I think we should simplify the code a lot and go with that.

Oh wait, but what if the user has a custom shell installed?  Does our current
code even work with that?

Comment 3

[[e-mail]]

Dean, in the case of a custom shell, then running explorer will (if memory
serves me correctly) still open an explorer window with the file selected, which
is probably what we want anyway. Are you in a position to write a patch for this?

Comment 4

[:deanis74]

Taking.

Comment 5

[:deanis74]

Attachment: cvs diff -u xpcom/io

Patch incorporating Neil's suggestion.	If the file is not found, an error is
raised that "The path '<path and filename>' does not exist or is not a
directory."  If you ask me, this code is much simpler and less prone to errors.
 Oh, and I think it works.

Comment 6

[:deanis74]

Reviews?

Comment 7

[[e-mail]]

Comment on attachment 92317 [details] [diff] [review]
cvs diff -u xpcom/io

>-  LONG r = (LONG) ::ShellExecute( NULL, "open", path.get(), NULL, NULL, SW_SHOWNORMAL);
>-  if (r < 32) 
>+    LONG r;
>+    r = (LONG) ::ShellExecute(NULL, "open", explorerPath.get(), explorerParams.get(),
>+                              NULL, SW_SHOWNORMAL);
>+    if (r < 32) 
>+        rv = NS_ERROR_FAILURE;

Dean, you may have copied bad code, but my docs say that ShellExecute returns
<= 32 on error. Also, why didn't you inline the LONG r?

I noticed that you seem to use 4-space indentation; is that consistent with the
rest of the file? The old code appeared to use 2-space indentation.

Comment 8

[:deanis74]

1. Yes, you're right.  I copied the existing code but it should check for <= 32.

2. You mean 'if ::ShellExecute() <= 32 return NS_ERROR_FAILURE'?  Good question.
 Again, I copied from the existing code.

3. /* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */

  Most of the file follows the tab width of 4, but Reveal and Launch don't.

Comment 9

[[e-mail]]

Dean, that's fine by me.

Comment 10

[:deanis74]

Attachment: take 2

1. No longer use a variable to handle the return value from ::ShellExecute().
2. No longer use the rv variable, as there are only three return points, and
one already uses the variable ret.
3. Changed < 32 to <= 32.
4. Pass ::GetForegroundWindow() to ::ShellExecute which, according to how I
read the docs, should make the foreground window the owner of any messageboxes
(eg. file not found).  In practice, this doesn't make a lick of difference on
Win2K.	The messagebox still comes up without a parent.

Comment 11

[[e-mail]]

> 4. Pass ::GetForegroundWindow() to ::ShellExecute which, according to how I
> read the docs, should make the foreground window the owner of any messageboxes
> (eg. file not found).  In practice, this doesn't make a lick of difference on
> Win2K.	The messagebox still comes up without a parent.

Other uses of ShellExecute in Mozilla all appear to pass NULL so which is
better, consistency or correctness? :-)

Comment 12

[:deanis74]

Makes no difference to me.

Comment 13

[:deanis74]

Aw man, I could've sworn I cc:ed Bill when I took this bug from him.  Bill,
there's a working, simple patch for this.  Any thoughts?

Comment 14

[:deanis74]

Comment on attachment 92530 [details] [diff] [review]
take 2

Need to double-quote the filename, to account for filenames with spaces,
commas, etc.

Comment 15

[:deanis74]

Attachment: double-quote the filename

1. Always wrap the filename in double quotes, to handle long filenames.
2. Remove the const declarations.  These are used only once, no need for the
extra code.
3. Remove passing ::GetForegroundWindow() as hWnd to ::ShellExecute(), to keep
in line with other uses of ::ShellExecute() in the app.

Ready for r=/sr=.

Comment 16

[[e-mail]]

> Need to double-quote the filename, to account for filenames with spaces,
> commas, etc.
FYI it appears to work with or without quotes on Windows 95, e.g.
C:\WINDOWS\explorer.exe /n,/select,C:\Program Files\Paint Shop Pro\psp.exe

Comment 17

[:deanis74]

Yes, it works for names with spaces but doesn't work for names with commas in them.

c:\winnt\explorer.exe /n,/select,c:\temp\test,1.txt

fails, while

c:\winnt\explorer.exe /n,/select,"c:\temp\test,1.txt"

works.  So there are at least some filenames that need to be quoted, so I just
quote them all.

Comment 18

[[e-mail]]

Silly me for not reading all of the text that I quoted. /Slaps wrist/

Comment 19

[Bill Law]

Comment on attachment 94582 [details] [diff] [review]
double-quote the filename

I think the lines:
+    explorerParams.Append("""");

are wrong.  That translates to two empty strings concatenated, I believe.  I
think they should be:
+    explorerParams.Append("\"");

With that fixed, r=law.

Comment 20

[:deanis74]

Wow, that's just dumb of me!  Yes, I'll definitely change that for the final
patch.  Who can sr= this?

Comment 21

[Alec Flett]

Comment on attachment 94582 [details] [diff] [review]
double-quote the filename

well, I guess this is ok but I'm not a huge fan of executing explorer directly.
 There must be a way to do this in-process, no?

I guess I'm concerned about some sort of security exploit involving executing
explorer directly. Maybe we should get mitch in on this.

also, the """" looks odd to me. Are you sure you didn't mean "\"\""? because to
me, """" looks like two empty strings concatenated together, similar to "a""b"

Comment 22

[Alec Flett]

cc'ing mstoltz for security stuff (mitch - see my previous comment) - do we have
a "secure" way of executing external processes - i.e. filtering valid filenames,
etc?

Comment 23

[:deanis74]

> also, the """" looks odd to me. Are you sure you didn't mean "\"\""? because to
> me, """" looks like two empty strings concatenated together, similar to "a""b"

Alec, yes.  See comment 19 and 20.  It should be "\"".  I've spent too much time
in other languages lately, and I forgot the C way of doing it.

Comment 24

[:deanis74]

Incidentally, though, """" adds a double quote for me, not a blank string.  It
has the same effect as "\"".

Comment 25

[Mitchell Stoltz (not reading bugmail)]

I am concerned about the security of this change. Let's hold off on checking
this in until we make sure it can't be used for anything sneaky.

Comment 26

[:deanis74]

I don't know how it can do anything "sneaky", since it uses "/select".  The
existing code does a ShellExecute(), and if you ask me the things it does are a
lot more risky than what this code proposes.

Comment 27

[:deanis74]

Mitchell, Alec, and Bill, have you thought about the security implications of
this?  I maintain that it's a safe way of doing things, and I believe that
Windows probably does this internally.

Comment 28

[[e-mail]]

Actually Windows doesn't even bother to verify the path to explorer.exe :-)

Comment 29

[:deanis74]

Can we decide if this is a security concern or not?  I'm fine with the new code,
and it sounds like Neil too.

Comment 30

[Mitchell Stoltz (not reading bugmail)]

I'm not sure if calling Explorer is a security risk or not. We probably need to
make sure we filter the parameters we pass, to make sure they can't do anything
nasty, but we don't have a standardized way of doing that. CCing Darin; he may
know more about calling Explorer.

Comment 31

[:deanis74]

Looking at the registry, the "Explore" context menu for folders uses this command:

  explorer /e,/root,{f39a0dc0-9cc8-11d0-a599-00c04fd64433},%L

Comment 32

[[e-mail]]

Strange, I have (both 95 and 2000)

C:\WINDOWS\Explorer.exe /e,/idlist,%I,%L

Comment 33

[:deanis74]

I've got the same thing I mentioned on my work and home machines, both running
Windows 2000.  That wasn't my point, though.  I was hoping to address the
question of whether it's OK to do this, and I haven't heard any security
problems from using the Explore command, so I don't think what we're doing is
anything out of the ordinary.

Comment 34

[Dan M]

I've been asked to comment. Not that I have any uncommon knowledge about this.
But it seems to me you just want to be extra careful to use the full path to
Explorer.exe and to the target file, and to not send any surprising parameters
to Explorer. And none of that buffer overflow stuff. The patch looks fine to me.

If it makes anyone feel any better, there's a page on MSDN about "security
issues of calling ShellExecute." It says what I just said. Or maybe it's the
other way around.
http://msdn.microsoft.com/library/default.asp?url=/library/en-us/shellcc/platform/shell/programmersguide/sec_shell.asp

Comment 35

[[e-mail]]

Comment on attachment 94582 [details] [diff] [review]
double-quote the filename

We need this to fix bug 211894.

Comment 36

[Alec Flett]

Comment on attachment 94582 [details] [diff] [review]
double-quote the filename

woah woah. I was going to say that I'm considering this.. but then I saw
+    explorerParams.Append("""");


This is the same as
+    explorerParams.Append("" "");

which is the same as
+    explorerParams.Append("");

which appends nothing. I think what you meant was
+    explorerParams.Append("\"");

but even better might be
+    explorerParams.Append('\"');

I still want to know if there is a safer way of calling explorer, and I'm
trying to figure out why the old way wasn't good. I realize it was complex, but
it relied on system APIs rather than passing specific (undocumented?)
parameters to explorer.

Comment 37

[:deanis74]

> woah woah. I was going to say that I'm considering this.. but then I saw
> +    explorerParams.Append("""");
> 
> This is the same as
> +    explorerParams.Append("" "");
> 
> which is the same as
> +    explorerParams.Append("");
> 
>which appends nothing.

Really?  In my testing it appended a double quote (") (see comment 24). 
Regardless, I'm fine changing that to use whatever syntax, that's minor.

One of the reasons the old method isn't good is because it waits a maximum of
only two seconds for the Explorer window to open before selecting the file.  We
could pump that up to 10 seconds, even 100 seconds, but because it's on a set
timer there's always the chance that Explorer will take longer to open than
we're waiting.

Comment 38

[Brodie]

Attachment: patch 4

Since Dean seems to be busy, I've taken his patch as the base and updated it.
The parameter /n is now not passed to explorer. This way explorer will reuse
any existing window which is already showing that folder. If there is no
existing window or existing windows are showing other folders, it will create a
new one. This is more user friendly behaviour I think.

One question, should Reveal() always show the parent folder of an item,
including the case of a directory? i.e. should Reveal() on C:\windows, (a) open
an explorer window showing the contents of c:\windows, or (b) open an explorer
window showing c:\ with windows selected. This patch does (a) which seems to be
more natural.

I've verified that this patch also fixes bug 211894.

Comment 39

[Brodie]

Attachment: patch 5

Comment 40

[[e-mail]]

Comment on attachment 149389 [details] [diff] [review]
patch 5

>+    nsresult rv;
>+   
>+    rv = ResolveAndStat(PR_TRUE);
Any reason why you separated out the declaration and removed the comment?
>+    if (NS_FAILED(rv) && rv != NS_ERROR_FILE_NOT_FOUND)
>+        return rv;

>+    // If there is no existing explorer window *showing that folder* then this will 
>+    // open a new window, otherwise it will reuse the existing window.
>+    // If the resolved path is a directory then we want to open it rather than 
>+    // opening the parent and selecting it, so we don't append the /select option.
>+    nsCAutoString explorerParams;
>+    if (mFileInfo64.type != PR_FILE_DIRECTORY) // valid because we ResolveAndStat above
>+        explorerParams.Append("/select,");
Sigh... this works fine in Windows 95 but on my W2K server it doesn't select
the file if the folder is already open :-(

>+    if (::ShellExecute(::GetForegroundWindow(), "open", explorerPath.get(),
I'm still not keen on GetForegroundWindow() because it may not be ours.
>+        explorerParams.get(), NULL, SW_SHOWNORMAL) <= (HINSTANCE) 32) 
This indentation is confusing... an extra 4 spaces perhaps?
>+        return NS_ERROR_FAILURE;

Comment 41

[Brodie]

>+    nsresult rv;
>+   
>+    rv = ResolveAndStat(PR_TRUE);
>Any reason why you separated out the declaration and removed the >comment?

Just personal style. I tend to separate variable declarations like nsresult when
I use the declared variable many times through the body of the function. I find
it makes it easier to see the declaration rather than having it buried, and if
the code gets moved around I don't get compile errors because I'm now using the
variable before it's declared. Since it's a simple stack variable it's no more
efficient one way or the other.

The comment I just removed because it matches the style of all other
ResolveAndStat calls. Easily replaced if you'd prefer.

>+    // opening the parent and selecting it, so we don't append the /select option.
>+    nsCAutoString explorerParams;
>+    if (mFileInfo64.type != PR_FILE_DIRECTORY) // valid because we
ResolveAndStat above
>+        explorerParams.Append("/select,");
> Sigh... this works fine in Windows 95 but on my W2K server it 
> doesn't select the file if the folder is already open :-(

I find the existing code doesn't always work either. We could always change it
so that we use the "/n" option. If we are opening a new window then it should
always select the file.

>+    if (::ShellExecute(::GetForegroundWindow(), "open", explorerPath.get(),
I'm still not keen on GetForegroundWindow() because it may not be ours.
>+        explorerParams.get(), NULL, SW_SHOWNORMAL) <= (HINSTANCE) 32) 
> This indentation is confusing... an extra 4 spaces perhaps?

Will do.

Comment 42

[[e-mail]]

(In reply to comment #41)
> The comment I just removed because it matches the style of all other
> ResolveAndStat calls. Easily replaced if you'd prefer.
Fine, although strangely they all seem to say
nsresult rv = ResolveAndStat(PR_TRUE);
;-)

> We could always change it so that we use the "/n" option.
> If we are opening a new window then it should always select the file.
OK, but just to clarify, this is just in the /select case.

>> I'm still not keen on GetForegroundWindow() because it may not be ours.
No comment on this one?

Comment 43

[Brodie]

> nsresult rv = ResolveAndStat(PR_TRUE);

Ok. I'll join the lusting throng and do it that way too.

>> I'm still not keen on GetForegroundWindow() because it may not be ours.
> No comment on this one?

Sorry, I missed that one. I just copied the existing code, but I also didn't
like that code when I saw it. What about if we just parent any dialog to the
desktop window instead? GetDesktopWindow()

Comment 44

[:deanis74]

> >> I'm still not keen on GetForegroundWindow() because it may not be ours.
> > No comment on this one?
> 
> Sorry, I missed that one. I just copied the existing code, but I also didn't
> like that code when I saw it. What about if we just parent any dialog to the
> desktop window instead? GetDesktopWindow()

Based on comment 10 and comment 11, I say pass NULL.

I'm also fine with using /n all the time.  That would give us predictable behavior.

Comment 45

[Brodie]

The way I've found the win32 API is that NULL for a hwnd is the same as 
GetDesktopWindow() when it needs a parent anyhow, so passing NULL for the 
parent is no different to passing the desktop window anyhow. I have no problem 
with that change.

As for always using "/n", I think that if we should only use it when we are 
specifying the "/select" parameter, since it seems that 2K doesn't select an 
object when it doesn't open the window. I guess this would be consistent with 
the Microsoft aim to prevent non-active windows from modifying the current 
users actions. i.e. if Mozilla isn't active at the time that it tries to open 
the window, then it shouldn't try to modify the currently selected object in 
explorer. This behaviour changed in 2K. 

I have no problemm with specifying /n when we also specify /select. However, 
when we are opening a folder (is what I have specified is the correct 
behaviour?) then we should not specify /n. Is this agreeable?

Comment 46

[:deanis74]

I can live with that.

Comment 47

[Brodie]

Attachment: patch 6

Patch with requested changes.

Comment 48

[Brodie]

To address alecf's concerns in comment 36:

> I still want to know if there is a safer way of calling explorer

Using the full path to explorer.exe in the system directory is as safe as we can
get. All of Microsoft's own code either does this or just calls "explorer.exe"
without a path. Additionally the "explorer.exe" file is protected by Windows
File Protection on Win2K+, so there shouldn't be much chance of trojans on those
platforms.

> I'm trying to figure out why the old way wasn't good. I realize 
> it was complex, but it relied on system APIs rather than 
> passing specific (undocumented?) parameters to explorer.

It is complex, it uses undocumented API and it doesn't always work. This way is
simple, it uses documented parameters to explorer
<http://support.microsoft.com/support/kb/articles/Q130/5/10.asp> and it works.

I've searched for an in-process method of doing this but I can't find a way for
*all* windows versions. Microsoft must have seen that this functionality was
missing and has added the SHOpenFolderAndSelectItems() function, but available
only in XP.

Comment 49

[Alec Flett]

Comment on attachment 149464 [details] [diff] [review]
patch 6

sorry for the delay, I was out of town. 

thanks for doing the research on this one...

sr=alecf

Comment 50

[Brodie]

As I lack CVS write access, I would greatly appreciate if someone would check
this in for me. Regards, Brodie.

Note that this fixes this bug as well as bug 211894.

Comment 51

[[e-mail]]

Fix checked in.

Comment 52

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 149464 [details] [diff] [review]
patch 6

since this fixes bug 211894, this would be good to have on the 1.7 branch

Comment 53

[OstGote!]

Request blocking flag, see comment 52.

Comment 54

[Daniel Veditz [:dveditz]]

Thanks for bringing this to our attention, blocking 1.7.6/aviary1.0.1 per drivers.

Comment 55

[Daniel Veditz [:dveditz]]

Fix checked into aviary1.0.1 branch

Comment 56

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 149464 [details] [diff] [review]
patch 6

retroactive a=caillon for 1.0.1.

We will also need this for 1.7.6 so a=caillon for that as well.

Comment 57

[Daniel Veditz [:dveditz]]

retroactively marked, perhaps, but drivers consensus was reached prior to checkin.

Now checked in to 1.7 branch as well.