devtool: strip release binaries out of debug info #1957

iulianbarbu · 2020-06-12T11:33:45Z

Reason for This PR

Cargo adds by default DWARF sections to release binaries. It is recommended to strip release binaries out of debug information because of security concerns, but also because this information is not relevant in a production environment.

#1652

Description of Changes

The provided solution adds a devtool command (devtool strip [--target-libc (musl|gnu)]) which strips release binaries out of debug information (DWARF sections). This solution is based on GNU strip tool, which strips away only debug information (using --strip-debug).

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

All commits in this PR are signed (git commit -s).
The reason for this PR is clearly provided (issue no. or explanation).
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
Any newly added unsafe code is properly documented.
Any API changes are reflected in firecracker/swagger.yaml.
Any user-facing changes are mentioned in CHANGELOG.md.

alxiord

I think you should also shrink the binary size limits in the associated integ test, the new binaries should be smaller.

alxiord · 2020-06-12T15:03:02Z

.cargo/config

-	"-C", "link-arg=-lgcc",
-	"-C", "link-arg=-lfdt",
-]
+target-dir = "build/cargo_target"


nit: add newline

With these change, simply doing "cargo build --target aarch64-unknown-linux-musl
" won't work anymore :( we do not want that, do we?

Yes, you are right. I'll fix the cargo build issue.

alxiord · 2020-06-12T15:03:37Z

tools/devtool

+    # On aarch64 musl depends on some libgcc functions (i.e `__addtf3` and other `*tf3` functions) for logic that uses
+    # long double. Such functions are not builtin in the rust compiler, so we need to get them from libgcc.
+    # No need for the `crt_static` flag as rustc appends it by default.
+    [ $machine = "aarch64" ] && RUSTFLAGS="-C link-arg=-lgcc -C link-arg=-lfdt"


nit: you can collapse multiple linker args into link-args=-lgcc -lfdt

alxiord · 2020-06-12T15:35:43Z

tools/devtool

+    [ $machine = "aarch64" ] && RUSTFLAGS="-C link-arg=-lgcc -C link-arg=-lfdt"
+    # Strip away debug info, since cargo adds them by default event when building artifacts for release.
+    # See: https://github.com/rust-lang/cargo/issues/4122.
+    [ $profile = "release" ] && cargo_args+=("--release") && RUSTFLAGS="$RUSTFLAGS -C link-arg=-s"


I would leave it to the user to decide whether or not they want symbols. We might need the unstripped binary for debugging, and with this change the user can't force a build with symbols. How about a new devtool parameter (devtool build --release --keepsyms or smth) that lets us to both?

Good idea. To not break things, we might want to leave the default behavior to be as it is and provide a flag --strip_debug_info, to enforce debug info stripping.

I would not bother modifying the devtool for this since users always have the choice of using "cargo build --release" if they want the unaltered version of what cargo obtains.

This removes all symbols, not only debug symbols. This means stack traces would be difficult to read. Keeping a small subset of symbols via strip --discard-locals --strip-debug would allow you to remove most symbols without breaking stack traces. It also means you don't have to change the linking, which doesn't work the same on all toolchains.

I would not bother modifying the devtool for this since users always have the choice of using "cargo build --release" if they want the unaltered version of what cargo obtains.

@dianpopa , do you suggest to implement stripping support as the default way of building the release binaries? Now I would prefer adding optional support for stripping, when building with tools/devtool build --release, rather than adding by default stripping support while being open to break customers. Using cargo build --release to get unstripped binaries depends on setting up the rust dev environment.

I see two ways for implementing this:

adding a flag to devtool, which strips away debug info would not break customers and will provide optional support for stripping debug info.

stripping debug info by default, when building the release binaries with devtool, will drop the support for building release binaries the "cargo" way (with debug info). If users need to build release binaries with cargo build --release they would need to set up rust dev environment (rust, cargo, rustup, toolchains etc).

Please let me know which you prefer and why.

This removes all symbols, not only debug symbols. This means stack traces would be difficult to read. Keeping a small subset of symbols via strip --discard-locals --strip-debug would allow you to remove most symbols without breaking stack traces.

@petreeftime , stripping only what is necessary is a valid point. I wasn't aware of selective stripping until now. I'll try to incorporate your suggestion.

It also means you don't have to change the linking, which doesn't work the same on all toolchains.

This argument conflicts with the suggestion to use strip. strip is part of the GNU binutils, which currently provides GNU ld and GNU as, used by tools/devtool build --release. So even if we change the toolchain, we can not say that we move away from GNU binutils (this is a core part of Linux toolchains in general). To me it seems that GNU ld and GNU as (GNU binutils in general) are core parts of Linux toolchains and changing stripping alternative from linker flags to strip tool it shouldn't make any difference while GNU binutils is still used as a core part of the toolchain we build against.

