Closed Bug 757965 Opened 13 years ago Closed 13 years ago

Don't attempt to stage the update if we don't have write access to the installation directory's parent

Categories

(Toolkit :: Application Update, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)

References

Details

Attachments

(2 files, 4 obsolete files)

From bug 757907 comment 8: > FWIW, I've hit this error twice today, on my work and home computer. Both of > them run Ubuntu 12.04 and have Firefox Nightly installed in /opt/firefox. > > /opt/firefox/updates/last-update.log: > > ...... > FINISH ADD Throbber-small.gif > succeeded > calling QuitProgressUI > Performing a replace request > SOURCE DIRECTORY /opt/firefox/updates/0 > DESTINATION DIRECTORY /opt/firefox/updated > Begin moving sourceDir (/opt/firefox) to tmpDir (/opt/firefox.bak) > rename_file: proceeding to rename the directory > rename_file: failed to rename file - src: /opt/firefox, > dst:/opt/firefox.bak, err: 13 > Moving sourceDir to tmpDir failed, err: 7 > failed: 7 > calling QuitProgressUI This probably happens because the user doesn't have write access to the parent directory, namely /opt in this case.
Assignee: nobody → ehsan
(In reply to Ehsan Akhgari [:ehsan] from comment #0) > This probably happens because the user doesn't have write access to the > parent directory, namely /opt in this case. Yes, that's right. I just have run "sudo chown unghost:unghost /opt" and then successufully updated my build. Sorry for misinformation, looks like I haven't owned /opt.
Blocks: 758101
Attached patch Patch (v1) (obsolete) — Splinter Review
Attachment #626691 - Flags: review?(robert.bugzilla)
Comment on attachment 626691 [details] [diff] [review] Patch (v1) >+XPCOMUtils.defineLazyGetter(this, "gCanStageUpdates", function aus_gCanStageUpdates() { >+ // If background updates are disabled, then just bail out! >+ if (!getPref("getBoolPref", PREF_APP_UPDATE_BACKGROUND, false)) { >+ return false; >+ } >+ >+#ifdef XP_UNIX >+ try { >+ var updateTestFile = getUpdateFile([FILE_PERMS_TEST]); >+ LOG("gCanStageUpdates - testing write access " + updateTestFile.path); >+ testWriteAccess(updateTestFile); >+#ifndef XP_MACOSX >+ // On Linux, we need to test the parent directory as well, as we need to be >+ // able to move files in that directory during the replacing step. >+ updateTestFile = getUpdateFile(['..', FILE_PERMS_TEST]); >+ LOG("gCanStageUpdates - testing write access " + updateTestFile.path); >+ testWriteAccess(updateTestFile); The test should be for creating a directory since that is what is actually needed. Also, shouldn't the tests be done for Windows as well for the non service case?
Attachment #626691 - Flags: review?(robert.bugzilla) → review-
Attached patch Patch (v2) (obsolete) — Splinter Review
Attachment #626691 - Attachment is obsolete: true
Attachment #626954 - Flags: review?(robert.bugzilla)
Comment on attachment 626954 [details] [diff] [review] Patch (v2) reviewed over irc
Attachment #626954 - Flags: review?(robert.bugzilla) → review-
Attached patch Patch (v3) (obsolete) — Splinter Review
Addressed the review comments over IRC. I switched to using createUnique to ensure that the temp file/directory names do not conflict with existing filesystem objects.
Attachment #626954 - Attachment is obsolete: true
Attachment #626967 - Flags: review?(robert.bugzilla)
Comment on attachment 626967 [details] [diff] [review] Patch (v3) >diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js >--- a/toolkit/mozapps/update/nsUpdateService.js >+++ b/toolkit/mozapps/update/nsUpdateService.js >@@ -306,25 +306,39 @@ XPCOMUtils.defineLazyGetter(this, "gOSVe > catch (e) { > // Not all platforms have a secondary widget library, so an error is nothing to worry about. > } > osVersion = encodeURIComponent(osVersion); > } > return osVersion; > }); > >+/** >+ * Tests to make sure that we can write to a given directory. >+ * If the test is successful, the function simply returns. Otherwise, it will >+ * throw an exception. >+ * >+ * @param updateTestFile a test file in the directory that needs to be tested. >+ * @param isDirectory whether a test directory should be created. Add @throws as used elsewhere in this file. >+ */ >+function testWriteAccess(updateTestFile, isDirectory) { nit: createFile or createDirectory would be a better param name. >+ const NORMAL_FILE_TYPE = Ci.nsILocalFile.NORMAL_FILE_TYPE; >+ const DIRECTORY_TYPE = Ci.nsILocalFile.DIRECTORY_TYPE; >+ if (updateTestFile.exists()) >+ updateTestFile.remove(false); This defeats the purpose of using createUnique that we discussed on irc in that if the parent of the installation directory in which we don't have any say as to what is located in that directory contains a file with the same name as the value of FILE_PERMS_TEST then we are going to try to delete it! You could go with another param to specify whether this code should try to delete updateTestFile. >+ updateTestFile.createUnique(isDirectory ? DIRECTORY_TYPE : NORMAL_FILE_TYPE, >+ isDirectory ? FileUtils.PERMS_DIRECTORY : FileUtils.PERMS_FILE); In the end I think this would all be much cleaner and clearer if the only time createUnique was used is when creating the directory in the parent of the installation directory. >+ updateTestFile.remove(false); >+} >+ > XPCOMUtils.defineLazyGetter(this, "gCanApplyUpdates", function aus_gCanApplyUpdates() { > try { >- const NORMAL_FILE_TYPE = Ci.nsILocalFile.NORMAL_FILE_TYPE; > var updateTestFile = getUpdateFile([FILE_PERMS_TEST]); > LOG("gCanApplyUpdates - testing write access " + updateTestFile.path); >- if (updateTestFile.exists()) >- updateTestFile.remove(false); >- updateTestFile.create(NORMAL_FILE_TYPE, FileUtils.PERMS_FILE); >- updateTestFile.remove(false); >+ testWriteAccess(updateTestFile, false); > #ifdef XP_WIN > var sysInfo = Cc["@mozilla.org/system-info;1"]. > getService(Ci.nsIPropertyBag2); > > // Example windowsVersion: Windows XP == 5.1 > var windowsVersion = sysInfo.getProperty("version"); > LOG("gCanApplyUpdates - windowsVersion = " + windowsVersion); > >@@ -382,31 +396,70 @@ XPCOMUtils.defineLazyGetter(this, "gCanA > # elevated again > */ > if (!userCanElevate) { > // if we're unable to create the test file this will throw an exception. > var appDirTestFile = FileUtils.getFile(KEY_APPDIR, [FILE_PERMS_TEST]); > LOG("gCanApplyUpdates - testing write access " + appDirTestFile.path); > if (appDirTestFile.exists()) > appDirTestFile.remove(false) >- appDirTestFile.create(NORMAL_FILE_TYPE, FileUtils.PERMS_FILE); >+ appDirTestFile.create(Ci.nsILocalFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE); > appDirTestFile.remove(false); > } > #endif //XP_WIN > } > catch (e) { > LOG("gCanApplyUpdates - unable to apply updates. Exception: " + e); > // No write privileges to install directory > return false; > } > > LOG("gCanApplyUpdates - able to apply updates"); > return true; > }); > >+XPCOMUtils.defineLazyGetter(this, "gCanStageUpdates", function aus_gCanStageUpdates() { >+ // If background updates are disabled, then just bail out! >+ if (!getPref("getBoolPref", PREF_APP_UPDATE_BACKGROUND, false)) { >+ LOG("gCanStageUpdates - unable to stage updates because the pref is disabled"); >+ return false; >+ } >+ >+#ifdef XP_WIN >+ if (getPref("getBoolPref", PREF_APP_UPDATE_SERVICE_ENABLED, false)) { >+ // No need to perform directory write checks, the maintenance service will >+ // be able to write to all directories. >+ LOG("gCanStageUpdates - able to stage updates because we'll use the service"); >+ return true; >+ } >+#endif >+ >+ try { >+ var updateTestFile = getUpdateFile([FILE_PERMS_TEST]); >+ LOG("gCanStageUpdates - testing write access " + updateTestFile.path); >+ testWriteAccess(updateTestFile, true); >+#ifndef XP_MACOSX >+ // On all platforms except Mac, we need to test the parent directory as well, >+ // as we need to be able to move files in that directory during the replacing >+ // step. >+ updateTestFile = getUpdateFile(['..', FILE_PERMS_TEST]); >+ LOG("gCanStageUpdates - testing write access " + updateTestFile.path); >+ testWriteAccess(updateTestFile, true); >+#endif >+ } >+ catch (e) { >+ LOG("gCanStageUpdates - unable to stage updates. Exception: " + e); >+ // No write privileges to install directory nit: Just use "No write privileges" since this can apply to the install directory or the parent directory of the install directory. Almost there!
Attachment #626967 - Flags: review?(robert.bugzilla) → review-
Attached patch Patch (v4)Splinter Review
Attachment #626967 - Attachment is obsolete: true
Attachment #627086 - Flags: review?(robert.bugzilla)
Comment on attachment 627086 [details] [diff] [review] Patch (v4) >+XPCOMUtils.defineLazyGetter(this, "gCanStageUpdates", function aus_gCanStageUpdates() { >+ // If background updates are disabled, then just bail out! >+ if (!getPref("getBoolPref", PREF_APP_UPDATE_BACKGROUND, false)) { >+ LOG("gCanStageUpdates - unable to stage updates because the pref is disabled"); nit: "gCanStageUpdates - staging updates is disabled by preference " + PREF_APP_UPDATE_BACKGROUND r=me with that and thanks!
Attachment #627086 - Flags: review?(robert.bugzilla) → review+
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Turns out at this fix was not quite sufficient as getUpdateDirFiles doesn't always return what I expect it to. Here, we need to check two directories: 1. The directory which will contain the "updated" directory. On Windows and Linux, this is always going to be the installation directory. If the service is not available on Windows, and we don't have write access to the installation directory, we should fall back to update on restart with a UAC prompt. On Mac, this is the bundle directory. 2. The directory where we will perform the replacing step. On Windows and Linux, this will be the parent of the installation directory. If the service is not available on Windows, and we don't have write access to the installation directory, we should fall back to update on restart with a UAC prompt. On Mac, this is also the bundle directory (as we don't replace the bundle directory itself), so this check is not required on Mac, since it's the same as step 1.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Note in comment 14 that if the service is enabled on Windows, we'll always assume that we have write access to both directories.
Attached patch New patch (obsolete) — Splinter Review
Attachment #628494 - Flags: review?(robert.bugzilla)
Comment on attachment 628494 [details] [diff] [review] New patch >commit 57dfc8aab8e42386e4981fde71607cc2039f3f98 >Author: Ehsan Akhgari <ehsan.akhgari@gmail.com> >Date: Wed May 30 17:57:05 2012 -0400 > > Bug 757965 - Properly test for write access before staging an update in the background; r=rstrong > > This patch makes sure that we test to make sure we can create a > directory in the installation directory and also in its parent (the > latter check only happens on Windows and Linux), so that we wouldn't try > to stage an update in the background if that's bound to fail if we don't > have the require write permissions. > >diff --git a/toolkit/mozapps/update/nsUpdateService.js b/toolkit/mozapps/update/nsUpdateService.js >index f8bc2db..6fe1d65 100644 >--- a/toolkit/mozapps/update/nsUpdateService.js >+++ b/toolkit/mozapps/update/nsUpdateService.js >@@ -430,15 +430,15 @@ XPCOMUtils.defineLazyGetter(this, "gCanStageUpdates", function aus_gCanStageUpda > #endif > > try { >- var updateTestFile = getUpdateFile([FILE_PERMS_TEST]); >+ var updateTestFile = getUpdatedDirParent(); >+ updateTestFile.append(FILE_PERMS_TEST); > LOG("gCanStageUpdates - testing write access " + updateTestFile.path); > testWriteAccess(updateTestFile, true); > #ifndef XP_MACOSX > // On all platforms except Mac, we need to test the parent directory as well, > // as we need to be able to move files in that directory during the replacing > // step. >- updateTestFile = getUpdateDirCreate([]); >- updateTestFile = updateTestFile.parent; >+ updateTestFile = getUpdatedDirParent().parent; > updateTestFile.append(FILE_PERMS_TEST); > LOG("gCanStageUpdates - testing write access " + updateTestFile.path); > updateTestFile.createUnique(Ci.nsILocalFile.DIRECTORY_TYPE, >@@ -551,6 +551,21 @@ function getUpdateDirCreate(pathArray) { > } > > /** >+ * Gets the directory where we'll store the updated directory while we stage >+ * updates in the background. This isn't true. You return the app dir on Windows / Linux and the parent directory is where it is stored. >+ * >+ * @return nsIFile object for the directory >+ */ >+function getUpdatedDirParent() { This just returns the app dir which is the dir to update on Windows and Linux not the updated dir parent. Does getInstallDirRoot or something similar clearer? In the comment you could have something like: Gets the root of the installation directory which is the application bundle directory on Mac OS X and the location of the application binary on all other platforms. >+ var dir = FileUtils.getDir(KEY_APPDIR, [], false); >+#ifdef XP_MACOSX >+ // On Mac, we store the Updated.app directory inside the bundle directory. >+ dir = dir.parent.parent; >+#endif >+ return dir; >+} >+ >+/** > * Gets the file at the specified hierarchy under the update root directory. > * @param pathArray > * An array of path components to locate beneath the directory r=me with the above or catch me on irc if you'd like to change the suggestions and please don't hesitate to let me know if I am wrong in the statements above.
Attachment #628494 - Flags: review?(robert.bugzilla) → review+
(In reply to Robert Strong [:rstrong] (do not email) from comment #17) > r=me with the above or catch me on irc if you'd like to change the > suggestions and please don't hesitate to let me know if I am wrong in the > statements above. This all makes sense to me. I'll fix these (and similar comments in the other bugs) when landing.
Attachment #628494 - Attachment is obsolete: true
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: