668038 - nsFilePicker.cpp (WIN32) Returned filenames like "C:filename.ext" are not canonicalized

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Dan Weiss]

I was looking into what caused Bug 639251, where it puts invalid paths into the download manager.  After looking around, I decided to look at the code for the file picker.

In the function "nsFilePicker::ShowW" of the file "windows/nsFilePicker.cpp", it calls GetSaveFileNameW.  Then it returns the exact path string (or multiple path strings) back to the caller with no validation beyond what Windows provides.  The return value is supposed to be a canonicalized path, but this is not always the case.

Some paths pass the built-in Windows validation, but mess up later on in Firefox, such as "C:file.html".  When you enter this into the Save dialog box, it returns the path as "C:./file.html".  This breaks the download manager, which proceeds to throw "file://C:./file.html" into the database, and it can't find the file it just saved.

A solution would be to call PathSearchAndQualifyW on each path returned, so all paths become full absolute paths.

Comment 1

[Brian R. Bondy [:bbondy]]

Attachment: Patch for canonicalized file paths when saving

This patch will try to get a corrected path first and if possible will use that.  Otherwise it will fall back to the way it used to work.  This only applies to when saving files from the nsFilePicker.

Comment 2

[[e-mail]]

Can you provide STR for how to trigger this?

Comment 3

[Brian R. Bondy [:bbondy]]

You could use JS to instantiate an nsFilePicker but it's easiest just to trigger it by right clicking on an image and selecting save image as...
Paths like c:file.png won't be accepted in the Windows 7 file picker, so I had to use Windows XP to reproduce the problem and test the fix.  Windows XP accepts paths like c:file.png and reproduces the bug.

I setup a VM for winxp and then just launched the nightly via a UNC path so that I didn't need to actually build on an XP VM.

Comment 4

[[e-mail]]

I was also able to reproduce this when opening files on XP.

Comment 5

[[e-mail]]

Comment on attachment 545099 [details] [diff] [review]
Patch for canonicalized file paths when saving

In fact, you can get all sorts of junk past the XP filepicker; it draws the line at mixing forward slashes with subfolders, but you can work around that with an extra "./" e.g. D:./foo/bar.gif; worse still is that you can do that in the multiple file picker, where the path needs to be qualified relative to the returned "parent" folder, except in the case of returning a single file. So just doing this in the case of the Save picker looks wrong.

Side note: presumably the file buffer only needs to be MAX_PATH long.

Comment 6

[Brian R. Bondy [:bbondy]]

Thanks for the review.

So to do:
- Use athSearchAndQualifyW for all file picker dialogs and not just save file dialogs
- Use MAX_PATH buffer size instead

Anything else I missed besides those 2?

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: Patch for canonicalized file paths when saving

Fixed so that we now canonicalize paths for all dialogs including multiple selection files.

I also found/fixed another bug that has always been in FF (WinXP again). 

If you select multiple files but specify the full path for each, it would still prepend the directory.
So for example if you specified:
 "C:\data.txt" "C:\data2.txt" 
Then it would be accepted and the result would be C:\C:\data.txt C:\C:\data2.txt

This is fixed so that the directory is only pre-pended when the path is relative.

Comment 8

[[e-mail]]

Comment on attachment 546788 [details] [diff] [review]
Patch for canonicalized file paths when saving

Can't try this out right now but will comment on coding style anyway.

>+#include <Shlwapi.h>
MinGW compiler needs system includes to be in lower case.

>+  if(PathSearchAndQualifyW(aInPath, qualifiedFileBuffer, MAX_PATH)) {
Nit: space after if

>+  void AssignCanonicalizedPath(const PRUnichar *aInPath, nsString &aOutPath);
This should be a class or static method, because it doesn't refer to any of the filepicker member variables. (I'm also not a fan of the name but neither do I consider myself capable of providing a better one.)

Comment 9

[[e-mail]]

There was one case where I was able to fool it.

My default file picker folder was on a network drive, and I tried to open "C:boot.ini", but I actually got "C:\Documents and Settings\Neil\boot.ini" as the result. Then when I tried again, the default folder was now on the C: drive, and I couldn't open "C:boot.ini", but I could open "C:untitled.txt" which opened the file "C:\Documents and Settings\Neil\untitled.txt" ...

Comment 10

[Brian R. Bondy [:bbondy]]

Attachment: Patch for canonicalized file paths when saving

Fixed as per review comments.  

(Not including Comment 9 yet)

Comment 11

[Brian R. Bondy [:bbondy]]

Are you running XP SP3 and sure the patch built successfully? 

When I use this:
<input size='40' type='file'>
  
And I try to open c:boot.ini it will resolve to C:\boot.ini for me.
I also tried to save a file to C:\boot.ini and right click on the download manager and it reveals to C:\boot.ini as it should.

I did try putting my folder to a network UNC path first as you mentioned.

Comment 12

[[e-mail]]

