Closed Bug 1634267 Opened 4 years ago Closed 4 years ago

Handle kDevicePathSpecifier in IsBlockedUNCPaths

Categories

(Core :: Storage: Quota Manager, enhancement, P2)

Product:
Core
Component:
Storage: Quota Manager
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla78
Tracking Flags:
Tracking Status
firefox77 + wontfix
firefox78 --- fixed

People

(Reporter: tt, Assigned: tt)

References

Details

Attachments

(1 file)

Bug 1634267 - DOS device paths shouldn't be blocked by IsBlockedUNCPath;
47 bytes, text/x-phabricator-request
jcristau
: approval-mozilla-release-
Details | Review

See https://bugzilla.mozilla.org/show_bug.cgi?id=1626846#c50 and https://bugzilla.mozilla.org/show_bug.cgi?id=1626846#c51

I guess the action here is:

  • Verify case in https://bugzilla.mozilla.org/show_bug.cgi?id=1626846#c48 is because of IsBlockedUNCPaths by a quick fix try build so that we can be 100% sure.
  • The proposed fix is either adding the profiles path prefix with the kDevicePathSpecifier into the allow list or transforming the file paths with the kDevicePathSpecifier back.

Hi Pulse,

IIUC, I think this fix should at least fix the existing issue in your comment.
To be 100% sure, would you mind helping me to verify whether it is with a build for the quick fix in comment 1? Many thanks!

I will revise the patch and submit it in this bug to fix the potential issue meanwhile.

Attention: if you are happy to help, please do make a copy of the profile before helping me to test that.

