Skip to content

Commit

Permalink
Remove absolute registry paths from panic messages in wasm (#1594)
Browse files Browse the repository at this point in the history
  • Loading branch information
brson authored Nov 3, 2024
1 parent caf6e43 commit e44b89f
Show file tree
Hide file tree
Showing 4 changed files with 150 additions and 22 deletions.
37 changes: 22 additions & 15 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion cmd/soroban-cli/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ ulid = { workspace = true, features = ["serde"] }
strum = "0.17.1"
strum_macros = "0.17.1"
async-compression = { version = "0.4.12", features = ["tokio", "gzip"] }

shell-escape = "0.1.5"
tempfile = "3.8.1"
toml_edit = "0.21.0"
rust-embed = { version = "8.2.0", features = ["debug-embed"] }
Expand Down
131 changes: 126 additions & 5 deletions cmd/soroban-cli/src/commands/contract/build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use cargo_metadata::{Metadata, MetadataCommand, Package};
use clap::Parser;
use itertools::Itertools;
use std::{
borrow::Cow,
collections::HashSet,
env,
ffi::OsStr,
Expand All @@ -12,6 +13,8 @@ use std::{
};
use stellar_xdr::curr::{Limits, ScMetaEntry, ScMetaV0, StringM, WriteXdr};

use crate::{commands::global, print::Print};

/// Build a contract from source
///
/// Builds all crates that are referenced by the cargo manifest (Cargo.toml)
Expand Down Expand Up @@ -96,6 +99,8 @@ pub enum Error {
CopyingWasmFile(io::Error),
#[error("getting the current directory: {0}")]
GettingCurrentDir(io::Error),
#[error("retreiving CARGO_HOME: {0}")]
CargoHome(io::Error),
#[error("reading wasm file: {0}")]
ReadingWasmFile(io::Error),
#[error("writing wasm file: {0}")]
Expand All @@ -108,7 +113,9 @@ const WASM_TARGET: &str = "wasm32-unknown-unknown";
const META_CUSTOM_SECTION_NAME: &str = "contractmetav0";

impl Cmd {
pub fn run(&self) -> Result<(), Error> {
pub fn run(&self, global_args: &global::Args) -> Result<(), Error> {
let print = Print::new(global_args.quiet);

let working_dir = env::current_dir().map_err(Error::GettingCurrentDir)?;

let metadata = self.metadata()?;
Expand Down Expand Up @@ -154,15 +161,31 @@ impl Cmd {
cmd.arg(format!("--features={activate}"));
}
}
let cmd_str = format!(
"cargo {}",
cmd.get_args().map(OsStr::to_string_lossy).join(" ")

if let Some(rustflags) = make_rustflags_to_remap_absolute_paths(&print)? {
cmd.env("CARGO_BUILD_RUSTFLAGS", rustflags);
}

let mut cmd_str_parts = Vec::<String>::new();
cmd_str_parts.extend(cmd.get_envs().map(|(key, val)| {
format!(
"{}={}",
key.to_string_lossy(),
shell_escape::escape(val.unwrap_or_default().to_string_lossy())
)
}));
cmd_str_parts.push("cargo".to_string());
cmd_str_parts.extend(
cmd.get_args()
.map(OsStr::to_string_lossy)
.map(Cow::into_owned),
);
let cmd_str = cmd_str_parts.join(" ");

if self.print_commands_only {
println!("{cmd_str}");
} else {
eprintln!("{cmd_str}");
print.infoln(cmd_str);
let status = cmd.status().map_err(Error::CargoCmd)?;
if !status.success() {
return Err(Error::Exit(status));
Expand Down Expand Up @@ -282,3 +305,101 @@ impl Cmd {
fs::write(target_file_path, wasm_bytes).map_err(Error::WritingWasmFile)
}
}

/// Configure cargo/rustc to replace absolute paths in panic messages / debuginfo
/// with relative paths.
///
/// This is required for reproducible builds.
///
/// This works for paths to crates in the registry. The compiler already does
/// something similar for standard library paths and local paths. It may not
/// work for crates that come from other sources, including the standard library
/// compiled from source, though it may be possible to accomodate such cases in
/// the future.
///
/// This in theory breaks the ability of debuggers to find source code, but
/// since we are only targetting wasm, which is not typically run in a debugger,
/// and stellar-cli only compiles contracts in release mode, the impact is on
/// debugging is expected to be minimal.
///
/// This works by setting the `CARGO_BUILD_RUSTFLAGS` environment variable,
/// with appropriate `--remap-path-prefix` option. It preserves the values of an
/// existing `CARGO_BUILD_RUSTFLAGS` environment variable.
///
/// This must be done some via some variation of `RUSTFLAGS` and not as
/// arguments to `cargo rustc` because the latter only applies to the crate
/// directly being compiled, while `RUSTFLAGS` applies to all crates, including
/// dependencies.
///
/// `CARGO_BUILD_RUSTFLAGS` is an alias for the `build.rustflags` configuration
/// variable. Cargo automatically merges the contents of the environment variable
/// and the variables from config files; and `build.rustflags` has the lowest
/// priority of all the variations of rustflags that Cargo accepts. And because
/// we merge our values with an existing `CARGO_BUILD_RUSTFLAGS`,
/// our setting of this environment variable should not interfere with the
/// user's ability to set rustflags in any way they want, but it does mean
/// that if the user sets a higher-priority rustflags that our path remapping
/// will be ignored.
///
/// The major downside of using `CARGO_BUILD_RUSTFLAGS` is that it is whitespace
/// separated, which means we cannot support paths with spaces. If we encounter
/// such paths we will emit a warning. Spaces could be accomodated by using
/// `CARGO_ENCODED_RUSTFLAGS`, but that has high precedence over other rustflags,
/// so we could be interfering with the user's own use of rustflags. There is
/// no "encoded" variant of `CARGO_BUILD_RUSTFLAGS` at time of writing.
///
/// This assumes that paths are Unicode and that any existing `CARGO_BUILD_RUSTFLAGS`
/// variables are Unicode. Non-Unicode paths will fail to correctly perform the
/// the absolute path replacement. Non-Unicode `CARGO_BUILD_RUSTFLAGS` will result in the
/// existing rustflags being ignored, which is also the behavior of
/// Cargo itself.
fn make_rustflags_to_remap_absolute_paths(print: &Print) -> Result<Option<String>, Error> {
let cargo_home = home::cargo_home().map_err(Error::CargoHome)?;
let cargo_home = format!("{}", cargo_home.display());

if cargo_home.find(|c: char| c.is_whitespace()).is_some() {
print.warnln("Cargo home directory contains whitespace. Dependency paths will not be remapped; builds may not be reproducible.");
return Ok(None);
}

if env::var("RUSTFLAGS").is_ok() {
print.warnln("`RUSTFLAGS` set. Dependency paths will not be remapped; builds may not be reproducible.");
return Ok(None);
}

if env::var("CARGO_ENCODED_RUSTFLAGS").is_ok() {
print.warnln("`CARGO_ENCODED_RUSTFLAGS` set. Dependency paths will not be remapped; builds may not be reproducible.");
return Ok(None);
}

if env::var("TARGET_wasm32-unknown-unknown_RUSTFLAGS").is_ok() {
print.warnln("`TARGET_wasm32-unknown-unknown_RUSTFLAGS` set. Dependency paths will not be remapped; builds may not be reproducible.");
return Ok(None);
}

let registry_prefix = format!("{cargo_home}/registry/src/");
let new_rustflag = format!("--remap-path-prefix={registry_prefix}=");

let mut rustflags = get_rustflags().unwrap_or_default();
rustflags.push(new_rustflag);

let rustflags = rustflags.join(" ");

Ok(Some(rustflags))
}

/// Get any existing `CARGO_BUILD_RUSTFLAGS`, split on whitespace.
///
/// This conveniently ignores non-Unicode values, as does Cargo.
fn get_rustflags() -> Option<Vec<String>> {
if let Ok(a) = env::var("CARGO_BUILD_RUSTFLAGS") {
let args = a
.split_whitespace()
.map(str::trim)
.filter(|s| !s.is_empty())
.map(str::to_string);
return Some(args.collect());
}

None
}
2 changes: 1 addition & 1 deletion cmd/soroban-cli/src/commands/contract/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ impl Cmd {
match &self {
Cmd::Asset(asset) => asset.run().await?,
Cmd::Bindings(bindings) => bindings.run().await?,
Cmd::Build(build) => build.run()?,
Cmd::Build(build) => build.run(global_args)?,
Cmd::Extend(extend) => extend.run().await?,
Cmd::Alias(alias) => alias.run(global_args)?,
Cmd::Deploy(deploy) => deploy.run(global_args).await?,
Expand Down

0 comments on commit e44b89f

Please sign in to comment.