sandreim · 2020-06-12T15:46:19Z

I am not sure we need to do this as part of the build system. I would leave the build system just as it is and move the stripping a step further into the release pipeline where the binary is stripped and a copy of the original is kept in cold storage for debugging purposes.
@firecracker-microvm/compute-capsule WDYT ?

dianpopa

Typos in commit body (i.e Othwerwise and RUSTBACKTRACE ->. RUST_BACKTRACE).
We need to update the documentation when instructing users about building firecracker
An integration test would be nice (i.e check that the file command does not contain not stripped)

dianpopa · 2020-06-15T09:27:05Z

.cargo/config

-	"-C", "link-arg=-lgcc",
-	"-C", "link-arg=-lfdt",
-]
+target-dir = "build/cargo_target"


With these change, simply doing "cargo build --target aarch64-unknown-linux-musl
" won't work anymore :( we do not want that, do we?

dianpopa · 2020-06-15T10:42:12Z

tools/devtool

+    [ $machine = "aarch64" ] && RUSTFLAGS="-C link-arg=-lgcc -C link-arg=-lfdt"
+    # Strip away debug info, since cargo adds them by default event when building artifacts for release.
+    # See: https://github.com/rust-lang/cargo/issues/4122.
+    [ $profile = "release" ] && cargo_args+=("--release") && RUSTFLAGS="$RUSTFLAGS -C link-arg=-s"


I would not bother modifying the devtool for this since users always have the choice of using "cargo build --release" if they want the unaltered version of what cargo obtains.

dianpopa · 2020-06-15T10:52:26Z

tools/devtool

+    # long double. Such functions are not builtin in the rust compiler, so we need to get them from libgcc.
+    # No need for the `crt_static` flag as rustc appends it by default.
+    [ $machine = "aarch64" ] && RUSTFLAGS="-C link-arg=-lgcc -C link-arg=-lfdt"
+    # Strip away debug info, since cargo adds them by default event when building artifacts for release.


nit: typo for "event"

andreeaflorescu · 2020-06-18T06:50:01Z

I am not sure we need to do this as part of the build system. I would leave the build system just as it is and move the stripping a step further into the release pipeline where the binary is stripped and a copy of the original is kept in cold storage for debugging purposes.
@firecracker-microvm/compute-capsule WDYT ?

+1 on this.

I was imagining this being implemented such that we have a devtool command that does building release binaries + stripping them at once (or as separate commands), without impacting in any way cargo build or ./tool/devtool build.

With that in mind, I wouldn't like us to modify ./cargo/config. Can this be done at the ./tools/devtool level only? Maybe with overriding RUSTFLAGS and exposing a new devtool command such as build-release-stripped (or some other better name). This command can then be used by an automated release process to create & upload the release artifacts.

iulianbarbu · 2020-06-18T10:37:24Z

I am not sure we need to do this as part of the build system. I would leave the build system just as it is and move the stripping a step further into the release pipeline where the binary is stripped

Why to separate the stripping of debug symbols (embedded into the release binaries at build time) in a separate step, inside the release pipeline? My understanding is that these debug symbols can be added or not when we build the executable files, so to me it seems that this is the step were we should provide optional build flags which can be used for building the binaries with our without debug info. Let me know what you think.

and a copy of the original is kept in cold storage for debugging purposes.

We wouldn't need to store binaries with debug symbols. We can get these binaries if we checkout on the release tag and build it.

iulianbarbu · 2020-06-18T11:10:00Z

+1 on this.

I was imagining this being implemented such that we have a devtool command that does building release binaries + stripping them at once (or as separate commands), without impacting in any way cargo build or ./tool/devtool build. With that in mind, I wouldn't like us to modify ./cargo/config.

Right, this makes sense. However, having something inside the devtool for building and stripping at once was not the suggestion of @sandreim , as far as I can tell. I will keep the .cargo/config, to not impact cargo build.

Can this be done at the ./tools/devtool level only? Maybe with overriding RUSTFLAGS and exposing a new devtool command such as build-release-stripped (or some other better name). This command can then be used by an automated release process to create & upload the release artifacts.

We can expose support for binaries stripping with a devtool/build flag, not necessary another build & strip command. What do you think?

andreeaflorescu · 2020-06-18T12:22:57Z

Right, this makes sense. However, having something inside the devtool for building and stripping at once was not the suggestion of @sandreim , as far as I can tell. I will keep the .cargo/config, to not impact cargo build.

I probably misunderstood then, sorry.

Can this be done at the ./tools/devtool level only? Maybe with overriding RUSTFLAGS and exposing a new devtool command such as build-release-stripped (or some other better name). This command can then be used by an automated release process to create & upload the release artifacts.

We can expose support for binaries stripping with a devtool/build flag, not necessary another build & strip command. What do you think?