The steps are:

  • Download the debug build in comment 6.
    In comment #6, there is a debug build for x64 Windows.
    In the panel on the bottom, you can find it in the "Job Details" section. And, it's in "artifact uploaded: target.zip".
  • Unzip the target.zip.
  • Open the Firefox binary in the unzipped files and run it with the profile
  • Try to reproduce the steps that you reported
  • Now, everything should be fine. If the extension is still broken, could you share the result here again?
  • Share the log here.
  • Check if network.file.disable_unc_paths is defined on the "about:config" (I assume that it's defined on your setting but just to be sure)

BTW, I tested all the extensions in https://bugzilla.mozilla.org/show_bug.cgi?id=1626846#c38, but none of them enable the IsBlockedUNCPaths. I guess there must be something else to cause IsBlockedUNCPaths to be enabled. (that is disabled by default)

Edit: updated based on comment 6

Flags: needinfo?(ausaitis)

Tom, the try build could have NS_WARN_IF for all NS_ERROR* in [1], that way we would be 100% sure what is failing.

[1] https://searchfox.org/mozilla-central/rev/b8fbb6ead517720daf0b0211115f407b4b951c74/xpcom/io/nsLocalFileWin.cpp#943

You can also add some warnings to FilePreferences::IsBlockedUNCPath (only for the try build).

try for adding some warnings: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fdcef9cc95923a4a64bd064c8d91689765ae97a

(I will update instructions in comment 3 once it's completed)

Edit: updated the url for addressing comment 4 (the previous one only adds warnings to IsBlockedUNCPaths and NS_APP_INDEXEDDB_PARENT_DIR)

Unfortunately, this build still has the same issue with dom.quotaManager.useDOSDevicePathSyntax set to true. I checked for the pref mentioned in Comment 3 network.file.disable_unc_paths which was also set to true.

Here's the paste

There's two parts to that paste, the first half is with dom.quotaManager.useDOSDevicePathSyntax set to true and the second half with the pref set to false.

BTW, here's what Temporary Containers says whit the pref set to true - Imgur

Flags: needinfo?(ausaitis)

(In reply to Pulse from comment #7)

Thanks for the help and reply!

Unfortunately, this build still has the same issue with dom.quotaManager.useDOSDevicePathSyntax set to true. I checked for the pref mentioned in Comment 3 network.file.disable_unc_paths which was also set to true.

Here's the paste

From this, it shows that:

  • At line 70, network.file.disable_unc_paths is true (code)
  • The file path prefix is NS_APP_USER_PROFILE_50_DIR (code)
  • The first error in QuotaManager is at line 79. It's here. That implies the original error was gone.
  • The error code is NS_ERROR_FILE_NOT_FOUND which can also be caused by file name too long issue.
  • There are many errors (WARNING: 'secondChar != L':' && (secondChar != L'\\' || firstChar != L'\\')', file /builds/worker/checkouts/gecko/xpcom/io/nsLocalFileWin.cpp, line 973), but there is no following error in QuotaManager so I assume that's from other modules which is not related to dom.quotaManager.useDOSDevicePathSyntax

I need to print out the file path in dom/quota/ActorsParent.cpp#2690 next time, but I should also need to figure out what I can get next time to avoid trapping in this loop (ask for verify -> find some issues -> ...)

I also notice that I didn't change this code right. I need to check if this can cause the issue you have in comment #7.

A question here is that whether you set network.file.disable_unc_paths manually or it was set by some extensions.

I had a little time this morning so I completely rebuilt my profile for the test. I used the same extensions and, as far as possible, the same settings. With the new profile network.file.disable_unc_paths wasn't enabled, in fact, it appears to be a hidden pref. However, adding the pref didn't seem to make any discernable difference.

Curiously, with the new settings and dom.quotaManager.useDOSDevicePathSyntax set to true things mostly worked correctly. For some reason, I had to reinstall Temporary Containers twice before it stopped showing the initialisation error I posted above but now that addon also seems to be working.

I have no idea why network.file.disable_unc_paths was set, perhaps it was something I did, although I don't remember making the change. Perhaps it was an addon I tried sometime in the past and no longer have installed.

What else can I do to help?

Update. Just so you know. With build 20200502214527, which is just today's Standard Nightly build, with dom.quotaManager.useDOSDevicePathSyntax true everything seems to be working correctly. If I add network.file.disable_unc_paths it breaks addons as described previously.

I can reproduce the issue with the debug build in comment 8 if I set the network.file.disable_unc_paths to true and use the temporary container extension. (Note that going the https://firefox-storage-test.glitch.me couldn't catch the issue)

If I use the debug build in comment 11, I couldn't reproduce the issue with setting the pref to true and using the extension. However, there are too many warning messages that jumping out (because I print all the files path in QuotaManager)

try for same stuff but without printing all file paths: https://treeherder.mozilla.org/#/jobs?repo=try&revision=363527cba0744a3ed3db9339d09c5aec6688fe63

https://firefox-storage-test.glitch.me can't catch it currently because it doesn't check "permanent" storage.

(In reply to Pulse from comment #10)

Update. Just so you know. With build 20200502214527, which is just today's Standard Nightly build, with dom.quotaManager.useDOSDevicePathSyntax true everything seems to be working correctly. If I add network.file.disable_unc_paths it breaks addons as described previously.

Thanks for providing more information!

However, adding the pref didn't seem to make any discernable difference.

Could you elaborate more on this? What value did you set to the pref? If you set the pref to false for the current profile, would the issue still exist?

So network.file.disable_unc_paths is hidden by default in Firefox, and, IIUC, it was designed for preventing files from smb url mostly.

Can you help me to do the steps again with downloading the debug build in comment 12?

(needinfo you in case you miss this)

Flags: needinfo?(ausaitis)

(In reply to Tom Tung [:tt, :ttung] from comment #12)

(Note that going the https://firefox-storage-test.glitch.me couldn't catch the issue)

Sorry for misleading!

The steps I did were:

  1. Verify storage operations are working by visiting https://firefox-storage-test.glitch.me
  2. Set the network.file.disable_unc_paths to true
  3. Restart FF
  4. Visit https://firefox-storage-test.glitch.me again

I should have cleared the storage for https://firefox-storage-test.glitch.me before the step 4.
If I clear the storage between step 3-4, https://firefox-storage-test.glitch.me shows that all storage operations are broken.

Hmm, it seems that I was wrong for the question (if a normal path should be considered as UNC path if prefixed with \?.) in https://bugzilla.mozilla.org/show_bug.cgi?id=1626846#c51

In this document: "A server or host name, which is prefaced by \\. The server name can be a NetBIOS machine name or an IP/FQDN address (IPv4 as well as v6 are supported)."

I don't think "?" is either a NetBIOS machine name or an IP/FQDN address. Besides, "?" is even not permitted for the NetBIOS machine name according to wiki.

Also, DOS device paths" and "UNC paths" are different sections in https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats.

I will update the patch based on this.
(Note that the build can still be used. I will probably only change the place for removing the prefix)

Attachment #9145416 - Attachment description: Bug 1634267 - Ensure IsBlockedUNCPath check the file paths without the "\\?\"; → Bug 1634267 - DOS device paths shouldn't be blocked by IsBlockedUNCPath;

Ran the test with the build in comment 12, as requested.

Everything worked even when I added network.file.disable_unc_paths and set to true.

Here's the paste

Flags: needinfo?(ausaitis)

(In reply to Pulse from comment #18)

Ran the test with the build in comment 12, as requested.

Everything worked even when I added network.file.disable_unc_paths and set to true.

Here's the paste

Thanks! That means the D73621 should fix the issue. I will address the requests from the reviewers so far.

Pushed by ttung@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1422721c3914 DOS device paths shouldn't be blocked by IsBlockedUNCPath; r=dom-workers-and-storage-reviewers,janv,froydnj

https://hg.mozilla.org/mozilla-central/rev/1422721c3914

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox78: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla78

Hi Pulse,

Could you verify if the issue disappears in the latest Nightly on your machine? Let me know if the issue still exists. Thanks!

Flags: needinfo?(ausaitis)

@Tom.

Everything has been working well up to and including the most recent Nightly build - Build ID 20200514211114 (15 May)

I've had network.file.disable_unc_paths and dom.quotaManager.useDOSDevicePathSyntax set true since I last posted.

Flags: needinfo?(ausaitis)

That verifies the issue is fixed. Thanks!

Given the information in https://bugzilla.mozilla.org/show_bug.cgi?id=1536796#c121 should we nominate this for a dot-release ridealong, since the next release is another 4 weeks out? (Not a recommendation, merely wondering if it makes sense :)

Flags: needinfo?(ttung)

Comment on attachment 9145416 [details]
Bug 1634267 - DOS device paths shouldn't be blocked by IsBlockedUNCPath;

Beta/Release Uplift Approval Request

A workaround is to either setting network.file.disable_unc_paths to false or setting dom.quotaManager.useDOSDevicePathSyntax to false. The first one is disabled and invisible by default. The latter one is the pref for protecting the feature in bug 1536796 and it's visible and true by default.

So, the impact would be that we need to ask users to set either of prefs above manually and wait until FF78 to avoid their add-ons from broken.

  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: Bug 1626513
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): The patches have been tested on Nightly and no any related issue have been reported so far. And the patches only cause the check that is enabled by network.file.disable_unc_paths to bypass the cases from bug 1536796.
  • String changes made/needed:
Flags: needinfo?(ttung)
Attachment #9145416 - Flags: approval-mozilla-release?

(In reply to Johann Hofmann [:johannh] from comment #25)

Given the information in https://bugzilla.mozilla.org/show_bug.cgi?id=1536796#c121 should we nominate this for a dot-release ridealong, since the next release is another 4 weeks out? (Not a recommendation, merely wondering if it makes sense :)

I think it makes sense and I requested uplift to Release for this patch (and it's base patch). I hope that's the correct procedure for nominating a dot-release ridealong. Let's let the release engineer evaluate if it's worthy to be a dot release or not.

I am tracking it for 77 in case we have a dot release to evaluate if we could take this patch as a ridealong.

status-firefox77: --- → affected
tracking-firefox77: --- → +

We're entering RC week for 78, this won't make 77

status-firefox77: affected → wontfix
Attachment #9145416 - Flags: approval-mozilla-release? → approval-mozilla-release-
Blocks: 1642931

Was this issue patched and verified?

We ran into this issue for the Temporary Containers plugin where the plugin can't load on a profile located on a network path.

So far no workarounds worked.

(In reply to marek.andreansky from comment #31)

Was this issue patched and verified?

We ran into this issue for the Temporary Containers plugin where the plugin can't load on a profile located on a network path.

So far no workarounds worked.

This patch is supposed to have solved the specific issue with the IsBlockedUNCPaths function, yes. However, this might not cover your specific use case. Please file a new bug and add more information like STR, log excerpt or similar in order to be able to take a look. Thank you for your support!

See Also: → 1733101
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: