New issue
Advanced search Search tips

Issue 783883 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 14
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

Some files are leaking into base images that should not have otherwise

Project Member Reported by ahass...@chromium.org, Nov 10 2017

Issue description

I see at least these three files are being leaked into base images:

/usr/bin/bspatch
/usr/bin/bsdiff
/usr/lib64/bsdiff.so

These files should not be there and based on src/third_party/chromiumos-overlay/dev-util/bsdiff/bsdiff-9999.ebuild:52 we are removing them for target images:

	if [[ $(cros_target) == "target_image" ]]; then
		rm "${D}"/usr/bin/bsdiff "${D}"/usr/bin/bspatch "${D}"/usr/$(get_libdir)/bsdiff.so
	fi

Why is this happening? I had tested this in the past a few times and they were OK. But I just checked a caroline base image, they were there. Do we know what is going on here?


 

Comment 1 by vapier@chromium.org, Nov 10 2017

are you looking at release images or locally built images ?
It was a canary M64 caroline image. I got it from goldeneye.
Ok, I mounted the base image using ./mount_image.sh chromiumos_base_image.bin and entered dir_3. cat /etc/lsb-release shows :

CHROMEOS_RELEASE_DESCRIPTION=10089.0.0 (Official Build) dev-channel caroline test

It seems the base image is still a test image? I contains a lot of include files in /usr/include for example.

I'm going to download another one and test it.
Same for a veyron_minnie build:

CHROMEOS_RELEASE_DESCRIPTION=10106.0.0 (Official Build) dev-channel veyron_minnie test

I don't think this is supposed to be like this.

Comment 5 by vapier@chromium.org, Nov 10 2017

base/dev/test shouldn't matter here.  the image generation phase should have run that code.  i don't have an answer off hand why it's not working.
Looked into a signed recovery image, the situation was a bit better there. There was none of the binary files, but library files still exists. Specially these two that I'm sure they shouldn't be there:
/usr/lib/bsdiff.so
/usr/lib/puffdiff.so

Also usr/include didn't have all those header files, but still had proto files. I'm not sure if they are supposed to be there either. Aren't they compiled at build time?


Cc: adlr@chromium.org

Comment 8 by vapier@chromium.org, Nov 10 2017

src/scripts/common.sh has some common MASK variables for deleting files like *.h from the base image.  we don't nuke proto files though as i think there are scenarios where we need them in the rootfs ?
Cc: dgarr...@chromium.org
There is no single clear owner for build_image, and so I'm not surprised that something like this would have crept in without being noticed.
image building is community ownership at this point
Labels: -Restrict-View-Google
Components: Internals>Installer
I was trying these experiments to see if it solves the problem, but it wasn't. 
- crrev.com/c/792340
- crrev.com/c/792341
- crrev.com/c/786631

Then I came across the delta_generator IUSE flag in update_engine. If this delta_generator flag is on, then all these binaries and libraries that I don't want them to go to the base image, shouldn't go. But unfortunately the delta_generator is not enabled with cros_host. It is enabled for target SDK (chromiumos-overlay/profiles/targets/sdk/package.use) and moblab(project-moblab/profiles/base/package.use). This means if we want to do the same for puffin and bsdiff, we have to define a similar IUSE flag for them and add them to target sdk and moblab. Currently these files are leaking to images and since they are not removed, we are not getting any error for target sdk and moblab. The same applies to brotli too. Its encoder is like 600K and I rather not to put it in base images, but we are doing it now and it would have the same problem as puffin and bsdiff. Can you think of any simpler solution?

How about adding new flags for both bsdiff and puffin and turn them on from update_engine ebuild when the delta_generator flag is on? I don't have a solution for brotli yet unless I do the same mess in its ebuild. Is that even possible? For example in update_engine ebuild:

DELTA_GENERATOR_RDEPEND="
    dev-util/puffin delta_generator
    dev-util/bsdiff delta_generator
"

RDEPEND="
	delta_generator? ( ${DELTA_GENERATOR_RDEPEND} )
"


I also uploaded crrev.com/c/791828 which adds proto files in the mask. The precq passed, but haven't test it more thoroughly yet. But my guess is that we eventually will be able to remove them too since we are not exporting protoc binary in the images and proto files are always have to be compiled with protoc.
Status: Started (was: Untriaged)
Project Member

Comment 16 by bugdroid1@chromium.org, Oct 24

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/091dcebfd92cc13819e859bc30b670896a02fffb

commit 091dcebfd92cc13819e859bc30b670896a02fffb
Author: Amin Hassani <ahassani@chromium.org>
Date: Wed Oct 24 23:45:05 2018

update_engine et.al: Prevent library leaks into rootfs

- Uprev bsdiff and puffin:
-- Marking 9999 ebuild for dev-util/puffin as stable.
-- Marking 9999 ebuild for dev-util/bsdiff as stable.
-- Migrated to .gn build files.
-- They are exported as static_library (previously shared library). This allows
   better control and prevents libpuffdiff.so and libbsdiff.so to leak into

   rootfs.
-- Migrate them to EAPI=6 for fun.
-- Export and run *_test instead of *_unittest

- Removes delta_generator ebuild flag from everywhere and just use cros_host

BUG=chromium:891899
BUG= chromium:783883 
TEST=sudo FEATURES=test emerge bsdiff puffin update_engine
CQ-DEPEND=CL:1291835

Change-Id: I4531053d4148cb6b108759660e96799f1367fddf
Reviewed-on: https://chromium-review.googlesource.com/1295293
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>

[modify] https://crrev.com/091dcebfd92cc13819e859bc30b670896a02fffb/dev-util/bsdiff/bsdiff-9999.ebuild
[rename] https://crrev.com/091dcebfd92cc13819e859bc30b670896a02fffb/dev-util/bsdiff/bsdiff-4.3.1-r19.ebuild
[modify] https://crrev.com/091dcebfd92cc13819e859bc30b670896a02fffb/dev-util/puffin/puffin-9999.ebuild
[modify] https://crrev.com/091dcebfd92cc13819e859bc30b670896a02fffb/chromeos-base/update_engine/update_engine-9999.ebuild
[modify] https://crrev.com/091dcebfd92cc13819e859bc30b670896a02fffb/profiles/targets/sdk/package.use
[rename] https://crrev.com/091dcebfd92cc13819e859bc30b670896a02fffb/dev-util/puffin/puffin-1.0.0-r425.ebuild

Status: Available (was: Started)
This issue has been marked as started, but has no owner. Making available.
Owner: ahass...@chromium.org
Status: Fixed (was: Available)

Sign in to add a comment