I think we should keep ./devtool build as is. The current logic of ./devtool build is to pass all the flags directly to cargo build. I've personally abused this functionality (and I find it very nice) to do things such as building firecracker with gnu. I personally prefer to have another build-stripped command that doesn't pass the flags to cargo build directly.

sandreim · 2020-06-18T12:32:26Z

I think we should keep ./devtool build as is. The current logic of ./devtool build is to pass all the flags directly to cargo build. I've personally abused this functionality (and I find it very nice) to do things such as building firecracker with gnu. I personally prefer to have another build-stripped command that doesn't pass the flags to cargo build directly.

+1 on this. Let's just have devtool build unchanged. Looks like strip is available in the dev container, why not make it available via devtool strip

iulianbarbu · 2020-06-18T12:41:55Z

I think we should keep ./devtool build as is. The current logic of ./devtool build is to pass all the flags directly to cargo build. I've personally abused this functionality (and I find it very nice) to do things such as building firecracker with gnu. I personally prefer to have another build-stripped command that doesn't pass the flags to cargo build directly.

+1 on this. Let's just have devtool build unchanged. Looks like strip is available in the dev container, why not make it available via devtool strip

Correct. Looked within the container with tools/devtool shell -p and strip seems to be present in the container. I looked on the Dockerfile and couldn't see where it is pulled from. However, I guess it is shipped with FROM ubuntu:18.04. I'll incorporate your suggestions. Thanks.

iulianbarbu

3. An integration test would be nice (i.e check that the file command does not contain not stripped)

@dianpopa , I am not sure how can we test this easily. Our integration tests has other means for building firecracker binaries, others than devtool. Testing how devtool works would need devtool specific tests, outside the pytest framework. Testing the devtool stripping functionality in our integration tests means porting the same functionality there and then test it. I can do this, but this would not test the added functionality from devtool.

iulianbarbu · 2020-06-18T12:43:41Z

.cargo/config

-	"-C", "link-arg=-lgcc",
-	"-C", "link-arg=-lfdt",
-]
+target-dir = "build/cargo_target"


Yes, you are right. I'll fix the cargo build issue.

iulianbarbu · 2020-06-19T11:33:12Z

I think you should also shrink the binary size limits in the associated integ test, the new binaries should be smaller.

@aghecenco , adding stripping support for devtool does not interfere with building firecracker binaries inside integration tests. Do you suggest to strip the binaries in the integration tests as well and then update the binaries sizes in the corresponding test?

andreeaflorescu · 2020-06-22T07:22:20Z

I think you should also shrink the binary size limits in the associated integ test, the new binaries should be smaller.

@aghecenco , adding stripping support for devtool does not interfere with building firecracker binaries inside integration tests. Do you suggest to strip the binaries in the integration tests as well and then update the binaries sizes in the corresponding test?

The integration tests should be run with the binary that ends up being used by our customers.

iulianbarbu · 2020-06-22T20:46:33Z

I think you should also shrink the binary size limits in the associated integ test, the new binaries should be smaller.

@aghecenco , adding stripping support for devtool does not interfere with building firecracker binaries inside integration tests. Do you suggest to strip the binaries in the integration tests as well and then update the binaries sizes in the corresponding test?

The integration tests should be run with the binary that ends up being used by our customers.

I've stripped away debug_info, after integration tests build step and updated the test_binary_size.py test. There were similar suggestions to have this in our integration tests as well, from @dpopa and @aghecenco as well. PTAL!

iulianbarbu · 2020-06-23T07:19:51Z

tools/devtool

+        say "Stripped binaries placed under $cargo_bin_dir."
+    }
+
+    return $?


It needs one more change, before merging. I'll update soon.

Suggested change

return $?

return $ret

dianpopa · 2020-06-23T11:55:13Z

tools/devtool