(In reply to comment #11)
> Are you running XP SP3 and sure the patch built successfully?
Yes. Without the patch, opening C:boot.ini produces an invalid path.

> When I use this:
> <input size='40' type='file'>
>   
> And I try to open c:boot.ini it will resolve to C:\boot.ini for me.
> I also tried to save a file to C:\boot.ini and right click on the download
> manager and it reveals to C:\boot.ini as it should.
> 
> I did try putting my folder to a network UNC path first as you mentioned.
I can try again tomorrow. It may depend on what the previous selection was.

Comment 13

[Brian R. Bondy [:bbondy]]

OK thanks, any extra steps to get your results would be appreciated.

Comment 14

[[e-mail]]

Sadly the build I tested this on has lost its Internet connection, and I can't quite remember if these are steps that go wrong:

1. Start opening a file. Filepicker opens at My Documents.
2. Navigate to D:\SomeDir
3. Open "D:SomeFile.txt"

Expected results: D:\SomeDir\SomeFile.txt

Actual result: D:\SomeFile.txt

I think this must result from the use of OFN_NOCHANGEDIR.

Comment 15

[Brian R. Bondy [:bbondy]]

Thanks! I'll take a look and submit a new patch when fixed.

Comment 16

[Brian R. Bondy [:bbondy]]

Attachment: Patch for canonicalized file paths when saving + fix for c:path

As you thought the problem was indeed with OFN_NOCHANGEDIR. 

The fix is to not use this flag and instead do the reverting of the working directory manually.  Since there are several exit points in that function I used an RAII class to do the work.  

If there are any errors getting the working directory, it will fall back to using OFN_NOCHANGEDIR.

Comment 17

[[e-mail]]

Comment on attachment 549803 [details] [diff] [review]
Patch for canonicalized file paths when saving + fix for c:path

Patch looks good, but I need to remember to test this on my XP build. So here's some bikeshedding to pass the time.

>+nsFilePicker::GetFixedPath(const PRUnichar *aInPath, nsString &aOutPath)
"Fixed" doesn't really explain what it does. GetQualifiedPath perhaps?

>+  if (GetCurrentDirectoryW(bufferLength, mOldWorkingDir) == 0)
>+  {
File style appears to be braces on the end of the previous line.

>+bool EnsureWorkingPathRAII::WorkingDirectoryObtained() const
Working path or working directory? You seem to be using Path more than Directory in the rest of the code. Perhaps call it HasWorkingPath()?

>+{
>+  return mOldWorkingDir != NULL;
Might be worth inlining this.

>+// The constructor will obtain the working path, the destructor
>+// will set the working path back to what it used to be.
>+class EnsureWorkingPathRAII
I think Mozilla style is to call this AutoRestoreWorkingPath

>+  nsAutoArrayPtr<PRUnichar> mOldWorkingDir;
I would have thought it was clear that this was the original path, so you can just call this mWorkingPath.

Comment 18

[[e-mail]]

Comment on attachment 549803 [details] [diff] [review]
Patch for canonicalized file paths when saving + fix for c:path

Rather confusingly, some (but not all) of your added lines are in CRLF format (they should all be in LF format). r=me with that and my above nits fixed.

Comment 19

[Brian R. Bondy [:bbondy]]

I have a bad habit of pasting from notepad into VS and that produces the problem.  Jimm corrected me recently on this for a different patch, but my previous patches still had the problem.  I'll fix and r? you.

Comment 20

[Brian R. Bondy [:bbondy]]

Attachment: Patch for canonicalized file paths when saving + fix for c:path

Comment 21

[[e-mail]]

Still waiting for a reply to comment 17...

Comment 22

[Brian R. Bondy [:bbondy]]

Attachment: Patch for canonicalized file paths when saving + fix for c:path

Implemented Comment 17 review comments.

Comment 23

[[e-mail]]

Comment on attachment 550644 [details] [diff] [review]
Patch for canonicalized file paths when saving + fix for c:path

>+  inline PRBool AutoRestoreWorkingPath::HasWorkingPath() const
This was bool before, which was fine! I guess it doesn't really make any difference. And I thought you didn't need the inline here either.

Comment 24

[Brian R. Bondy [:bbondy]]

I noticed that no other bool was used in the file but many PRBools were used.

> And I thought you didn't need the inline here either.

Sorry maybe I misunderstood the following from Comment 17:
 
>+{
>+  return mOldWorkingDir != NULL;
> Might be worth inlining this.

Could you clarify?

Comment 25

[[e-mail]]

(In reply to Brian R. Bondy from comment #24)
> I noticed that no other bool was used in the file but many PRBools were used.
Well, I think people are working on s/PRBool/bool/. Try asking mwu on IRC.

> > And I thought you didn't need the inline here either.
> Sorry maybe I misunderstood
It turns out that some people like putting the inline keyword on methods that default to inline, just to make things more obvious. It's not a problem.

Comment 26

[Brian R. Bondy [:bbondy]]

Attachment: Patch for canonicalized file paths when saving + fix for c:path w/ replaced PRBool with bool

Comment 27

[[e-mail]]

Comment on attachment 552583 [details] [diff] [review]
Patch for canonicalized file paths when saving + fix for c:path w/ replaced PRBool with bool

You shouldn't feel the need to rerequest review for trivial changes like this, particularly now that you've got level 3 access!

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://hg.mozilla.org/mozilla-central/rev/9f28a8fec3cb