New issue
Advanced search Search tips

Issue 780575 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 780560



Sign in to add a comment

imageloader avoids copy at RegisterComponent&LoadComponent

Project Member Reported by xiaochu@chromium.org, Nov 1 2017

Issue description

imageloader currently makes a copy of component at /var/lib/imageloader/<component>/<version> which takes up more space than required. It frees up half space if imageloader can avoid copy at RegisterComponent and LoadComponent in place.

Register a component:
* Avoid calling imageloader RegisterComponent
* OnCustomInstall does nothing which requires no callback and current threading model works fine.

Load a component: 
* uses imageloader API LoadComponentAtPath to load a component directly in-place.
* cache component install path from browser_process_platform_part_chromeos (from ComponentReady) so that LoadComponentAtPath can use.

benefits:
1. fewer space requirement: component updater & imageloader shares the same copy.
2. no threading issue any more since time consuming copying no longer happens in OnCustomInstall (which has to be async).
 
Blocking: 780560
Description: Show this description
Description: Show this description
Description: Show this description
Description: Show this description
Cc: waff...@chromium.org kerrnel@chromium.org adlr@chromium.org
can you please review this?
Description: Show this description

Comment 8 by derat@chromium.org, Nov 29 2017

Has someone on the security side reviewed this? How will the paths be sanitized?
The path is provided by ComponentReady which is a callback called by component updater after a component is installed. LoadComponent verifies the integrity of component. 

Actually, component at the path that ComponentReady provides can always be copied by imageloader successfully and LoadComponent mount the copy with all the verification. In this case, the path provided by ComponentReady should be safe. What do you think kerrnel@?
Project Member

Comment 10 by bugdroid1@chromium.org, Nov 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/system_api/+/7092a3102eae259d6ccca85a8f68d0ebf78394d2

commit 7092a3102eae259d6ccca85a8f68d0ebf78394d2
Author: Xiaochu Liu <xiaochu@chromium.org>
Date: Thu Nov 30 09:13:16 2017

Add constant to imageloader dbus API

Add LoadComponentAtPath to imageloader dbus API.

BUG= chromium:780575 
TEST=None

Change-Id: I468586284afd077ce5f7b0c45a9a39eb68039013
Reviewed-on: https://chromium-review.googlesource.com/797359
Commit-Ready: Xiaochu Liu <xiaochu@chromium.org>
Tested-by: Xiaochu Liu <xiaochu@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>

[modify] https://crrev.com/7092a3102eae259d6ccca85a8f68d0ebf78394d2/dbus/service_constants.h

We don't sanitize the paths because ImageLoader will only load anything signed by Google, and then it's verified with dm-verity. 
Project Member

Comment 12 by bugdroid1@chromium.org, Nov 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d24ee15befd89c494b9e502cf49d25a662831ee

commit 9d24ee15befd89c494b9e502cf49d25a662831ee
Author: Xiaochu Liu <xiaochu@chromium.org>
Date: Thu Nov 30 20:53:05 2017

Roll src/third_party/cros_system_api/ 9b6a22544..7092a3102 (3 commits)

https://chromium.googlesource.com/chromiumos/platform/system_api.git/+log/9b6a22544458..7092a3102eae

$ git log 9b6a22544..7092a3102 --date=short --no-merges --format='%ad %ae %s'
2017-11-29 xiaochu Add constant to imageloader dbus API
2017-11-29 derat system_api: Document potential races in suspend readiness.
2017-11-27 allenvic smbprovider: Add ReadDirectoryOptions and GetMetadataEntryOptions

Created with:
  roll-dep src/third_party/cros_system_api

Bug= chromium:780575 
TEST=None

Change-Id: I0f633691030275ebf8d3e398efc9b9a568a69db9
Reviewed-on: https://chromium-review.googlesource.com/801211
Reviewed-by: Dan Erat <derat@chromium.org>
Commit-Queue: Xiaochu Liu <xiaochu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520673}
[modify] https://crrev.com/9d24ee15befd89c494b9e502cf49d25a662831ee/DEPS

Project Member

Comment 13 by bugdroid1@chromium.org, Dec 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a8fb3d1f496e54e2abca7fbde01257434957552f

commit a8fb3d1f496e54e2abca7fbde01257434957552f
Author: Xiaochu Liu <xiaochu@chromium.org>
Date: Tue Dec 05 06:07:01 2017

avoid copy in component registration

Skip imageloader RegisterComponent (component copy) in OnCustomInstall
and loads the copy in chrome.

Additions:
Add LoadComponentAtPath in ImageLoaderClient.
Save the (compatible) component path in browser_process.

BUG= chromium:780575 
TEST=unittest; install/load component successfully.

Change-Id: Ic14f7d33ee6c8971dd6326f39fda546c94959e14
Reviewed-on: https://chromium-review.googlesource.com/801535
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Dan Erat <derat@chromium.org>
Reviewed-by: Joshua Pawlicki <waffles@chromium.org>
Commit-Queue: Xiaochu Liu <xiaochu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521622}
[modify] https://crrev.com/a8fb3d1f496e54e2abca7fbde01257434957552f/chrome/browser/browser_process_platform_part_chromeos.cc
[modify] https://crrev.com/a8fb3d1f496e54e2abca7fbde01257434957552f/chrome/browser/browser_process_platform_part_chromeos.h
[modify] https://crrev.com/a8fb3d1f496e54e2abca7fbde01257434957552f/chrome/browser/component_updater/cros_component_installer.cc
[modify] https://crrev.com/a8fb3d1f496e54e2abca7fbde01257434957552f/chrome/browser/component_updater/cros_component_installer_unittest.cc
[modify] https://crrev.com/a8fb3d1f496e54e2abca7fbde01257434957552f/chromeos/dbus/fake_image_loader_client.cc
[modify] https://crrev.com/a8fb3d1f496e54e2abca7fbde01257434957552f/chromeos/dbus/fake_image_loader_client.h
[modify] https://crrev.com/a8fb3d1f496e54e2abca7fbde01257434957552f/chromeos/dbus/image_loader_client.cc
[modify] https://crrev.com/a8fb3d1f496e54e2abca7fbde01257434957552f/chromeos/dbus/image_loader_client.h

Status: Fixed (was: Untriaged)
thanks all!
Status: Archived (was: Fixed)

Sign in to add a comment