We've been leaving /system rw for development purposes, which has let us get away with not doing this. We need to remount /system rw before starting to apply updates. Currently we may be saving downloaded updates to /system as well. Those need to go to /data or /sdcard.

Comment on attachment 648867 [details] [diff] [review] updater remount support - v1 Looking for a little feedback on this before we put it through the review paces. One open question I have right now: Should we / can we do anything if we leave the /system partition in read-write mode because of some error?

Comment on attachment 648867 [details] [diff] [review] updater remount support - v1 I don't understand what UpdRootD is doing here, but this approach in general is pretty much what I had in mind. Will need review with a fine-toothed comb though ;).

(In reply to Marshall Culpepper [:marshall_law] from comment #2) > One open question I have right now: Should we / can we do anything if we > leave the /system partition in read-write mode because of some error? If something goes wrong while remounting (other than EINTR), we should reboot.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #3) > I don't understand what UpdRootD is doing here, but this approach in general > is pretty much what I had in mind. It's the directory key for the update root, used by nsUpdateService and nsUpdateDriver. AFAICT, the current code requires this directory be returned by both our DirectoryProvider (so it can be served up to the DirectoryService), and nsXREDirProvider (so XRE bootup can query it before XPCOM is loaded) > Will need review with a fine-toothed comb though ;). +100. this remounting stuff is serious business :) On a side note -- I plan to get the current update mochitests running and working in B2G. I'll need to file a follow up for that..

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4) > (In reply to Marshall Culpepper [:marshall_law] from comment #2) > > One open question I have right now: Should we / can we do anything if we > > leave the /system partition in read-write mode because of some error? > > If something goes wrong while remounting (other than EINTR), we should > reboot. Hrm, that seems a little intrusive, though I guess no more so than restarting the process. One other question I remembered -- Do we plan to always run the b2g process as root? If not, remounting /system and rebooting the device may require a way to get elevated permissions..

Rebooting if remount fails is a security defense: we don't want to leave our system files +rw.

Comment on attachment 648867 [details] [diff] [review] updater remount support - v1 Review of attachment 648867 [details] [diff] [review]: ----------------------------------------------------------------- This looks good overall. Minusing mostly because of the error handling and also the usage of sscanf. Note that this allows for the /system partition to be remounted as RW at the same time that Gecko is running. I'm not entirely convinced that this is safe yet. You need to make sure that the permissions on everything under /system is sane at the very least. Also, I suggest you ask Robert Strong to review the final version of the patch. ::: toolkit/mozapps/update/updater/updater.cpp @@ +2161,5 @@ > + ReadWrite, > + Unknown > +} MountPermissions; > + > +class AutoSystemMounter Even though this is already a problem in the updater.cpp file, I would very much rather if you put all of this code in its own header and just #include it in updater.cpp #ifdef MOZ_WIDGET_GONK. updater.cpp is already too big to be grok'ed. @@ +2176,5 @@ > + RemountSystem(ReadOnly); > + free(mDevice); > + } > + > + void RemountSystem(MountPermissions permissions) Nit: make this and the following methods (with the exception of GetPermissions) private. @@ +2192,5 @@ > + UpdateMountStatus(); > + if (mPermissions != permissions) { > + // TODO: think about what to do if we leave /system in read-write > + // because of some failure.. > + LOG(("RemountSystem: /system permissions didn't match after remount: %s\n")); Note that you probably want to log what happened here regardless of the TODO comment. @@ +2198,5 @@ > + } > + } > + > + // Code derived and modified from AOSP system/core/adb/remount_service.c > + void UpdateMountStatus() This method ignores the failure cases. I think this should return a bool and let the caller handle the failure cases. @@ +2237,5 @@ > + char mountDir[strSize]; > + char mountPermissions[strSize]; > + > + int rv = sscanf(mount, "%255s %255s %*s %255s %*d %*d\n", > + mountDev, mountDir, mountPermissions); sscanf is unsafe, as it can write past the end of these buffers. I suggest you write the parsing code manually -- it should be pretty simple. @@ +2262,5 @@ > + } > + return true; > + } > + > + void MountSystem(unsigned long flags) This method also eats the errors. This seems wrong. @@ +2280,5 @@ > + readOnly ? "read-only" : "read-write", strerror(errno))); > + } > + } > + > + MountPermissions GetPermissions() Nit: make this const please. @@ +3027,1 @@ > LogFinish(); You need to verify that nothing passed this point tries to write to /system. I think that should be the case, just nothing this here to remind you to double-check this.

(In reply to Ehsan Akhgari [:ehsan] from comment #8) > @@ +2237,5 @@ > > + char mountDir[strSize]; > > + char mountPermissions[strSize]; > > + > > + int rv = sscanf(mount, "%255s %255s %*s %255s %*d %*d\n", > > + mountDev, mountDir, mountPermissions); > > sscanf is unsafe, as it can write past the end of these buffers. I suggest > you write the parsing code manually -- it should be pretty simple. Can you give more details? I don't see how that is possible, since a maximum length of 255 is given for each string (%255s), and each one is allocated on the stack with 256 characters. sscanf should only overrun the buffer when no length modifier is given (i.e. %s)

(In reply to comment #9) > (In reply to Ehsan Akhgari [:ehsan] from comment #8) > > @@ +2237,5 @@ > > > + char mountDir[strSize]; > > > + char mountPermissions[strSize]; > > > + > > > + int rv = sscanf(mount, "%255s %255s %*s %255s %*d %*d\n", > > > + mountDev, mountDir, mountPermissions); > > > > sscanf is unsafe, as it can write past the end of these buffers. I suggest > > you write the parsing code manually -- it should be pretty simple. > > Can you give more details? I don't see how that is possible, since a maximum > length of 255 is given for each string (%255s), and each one is allocated on > the stack with 256 characters. sscanf should only overrun the buffer when no > length modifier is given (i.e. %s) Hrm, you're right! I should have paid more attention. Please disregard my comment about sscanf.

This patch splits code out into automounter_gonk.cpp/h, and falls back to a reboot when /system is left in read-write. Note: In the testing for these updates, sometimes the updater process would fail to remount /system as read-only because the linker had mem-mapped b2g.bak/libmozglue.so, marking the filesystem as busy. Apparently the updater never actually required mozglue, but was being linked against it anyway for Gonk -- a consequence of using WRAP_LDFLAGS to hardcode -lmozglue. Clearing WRAP_LDFLAGS in the updater's Makefile.in seems to fix this issue.

(In reply to comment #11) > Note: In the testing for these updates, sometimes the updater process would > fail to remount /system as read-only because the linker had mem-mapped > b2g.bak/libmozglue.so, marking the filesystem as busy. Apparently the updater > never actually required mozglue, but was being linked against it anyway for > Gonk -- a consequence of using WRAP_LDFLAGS to hardcode -lmozglue. Clearing > WRAP_LDFLAGS in the updater's Makefile.in seems to fix this issue. I would appreciate if you can take that part out of this patch and file another bug and post it there. That is something that we need to fix, and I would hate for that fix to be removed in case we end up backing out this patch for example.

(In reply to Ehsan Akhgari [:ehsan] from comment #12) > I would appreciate if you can take that part out of this patch and file > another bug and post it there. Done :) Bug 781868 is the new bug

Sorry for the review lag here --- will do this tomorrow. I think trying to review this at 0200 is not a good idea ;).

Comment on attachment 650673 [details] [diff] [review] updater remount support - v2 The change to using UPDATE_ROOT_DIR on /data makes me a bit nervous. We can statically ensure that /system has enough space to download updates. We can't make the same guarantee about /data. What happens if we run out of space on /data? This won't be uncommon on lower-end devices when users install a lot of apps. We should chat with Ehsan and Rob about how reserving space / running out of space for updates works. >diff --git a/toolkit/mozapps/update/updater/automounter_gonk.cpp b/toolkit/mozapps/update/updater/automounter_gonk.cpp >+/** >+ * This class will remount the /system partition as read-write in Gonk to allow >+ * the updater write access. Upon destruction, /system will be remounted back to >+ * read-only. If something causes /system to remain read-write, this class will >+ * reboot the device and allow the system to mount as read-only. >+ * Let's move this comment to the header. >+GonkAutoMounter::GonkAutoMounter() : mDevice(NULL), mPermissions(Unknown) >+{ >+ bool result = RemountSystem(ReadWrite); >+ if (!result) { if (!Remount()) { >+bool >+GonkAutoMounter::RemountSystem(MountPermissions permissions) >+{ >+ bool result = UpdateMountStatus(); >+ if (!result) { if (!UpdateMountStatus()) >+ result = MountSystem(flags); >+ if (!result) { if (!MountSystem()) { >+ result = UpdateMountStatus(); >+ return result && mPermissions == permissions; return UpdateMountStatus() && mPermi... >+bool >+GonkAutoMounter::UpdateMountStatus() >+{ >+ FILE *mountsFile = NS_tfopen(kGonkMountsPath, "r"); >+ Need to handle NULL return and errno == EINTR, like so: do { mountsFile = NS_tfopen(); } while (mountsFile == NULL && errno == EINTR); >+ if (mountsFile == NULL) { if (!mountsFile) { >+ bool foundSystem = false; >+ const char *token = strtok(mountData, "\n"); Use strtok_r >+bool >+GonkAutoMounter::ProcessMount(const char *mount) >+{ >+ if (strcmp(kGonkSystemPath, mountDir) != 0) { Convention is |if (strcmp(...))| >+ if (strcmp("ro", mountPermissions) == 0) { Similarly, |if (!strcmp(...))| >+bool >+GonkAutoMounter::MountSystem(unsigned long flags) >+{ >+ bool readOnly = (flags & MS_RDONLY) > 0; (flags & MS_READONLY) >diff --git a/toolkit/mozapps/update/updater/automounter_gonk.h b/toolkit/mozapps/update/updater/automounter_gonk.h >+typedef enum { enum MountAccess >+ >+class GonkAutoMounter See note above; let's move the comment here. >diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp >+ { >+ GonkAutoMounter mounter; >+ if (mounter.GetPermissions() != MountPermissions::ReadWrite) { Log this condition; it's very worth knowing :). >diff --git a/toolkit/toolkit-makefiles.sh b/toolkit/toolkit-makefiles.sh >+ if [ "$OS_TARGET" != "Android" -o "$MOZ_WIDGET_TOOLKIT" = "gonk" ]; then "$MOZ_WIDGET_TOOLKIT" != "android" This looks really solid, nice work :). Most of the comments above are nits. I'd like to take a look at one more patch mostly to give myself one opportunity to catch something subtle ;).

(In reply to comment #15) > Comment on attachment 650673 [details] [diff] [review] > --> https://bugzilla.mozilla.org/attachment.cgi?id=650673 > updater remount support - v2 > > The change to using UPDATE_ROOT_DIR on /data makes me a bit nervous. > We can statically ensure that /system has enough space to download > updates. We can't make the same guarantee about /data. What happens > if we run out of space on /data? This won't be uncommon on lower-end > devices when users install a lot of apps. > > We should chat with Ehsan and Rob about how reserving space / running > out of space for updates works. We do not reserve any space. If we run out of space, we fail to download or install the update.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #15) > Comment on attachment 650673 [details] [diff] [review] > updater remount support - v2 > > The change to using UPDATE_ROOT_DIR on /data makes me a bit nervous. > We can statically ensure that /system has enough space to download > updates. We can't make the same guarantee about /data. What happens > if we run out of space on /data? This won't be uncommon on lower-end > devices when users install a lot of apps. We should probably address this in a new bug, since it will require fallback rules, which the current updater doesn't support. IIRC, our new flow was: - Download updates to /mnt/sdcard if there is enough space - If not, fall-back to /data/local - In either case, updates are extracted and staged into /system Thoughts? > > Need to handle NULL return and errno == EINTR, like so: > > do { > mountsFile = NS_tfopen(); > } while (mountsFile == NULL && errno == EINTR); > FWIW, none of the other NS_tfopen calls in updater.cpp handle EINTR in this way, so it would still be possible to interrupt i.e. in the check for update.status, or the open of update.log. Maybe this deserves another follow-up if it's critical to the safety of the standalone updater.. > >+ if (mountsFile == NULL) { > > if (!mountsFile) { Just following the styles set forth in updater.cpp here.. IMO the standalone updater code does vary quite a bit from other Gecko code though. > > >+ bool foundSystem = false; > >+ const char *token = strtok(mountData, "\n"); > > Use strtok_r I'm not sure I understand the benefit of using strtok_r here. Reentrancy / nested parse loops aren't something that I use here. Is there an extra level of safety provided by strtok_r I'm unaware of? :) > > >+bool > >+GonkAutoMounter::ProcessMount(const char *mount) > >+{ > > >+ if (strcmp(kGonkSystemPath, mountDir) != 0) { > > Convention is |if (strcmp(...))| In some parts of Gecko yes, in this part, the convention is == 0 / != 0 :) See updater.cpp > >diff --git a/toolkit/mozapps/update/updater/updater.cpp b/toolkit/mozapps/update/updater/updater.cpp > > >+ { > >+ GonkAutoMounter mounter; > >+ if (mounter.GetPermissions() != MountPermissions::ReadWrite) { > > Log this condition; it's very worth knowing :). The condition is logged in the GonkAutoMounter constructor :)

After further testing last week, I've found some new problems: - When the b2g process is restarted, Remounting back to read-only will sometimes fail because libmozglue.so is implicitly loaded (LD_PRELOAD) initially by the b2g process when it replaces itself w/ updater using execv. To work around this, I've decided to split the updater into a one-shot Gonk service that can be controlled independently of the b2g process. Along with a new patch, I will also have a pull request for this service in gonk-misc. - Another exit blocker has surfaced, this time it's the HAL orientation sensor being disabled twice, and the GonkSensor code doing a poll() anyway. More details can be found in Bug 784079.

This patch addresses cjones' review comments, and separates the b2g updater as it's own service. The matching gonk-misc pull request is here: https://github.com/mozilla-b2g/gonk-misc/pull/19

Apologies for the last patch -- after talking w/ mwu I realized that execv was copying the process environment, and taking LD_PRELOAD with it. I have a simpler fix in this patch that removes the need for a secondary update service. I've also closed my pull request, as it's no longer necessary.

(In reply to Marshall Culpepper [:marshall_law] from comment #17) > (In reply to Chris Jones [:cjones] [:warhammer] from comment #15) > > Comment on attachment 650673 [details] [diff] [review] > > updater remount support - v2 > > > > The change to using UPDATE_ROOT_DIR on /data makes me a bit nervous. > > We can statically ensure that /system has enough space to download > > updates. We can't make the same guarantee about /data. What happens > > if we run out of space on /data? This won't be uncommon on lower-end > > devices when users install a lot of apps. > > We should probably address this in a new bug, since it will require fallback > rules, which the current updater doesn't support. IIRC, our new flow was: > - Download updates to /mnt/sdcard if there is enough space > - If not, fall-back to /data/local > - In either case, updates are extracted and staged into /system > > Thoughts? > Yes, new bug. > > > > Need to handle NULL return and errno == EINTR, like so: > > > > do { > > mountsFile = NS_tfopen(); > > } while (mountsFile == NULL && errno == EINTR); > > > > FWIW, none of the other NS_tfopen calls in updater.cpp handle EINTR in this > way, so it would still be possible to interrupt i.e. in the check for > update.status, or the open of update.log. Maybe this deserves another > follow-up if it's critical to the safety of the standalone updater.. > Yes, we need to handle EINTR. New bug is fine, please file. > > >+ if (mountsFile == NULL) { > > > > if (!mountsFile) { > > Just following the styles set forth in updater.cpp here.. IMO the standalone > updater code does vary quite a bit from other Gecko code though. > Yay Mozilla. When in Rome ... > > > > >+ bool foundSystem = false; > > >+ const char *token = strtok(mountData, "\n"); > > > > Use strtok_r > > I'm not sure I understand the benefit of using strtok_r here. Reentrancy / > nested parse loops aren't something that I use here. Is there an extra level > of safety provided by strtok_r I'm unaware of? :) > It prevents later programmers from stepping on landmines when they edit this code, and prevents us from stepping on landmines the next time a new bionic commit does something dumb, e.g. Using strtok_r is also just a good habit to be in, code cleanliness wise.

Comment on attachment 653545 [details] [diff] [review] updater remount support - v4 r=me with strtok_r switch and followups filed for reserving space and handling EINTR.

> > > >+ bool foundSystem = false; > > > >+ const char *token = strtok(mountData, "\n"); > > > > > > Use strtok_r > > > > I'm not sure I understand the benefit of using strtok_r here. Reentrancy / > > nested parse loops aren't something that I use here. Is there an extra level > > of safety provided by strtok_r I'm unaware of? :) > > > > It prevents later programmers from stepping on landmines when they edit this > code, and prevents us from stepping on landmines the next time a new bionic > commit does something dumb, e.g. > > Using strtok_r is also just a good habit to be in, code cleanliness wise. I thought that an explanation about why "using strtok is bad" would be good. The real reason NOT to use strtok, is that strtok uses a global variable, which makes general use of it safe in single threaded processes, and not in multi-threaded processes. In a multi-threaded process, calling strtok(non-NULL, "\n") will always work, but you can never rely on calling strtok(NULL, "\n") because another thread may have called strtok(non-NULL, something) between your two calls to strtok. So, the general guidance is to always use strtok_r in multi-threaded programs, and only use strtok in single-threaded programs. Using strtok_r in single-threaded programs protects you down the road when the program becomes multi-threaded.

(In reply to Chris Jones [:cjones] [:warhammer] from comment #22) > r=me with strtok_r switch and followups filed for reserving space and > handling EINTR. woot! will do.. (In reply to Dave Hylands [:dhylands] from comment #23) > The real reason NOT to use strtok, is that strtok uses a global variable, > which makes general use of it safe in single threaded processes, and not in > multi-threaded processes. Thanks for the explanation Dave!

(In reply to Dave Hylands [:dhylands] from comment #23) > So, the general guidance is to always use strtok_r in multi-threaded > programs, and only use strtok in single-threaded programs. Using strtok_r in > single-threaded programs protects you down the road when the program becomes > multi-threaded. Further, even in single-threaded programs, if you have a tokenizer loop that uses strtok(), and your loop calls into helper code, and that helper code uses strtok() as well, your program is just as busted.

Updated to check all mount options in /proc/mounts, and use strtok_r. Found this while testing on otoro.

Follow ups have been filed: Bug 785124 for download storage requirements Bug 785138 for fopen EINTR handling

Comment on attachment 654635 [details] [diff] [review] updater remount support - v5 I just did a quick scan and this looks fine though I'd like bbondy to also take a look.

Comment on attachment 654635 [details] [diff] [review] updater remount support - v5 Review of attachment 654635 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/update/updater/updater.cpp @@ +2631,5 @@ > + // to be called > + > + { > + GonkAutoMounter mounter; > + if (mounter.GetAccess() != MountAccess::ReadWrite) { You may want to add a new error code in toolkit/mozapps/update/common/errors.h #define FILESYSTEM_MOUNT_ERROR 43 Then before you return 1;, call: WriteStatusFile(FILESYSTEM_MOUNT_ERROR); By doing this you'll get telemetry data for if that failure actually happens. You'll also ensure you have the wanted handling after a failed update. After this lands by the way you may want to keep an eye on https://metrics.mozilla.com/data/ for UPDATER_STATUS_CODES (43). You can test locally that it's being sent by using the about:telemetry addon. You'll also want to add the error here: toolkit/mozapps/update/nsUpdateService.js I think it should be handled in the same way as WRITE_ERROR is handled. > function handleUpdateFailure(update, errorCode) { > update.errorCode = parseInt(errorCode); > if (update.errorCode == WRITE_ERROR || > + ... FILESYSTEM_MOUNT_ERROR > update.errorCode == WRITE_ERROR_ACCESS_DENIED || > update.errorCode == WRITE_ERROR_SHARING_VIOLATION || > update.errorCode == WRITE_ERROR_CALLBACK_APP) { > // In some cases, we want to just show a simple alert dialog: > if (update.state == STATE_FAILED && > (update.errorCode == WRITE_ERROR || > + ... FILESYSTEM_MOUNT_ERROR > update.errorCode == WRITE_ERROR_ACCESS_DENIED || > update.errorCode == WRITE_ERROR_SHARING_VIOLATION || > update.errorCode == WRITE_ERROR_CALLBACK_APP)) { Please test what happens manually though if possible. You may also want to LaunchCallbackApp in this case.

Comment on attachment 654635 [details] [diff] [review] updater remount support - v5 Review of attachment 654635 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/mozapps/update/updater/automounter_gonk.cpp @@ +58,5 @@ > + // For more, see system/core/libcutils/android_reboot.c > + LOGE("Could not remount %s as read-only, forcing a system reboot.", > + kGonkSystemPath); > + > + if (android_reboot(ANDROID_RB_RESTART, 0, NULL) != 0) { Also before this we should flush the log

Incorporated feedback from Brian. Manually enabling the new error condition seems to work (though our error prompt just prints to the console right now).

Comment on attachment 655190 [details] [diff] [review] updater remount support - v6 Review of attachment 655190 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks good! ::: toolkit/mozapps/update/nsUpdateService.js @@ +141,5 @@ > +const UNEXPECTED_BZIP_ERROR = 39; > +const UNEXPECTED_MAR_ERROR = 40; > +const UNEXPECTED_BSPATCH_ERROR = 41; > +const UNEXPECTED_FILE_OPERATION_ERROR = 42; > +const FILESYSTEM_MOUNT_READWRITE_ERROR = 43 nit: Please add a semicolon after this line. ::: toolkit/mozapps/update/updater/automounter_gonk.cpp @@ +60,5 @@ > + > + if (android_reboot(ANDROID_RB_RESTART, 0, NULL) != 0) { > + LOGE("Safe system reboot failed, attempting to force"); > + > + if (reboot(RB_AUTOBOOT) != 0) { nit: One more LogFlush(); before this line

Try results for this patch w/ Brian's new suggestions: https://tbpl.mozilla.org/?tree=Try&rev=3cba1701265b A few Android failures that look related to device connectivity. Build looks otherwise good for other platforms. I'm going to go ahead and land this in inbound.