snap-seccomp: link libseccomp statically to snap-seccomp #3579

Merged
merged 3 commits into from Jul 12, 2017

Conversation

Projects
None yet
5 participants
Collaborator

mvo5 commented Jul 11, 2017

This ensures we do not run into library version mismatches when
snap-seccomp is run from the core snap.

This also needs to be pushed to the release/2.26 branch

Assuming this fixes the issues, +1 (this approach was discussed in the forum)

Link libseccomp statically to snap-seccomp
This ensures we do not run into library version mismatches when
snap-seccomp is run from the core snap.

codecov-io commented Jul 11, 2017

Codecov Report

Merging #3579 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3579      +/-   ##
==========================================
- Coverage   76.82%   76.82%   -0.01%     
==========================================
  Files         379      379              
  Lines       26314    26314              
==========================================
- Hits        20216    20215       -1     
  Misses       4304     4304              
- Partials     1794     1795       +1
Impacted Files Coverage Δ
cmd/snap-seccomp/main.go 51.68% <ø> (ø) ⬆️
overlord/snapstate/snapstate.go 81.17% <0%> (-0.24%) ⬇️
overlord/ifacestate/helpers.go 65.54% <0%> (ø) ⬆️
cmd/snap/cmd_aliases.go 96% <0%> (+2%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75e3302...daefc01. Read the comment docs.

Just one comment, +1 on the idea, may need 0.001 adjustments.

cmd/snap-seccomp/main.go
@@ -19,6 +19,10 @@
package main
+//#cgo pkg-config: --static --libs libseccomp
@zyga

zyga Jul 11, 2017

Contributor

Ironically this is incorrect as you'd actually link with libseccomp dynamically. This outputs -lseccomp which is added to the compiler. The line below hard-codes that same (effective) library between two linker options that switch link mode. I think we should drop the pkg-config line and just keep the LDFLAGS line below.

I'll test this locally and push if you agree.

@mvo5

mvo5 Jul 11, 2017

Collaborator

Sure, that works for me

@zyga

zyga Jul 11, 2017

Contributor

Pushed, +1 and I'll merge it when green

@@ -21,7 +21,7 @@ Build-Depends: autoconf,
init-system-helpers,
libapparmor-dev,
libglib2.0-dev,
- libseccomp-dev (>= 2.1.1-1ubuntu1~trusty3),
+ libseccomp-dev (>= 2.1.1-1ubuntu1~trusty4),
@zyga

zyga Jul 11, 2017

Contributor

What is this update about?

@mvo5

mvo5 Jul 11, 2017

Collaborator

libseccomp-dev in trusty does not have a static library. I added it for trusty and uploaded a new SRU to unblock this, see https://launchpad.net/ubuntu/+source/libseccomp/2.1.1-1ubuntu1~trusty4

@zyga

zyga Jul 11, 2017

Contributor

❤️ thank you!

Looks good-enough to me. Ideally we'll embed the specific bits of libseccomp that we need into the binary proper, as we discussed, but if this works we can push it forward for now and figure the embedding later.

echo "Install test-snapd-tools and verify it works"
snap install test-snapd-tools
test-snapd-tools.echo hello | MATCH hello
+ # FIXME: use dirs.sh in 2.27+
+ echo "Ensure snap-seccomp is statically linked"
+ if ldd /usr/lib/snapd/snap-seccomp | MATCH libseccomp ; then
@niemeyer

niemeyer Jul 11, 2017

Contributor

Nice, thanks for testing this.

zyga added some commits Jul 11, 2017

cmd/snap-seccomp: drop pkg-config line
To justify I'll just paste my comment from the pull request:

    Ironically this is incorrect as you'd actually link with libseccomp
    dynamically. This outputs -lseccomp which is added to the compiler. The
    line below hard-codes that same (effective) library between two linker
    options that switch link mode. I think we should drop the pkg-config
    line and just keep the LDFLAGS line below.

It tests out fine and produces the correctly linked snap-seccomp

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

@mvo5 mvo5 merged commit d2fdb18 into snapcore:master Jul 12, 2017

1 of 7 checks passed

artful-amd64 autopkgtest finished (failure)
Details
xenial-amd64 autopkgtest finished (failure)
Details
xenial-i386 autopkgtest finished (failure)
Details
xenial-ppc64el autopkgtest finished (failure)
Details
yakkety-amd64 autopkgtest finished (failure)
Details
zesty-amd64 autopkgtest finished (failure)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment