Last Comment Bug 668038 - nsFilePicker.cpp (WIN32) Returned filenames like "C:filename.ext" are not canonicalized
: nsFilePicker.cpp (WIN32) Returned filenames like "C:filename.ext" are not can...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Win32 (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
: Jim Mathies [:jimm]
Mentors:
: 639251 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-28 14:42 PDT by Dan Weiss
Modified: 2011-08-29 10:13 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for canonicalized file paths when saving (1.86 KB, patch)
2011-07-10 15:40 PDT, Brian R. Bondy [:bbondy]
neil: review-
Details | Diff | Splinter Review
Patch for canonicalized file paths when saving (4.86 KB, patch)
2011-07-19 08:16 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for canonicalized file paths when saving (4.82 KB, patch)
2011-07-19 10:15 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for canonicalized file paths when saving + fix for c:path (8.73 KB, patch)
2011-08-01 08:19 PDT, Brian R. Bondy [:bbondy]
neil: review+
Details | Diff | Splinter Review
Patch for canonicalized file paths when saving + fix for c:path (8.71 KB, patch)
2011-08-03 18:59 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for canonicalized file paths when saving + fix for c:path (10.98 KB, patch)
2011-08-04 04:04 PDT, Brian R. Bondy [:bbondy]
neil: review+
Details | Diff | Splinter Review
Patch for canonicalized file paths when saving + fix for c:path w/ replaced PRBool with bool (10.97 KB, patch)
2011-08-11 20:11 PDT, Brian R. Bondy [:bbondy]
neil: review+
Details | Diff | Splinter Review

Description Dan Weiss 2011-06-28 14:42:35 PDT
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] 2011-07-10 15:40:54 PDT
Created attachment 545099 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-07-10 16:29:23 PDT
Can you provide STR for how to trigger this?
Comment 3 Brian R. Bondy [:bbondy] 2011-07-10 16:48:26 PDT
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 neil@parkwaycc.co.uk 2011-07-11 04:47:31 PDT
I was also able to reproduce this when opening files on XP.
Comment 5 neil@parkwaycc.co.uk 2011-07-11 05:12:54 PDT
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] 2011-07-11 05:19:20 PDT
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] 2011-07-19 08:16:57 PDT
Created attachment 546788 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-07-19 09:03:23 PDT
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 neil@parkwaycc.co.uk 2011-07-19 10:11:14 PDT
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] 2011-07-19 10:15:09 PDT
Created attachment 546817 [details] [diff] [review]
Patch for canonicalized file paths when saving

Fixed as per review comments.  

(Not including Comment 9 yet)
Comment 11 Brian R. Bondy [:bbondy] 2011-07-19 10:31:03 PDT
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 neil@parkwaycc.co.uk 2011-07-19 16:54:35 PDT
(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] 2011-07-19 16:55:48 PDT
OK thanks, any extra steps to get your results would be appreciated.
Comment 14 neil@parkwaycc.co.uk 2011-07-20 13:46:44 PDT
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] 2011-07-20 13:47:58 PDT
Thanks! I'll take a look and submit a new patch when fixed.
Comment 16 Brian R. Bondy [:bbondy] 2011-08-01 08:19:38 PDT
Created attachment 549803 [details] [diff] [review]
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 neil@parkwaycc.co.uk 2011-08-02 05:19:43 PDT
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 neil@parkwaycc.co.uk 2011-08-03 07:09:54 PDT
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] 2011-08-03 07:12:07 PDT
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] 2011-08-03 18:59:23 PDT
Created attachment 550580 [details] [diff] [review]
Patch for canonicalized file paths when saving + fix for c:path
Comment 21 neil@parkwaycc.co.uk 2011-08-04 02:39:59 PDT
Still waiting for a reply to comment 17...
Comment 22 Brian R. Bondy [:bbondy] 2011-08-04 04:04:48 PDT
Created attachment 550644 [details] [diff] [review]
Patch for canonicalized file paths when saving + fix for c:path

Implemented Comment 17 review comments.
Comment 23 neil@parkwaycc.co.uk 2011-08-04 05:39:08 PDT
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] 2011-08-04 05:40:54 PDT
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 neil@parkwaycc.co.uk 2011-08-09 05:53:52 PDT
(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] 2011-08-11 20:11:42 PDT
Created attachment 552583 [details] [diff] [review]
Patch for canonicalized file paths when saving + fix for c:path w/ replaced PRBool with bool
Comment 27 neil@parkwaycc.co.uk 2011-08-12 04:07:33 PDT
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 (⌚ UTC+1/+2) 2011-08-21 11:44:38 PDT
http://hg.mozilla.org/mozilla-central/rev/9f28a8fec3cb
Comment 29 Dan Weiss 2011-08-29 10:13:33 PDT
*** Bug 639251 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.