@@ -385,6 +385,15 @@ cmd_help() {
    echo "        -l, --libc musl|gnu   Choose the libc flavor against which Firecracker will"
    echo "                              be linked. Default is musl."
    echo ""
+    echo "    strip [--target-libc musl|gnu]"
+    echo "        Strip from debug symbols the Firecracker release binaries."


nit: "Strip debug symbols from the.."?

dianpopa · 2020-06-23T12:28:10Z

CHANGELOG.md

@@ -25,6 +25,7 @@
  snapshot.
 - Added a new API call, `PUT /snapshot/load`, for loading a snapshot.
 - Added metrics for the vsock device.
+- Added devtool strip command to strip release binaries out of DWARF sections.


hm, "strip the binary out of?" We are not removing DWARF sections, right? Should we?

Anyhow, I'd change this to something more obvious "Added devtool strip command which removes debug symbols from the release binaries."

tools/devtool

gbionescu · 2020-06-23T12:32:56Z

tests/host_tools/cargo_build.py

@@ -69,6 +69,9 @@ def get_firecracker_binaries():
    fc_bin_path = "{}/{}".format(out_dir, FC_BINARY_NAME)
    jailer_bin_path = "{}/{}".format(out_dir, JAILER_BINARY_NAME)

+    cmd = "strip --strip-debug {} {}".format(fc_bin_path, jailer_bin_path)
+    utils.run_cmd(cmd)


Suggested change

utils.run_cmd(cmd)

utils.run_cmd(

"strip --strip-debug {} {}".format(fc_bin_path, jailer_bin_path))

Added `devtool strip` command. This command can strip debug symbols from Firecracker release binaries. The command can be parameterized by `--target-libc (musl|gnu)`, which sets the libc used by a specific toolchain (e.g. x86_64-uknown-linux-${libc_flavor}). Signed-off-by: Iulian Barbu <[email protected]>

iulianbarbu · 2020-06-23T17:54:22Z

@dianpopa, @gc-plp , I've addressed your feedback! PTAL.

dianpopa · 2020-06-24T08:53:04Z

@dpopa, @gc-plp , I've addressed your feedback! PTAL.

nit: @dianpopa :D
I will take a look!

iulianbarbu self-assigned this Jun 12, 2020

alxiord reviewed Jun 12, 2020

View reviewed changes

dianpopa requested changes Jun 15, 2020

View reviewed changes

dianpopa added Quality: Improvement labels Jun 15, 2020

iulianbarbu commented Jun 18, 2020

View reviewed changes

iulianbarbu force-pushed the strip_debug_info_from_release branch 6 times, most recently from de6456a to 5358b92 Compare June 18, 2020 19:58

iulianbarbu added Status: Awaiting review Indicates that a pull request is ready to be reviewed and removed Status: Author labels Jun 18, 2020

iulianbarbu force-pushed the strip_debug_info_from_release branch from 5358b92 to d818be3 Compare June 22, 2020 06:36

iulianbarbu force-pushed the strip_debug_info_from_release branch from d818be3 to f4fce84 Compare June 22, 2020 18:57

iulianbarbu force-pushed the strip_debug_info_from_release branch from f4fce84 to b55f915 Compare June 22, 2020 21:02

andreeaflorescu previously approved these changes Jun 23, 2020

View reviewed changes

iulianbarbu commented Jun 23, 2020

View reviewed changes

iulianbarbu dismissed andreeaflorescu’s stale review via b6c9fc8 June 23, 2020 07:21

iulianbarbu force-pushed the strip_debug_info_from_release branch 2 times, most recently from b6c9fc8 to 7592923 Compare June 23, 2020 07:22

dianpopa reviewed Jun 23, 2020

View reviewed changes

gbionescu reviewed Jun 23, 2020

View reviewed changes

iulianbarbu force-pushed the strip_debug_info_from_release branch from 7592923 to b9a87d7 Compare June 23, 2020 17:41

iulianbarbu force-pushed the strip_debug_info_from_release branch from b9a87d7 to 73321bb Compare June 23, 2020 17:50

dianpopa approved these changes Jun 24, 2020

View reviewed changes

Merge branch 'master' into strip_debug_info_from_release

dfc0f46

gbionescu approved these changes Jun 25, 2020

View reviewed changes

dianpopa merged commit de5ed9c into firecracker-microvm:master Jun 25, 2020

	utils.run_cmd(cmd)
	utils.run_cmd(
	"strip --strip-debug {} {}".format(fc_bin_path, jailer_bin_path))

devtool: strip release binaries out of debug info #1957

devtool: strip release binaries out of debug info #1957

Uh oh!

Conversation

iulianbarbu commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reason for This PR

Description of Changes

License Acceptance

PR Checklist

Uh oh!

alxiord left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iulianbarbu Jun 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandreim commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dianpopa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreeaflorescu commented Jun 18, 2020

Uh oh!

iulianbarbu commented Jun 18, 2020

Uh oh!

iulianbarbu commented Jun 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andreeaflorescu commented Jun 18, 2020

Uh oh!

sandreim commented Jun 18, 2020

Uh oh!

iulianbarbu commented Jun 18, 2020

Uh oh!

iulianbarbu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iulianbarbu commented Jun 19, 2020

Uh oh!

andreeaflorescu commented Jun 22, 2020

Uh oh!

iulianbarbu commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iulianbarbu commented Jun 12, 2020 •

edited

Loading

iulianbarbu Jun 17, 2020 •

edited

Loading

sandreim commented Jun 12, 2020 •

edited

Loading

iulianbarbu commented Jun 18, 2020 •

edited

Loading

iulianbarbu left a comment •

edited

Loading

iulianbarbu commented Jun 22, 2020 •

edited

Loading

iulianbarbu commented Jun 23, 2020 •

edited

Loading