Skip to content
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .changes/fix-binary-patching.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
"tauri": patch:perf
"tauri-cli": patch:perf
"tauri-bundler": patch:perf
"@tauri-apps/cli": patch:perf
---

Change the way bundle type information is added to binary files. Intead of looking up value of a variable we simply look for default value.
78 changes: 38 additions & 40 deletions crates/tauri-bundler/src/bundle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
// SPDX-License-Identifier: MIT

mod category;
mod kmp;
#[cfg(target_os = "linux")]
mod linux;
#[cfg(target_os = "macos")]
Expand All @@ -15,29 +16,33 @@ mod windows;

use tauri_utils::{display_path, platform::Target as TargetPlatform};

const BUNDLE_VAR_TOKEN: &[u8] = b"__TAURI_BUNDLE_TYPE_VAR_UNK";
/// Patch a binary with bundle type information
fn patch_binary(binary: &PathBuf, package_type: &PackageType) -> crate::Result<()> {
match package_type {
#[cfg(target_os = "linux")]
PackageType::AppImage | PackageType::Deb | PackageType::Rpm => {
log::info!(
"Patching binary {:?} for type {}",
binary,
package_type.short_name()
);
linux::patch_binary(binary, package_type)?;
}
PackageType::Nsis | PackageType::WindowsMsi => {
log::info!(
"Patching binary {:?} for type {}",
binary,
package_type.short_name()
);
windows::patch_binary(binary, package_type)?;
}
_ => (),
}
let mut file_data = std::fs::read(binary).expect("Could not read binary file.");

if let Some(bundle_var_index) = kmp::index_of(BUNDLE_VAR_TOKEN, &file_data) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't look at it closely yet, but I think this would fail when patching the binary for the second bundle type right? (e.g .msi first and __TAURI_BUNDLE_TYPE becomes __TAURI_BUNDLE_TYPE_VAR_MSI already that when we do it for the NSIS bundle, we can no longer find __TAURI_BUNDLE_TYPE_VAR_UNK)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was an issue with double signing the binaries or patching already signed binary (I don't remember exactly) and the fix was to copy the original binary before each bundle so this should be fine. I tested on linux with standard deb,rpm and appImage and didn't get any errors.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you remember if after the bundling is all done the binary in target/release/ will be a fresh one or the one from the last patch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've double checked and something is not right. I will fix it so that we always patch and bundle a copy of the binary and at the end we're left with the original, unpatched one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still a bit concerned about this approach, like if we stopped at one of these steps (user pressed ctrl-c or we had an error somewhere), there's no way to recover/bundle again other than rebuilding the app's binary through cargo, and since we're matching any string in the binary, although unlikely, we risk replacing the wrong things

In my opinion, this method should not be used for the binary patching/main detection method

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we make sure to never touch the binary in target/release/ and always work on the copy somewhere else then you can just re-run tauri bundle but not sure if that's what you mean

Ah, didn't think of that, that should probably fix this problem, it might leave files in the temp folder but I guess if that's somewhere inside target, the next cargo clean removes it anyways

True but this is so unlikely i wouldn't even count this as an argument

Fair, I just don't really like that chance to be honest 😂

well, any ideas left for the patching idea?

I messed up the words when moving them, I was thinking it shouldn't be the 'main binary patching/detection' method (e.g. use the current one and adding this for a backup)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I messed up the words when moving them, I was thinking it shouldn't be the 'main binary patching/detection' method (e.g. use the current one and adding this for a backup)

ah my bad. I mean honestly fair enough! we can keep both 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  // When packaging multiple binary types, we make a copy of the unsigned main_binary so that we can
  // restore it after each package_type step. This avoids two issues:
  //  - modifying a signed binary without updating its PE checksum can break signature verification
  //    - codesigning tools should handle calculating+updating this, we just need to ensure
  //      (re)signing is performed after every `patch_binary()` operation
  //  - signing an already-signed binary can result in multiple signatures, causing verification errors
  let main_binary_reset_required = matches!(target_os, TargetPlatform::Windows)
    && settings.windows().can_sign()
    && package_types.len() > 1;

So I remembered correctly we do this but it's just for Windows right now. I'm assuming simply extending this approach to Linux will be enough, right? It's basically boils down to the same issue. I will work on this later today.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the future we should look into the "work on temp copy only" approach but that's not urgent

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So just to be clear, are we going to use this method for binary patching exclusively or do we want to use this as a fallback? (e.g. the find link section thing -> find the string and replace)

let bundle_type = match package_type {
crate::PackageType::Deb => b"__TAURI_BUNDLE_TYPE_VAR_DEB",
crate::PackageType::Rpm => b"__TAURI_BUNDLE_TYPE_VAR_RPM",
crate::PackageType::AppImage => b"__TAURI_BUNDLE_TYPE_VAR_APP",
crate::PackageType::Nsis => b"__TAURI_BUNDLE_TYPE_VAR_NSS",
crate::PackageType::WindowsMsi => b"__TAURI_BUNDLE_TYPE_VAR_MSI",
_ => {
return Err(crate::Error::InvalidPackageType(
package_type.short_name().to_owned(),
))
}
};

file_data[bundle_var_index..bundle_var_index + BUNDLE_VAR_TOKEN.len()]
.copy_from_slice(bundle_type);

std::fs::write(binary, &file_data)
.map_err(|e| crate::Error::BinaryWriteError(e.to_string()))?;
} else {
return Err(crate::Error::MissingBundleTypeVar);
}
Ok(())
}

Expand Down Expand Up @@ -92,22 +97,16 @@ pub fn bundle_project(settings: &Settings) -> crate::Result<Vec<Bundle>> {
.expect("Main binary missing in settings");
let main_binary_path = settings.binary_path(main_binary);

// When packaging multiple binary types, we make a copy of the unsigned main_binary so that we can
// restore it after each package_type step. This avoids two issues:
// We make a copy of the unsigned main_binary so that we can restore it after each package_type step.
// This allows us to patch the binary correctly and avoids two issues:
// - modifying a signed binary without updating its PE checksum can break signature verification
// - codesigning tools should handle calculating+updating this, we just need to ensure
// (re)signing is performed after every `patch_binary()` operation
// - signing an already-signed binary can result in multiple signatures, causing verification errors
let main_binary_reset_required = matches!(target_os, TargetPlatform::Windows)
&& settings.windows().can_sign()
&& package_types.len() > 1;
let mut unsigned_main_binary_copy = tempfile::tempfile()?;
if main_binary_reset_required {
let mut unsigned_main_binary = std::fs::File::open(&main_binary_path)?;
std::io::copy(&mut unsigned_main_binary, &mut unsigned_main_binary_copy)?;
}
let mut main_binary_copy = tempfile::tempfile()?;
let mut main_binary_orignal = std::fs::File::open(&main_binary_path)?;
std::io::copy(&mut main_binary_orignal, &mut main_binary_copy)?;

let mut main_binary_signed = false;
let mut bundles = Vec::<Bundle>::new();
for package_type in &package_types {
// bundle was already built! e.g. DMG already built .app
Expand All @@ -121,16 +120,7 @@ pub fn bundle_project(settings: &Settings) -> crate::Result<Vec<Bundle>> {

// sign main binary for every package type after patch
if matches!(target_os, TargetPlatform::Windows) && settings.windows().can_sign() {
if main_binary_signed && main_binary_reset_required {
let mut signed_main_binary = std::fs::OpenOptions::new()
.write(true)
.truncate(true)
.open(&main_binary_path)?;
unsigned_main_binary_copy.seek(SeekFrom::Start(0))?;
std::io::copy(&mut unsigned_main_binary_copy, &mut signed_main_binary)?;
}
windows::sign::try_sign(&main_binary_path, settings)?;
main_binary_signed = true;
}

let bundle_paths = match package_type {
Expand Down Expand Up @@ -172,6 +162,14 @@ pub fn bundle_project(settings: &Settings) -> crate::Result<Vec<Bundle>> {
package_type: package_type.to_owned(),
bundle_paths,
});

// Restore unsigne and unpatched binary
let mut modified_main_binary = std::fs::OpenOptions::new()
.write(true)
.truncate(true)
.open(&main_binary_path)?;
main_binary_copy.seek(SeekFrom::Start(0))?;
std::io::copy(&mut main_binary_copy, &mut modified_main_binary)?;
}

if let Some(updater) = settings.updater() {
Expand Down
59 changes: 59 additions & 0 deletions crates/tauri-bundler/src/bundle/kmp/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// Copyright 2016-2019 Cargo-Bundle developers <https://github.com/burtonageo/cargo-bundle>
// Copyright 2019-2024 Tauri Programme within The Commons Conservancy
// SPDX-License-Identifier: Apache-2.0
// SPDX-License-Identifier: MIT

// Knuth–Morris–Pratt algorithm
// based on https://github.com/howeih/rust_kmp
pub fn index_of(pattern: &[u8], target: &[u8]) -> Option<usize> {
let failure_function = find_failure_function(pattern);

let mut t_i: usize = 0;
let mut p_i: usize = 0;
let target_len = target.len();
let mut result_idx = None;
let pattern_len = pattern.len();

while (t_i < target_len) && (p_i < pattern_len) {
if target[t_i] == pattern[p_i] {
if result_idx.is_none() {
result_idx.replace(t_i);
}
t_i += 1;
p_i += 1;
if p_i >= pattern_len {
return result_idx;
}
} else {
if p_i == 0 {
p_i = 0;
t_i += 1;
} else {
p_i = failure_function[p_i - 1];
}
result_idx = None;
}
}
None
}

fn find_failure_function(pattern: &[u8]) -> Vec<usize> {
let mut i = 1;
let mut j = 0;
let pattern_length = pattern.len();
let end_i = pattern_length - 1;
let mut failure_function = vec![0usize; pattern_length];
while i <= end_i {
if pattern[i] == pattern[j] {
failure_function[i] = j + 1;
i += 1;
j += 1;
} else if j == 0 {
failure_function[i] = 0;
i += 1;
} else {
j = failure_function[j - 1];
}
}
failure_function
}
5 changes: 0 additions & 5 deletions crates/tauri-bundler/src/bundle/linux/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,3 @@ pub mod appimage;
pub mod debian;
pub mod freedesktop;
pub mod rpm;

mod util;

#[cfg(target_os = "linux")]
pub use util::patch_binary;
59 changes: 0 additions & 59 deletions crates/tauri-bundler/src/bundle/linux/util.rs

This file was deleted.

2 changes: 0 additions & 2 deletions crates/tauri-bundler/src/bundle/windows/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,3 @@ pub use util::{
NSIS_OUTPUT_FOLDER_NAME, NSIS_UPDATER_OUTPUT_FOLDER_NAME, WIX_OUTPUT_FOLDER_NAME,
WIX_UPDATER_OUTPUT_FOLDER_NAME,
};

pub use util::patch_binary;
72 changes: 0 additions & 72 deletions crates/tauri-bundler/src/bundle/windows/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,75 +77,3 @@ pub fn os_bitness<'a>() -> Option<&'a str> {
_ => None,
}
}

pub fn patch_binary(binary_path: &PathBuf, package_type: &crate::PackageType) -> crate::Result<()> {
let mut file_data = std::fs::read(binary_path)?;

let pe = match goblin::Object::parse(&file_data)? {
goblin::Object::PE(pe) => pe,
_ => {
return Err(crate::Error::BinaryParseError(
std::io::Error::new(std::io::ErrorKind::InvalidInput, "binary is not a PE file").into(),
));
}
};

let tauri_bundle_section = pe
.sections
.iter()
.find(|s| s.name().unwrap_or_default() == ".taubndl")
.ok_or(crate::Error::MissingBundleTypeVar)?;

let data_offset = tauri_bundle_section.pointer_to_raw_data as usize;
let pointer_size = if pe.is_64 { 8 } else { 4 };
let ptr_bytes = file_data
.get(data_offset..data_offset + pointer_size)
.ok_or(crate::Error::BinaryOffsetOutOfRange)?;
// `try_into` is safe to `unwrap` here because we have already checked the slice's size through `get`
let ptr_value = if pe.is_64 {
u64::from_le_bytes(ptr_bytes.try_into().unwrap())
} else {
u32::from_le_bytes(ptr_bytes.try_into().unwrap()).into()
};

let rdata_section = pe
.sections
.iter()
.find(|s| s.name().unwrap_or_default() == ".rdata")
.ok_or_else(|| {
crate::Error::BinaryParseError(
std::io::Error::new(std::io::ErrorKind::InvalidInput, ".rdata section not found").into(),
)
})?;

let rva = ptr_value.checked_sub(pe.image_base as u64).ok_or_else(|| {
crate::Error::BinaryParseError(
std::io::Error::new(std::io::ErrorKind::InvalidData, "invalid RVA offset").into(),
)
})?;

// see "Relative virtual address (RVA)" for explanation of offset arithmetic here:
// https://learn.microsoft.com/en-us/windows/win32/debug/pe-format#general-concepts
let file_offset = rdata_section.pointer_to_raw_data as usize
+ (rva as usize).saturating_sub(rdata_section.virtual_address as usize);

// Overwrite the string at that offset
let string_bytes = file_data
.get_mut(file_offset..file_offset + 3)
.ok_or(crate::Error::BinaryOffsetOutOfRange)?;
match package_type {
crate::PackageType::Nsis => string_bytes.copy_from_slice(b"NSS"),
crate::PackageType::WindowsMsi => string_bytes.copy_from_slice(b"MSI"),
_ => {
return Err(crate::Error::InvalidPackageType(
package_type.short_name().to_owned(),
"windows".to_owned(),
));
}
}

std::fs::write(binary_path, &file_data)
.map_err(|e| crate::Error::BinaryWriteError(e.to_string()))?;

Ok(())
}
16 changes: 3 additions & 13 deletions crates/tauri-bundler/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -96,24 +96,14 @@ pub enum Error {
#[error("Binary parse error: `{0}`")]
BinaryParseError(#[from] goblin::error::Error),
/// Package type is not supported by target platform
#[error("Wrong package type {0} for platform {1}")]
InvalidPackageType(String, String),
#[error("Wrong package type {0}")]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we could make breaking changes here

@FabianLars any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was only used by the binary patching code. I don't think any external code will use this error as it's very specific.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have downstream bundler users so ideally we don't break here. also not sure if we wanted to keep the old logic around or not @Legend-Master ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we keep InvalidPackageType the same and mark the other variants deprecated?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah yeah - my ping was a bit offtopic here. Just deprecate it, change the comment to say that it's unused and move on.

InvalidPackageType(String),
/// Bundle type symbol missing in binary
#[cfg_attr(
target_os = "linux",
error("__TAURI_BUNDLE_TYPE variable not found in binary. Make sure tauri crate and tauri-cli are up to date and that symbol stripping is disabled (https://doc.rust-lang.org/cargo/reference/profiles.html#strip)")
)]
#[cfg_attr(
not(target_os = "linux"),
error("__TAURI_BUNDLE_TYPE variable not found in binary. Make sure tauri crate and tauri-cli are up to date")
)]
#[error("__TAURI_BUNDLE_TYPE variable not found in binary. Make sure tauri crate and tauri-cli are up to date")]
MissingBundleTypeVar,
/// Failed to write binary file changed
#[error("Failed to write binary file changes: `{0}`")]
BinaryWriteError(String),
/// Invalid offset while patching binary file
#[error("Invalid offset while patching binary file")]
BinaryOffsetOutOfRange,
/// Unsupported architecture.
#[error("Architecture Error: `{0}`")]
ArchError(String),
Expand Down
12 changes: 6 additions & 6 deletions crates/tauri-utils/src/platform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -353,18 +353,18 @@ fn resource_dir_from<P: AsRef<std::path::Path>>(
#[cfg_attr(target_vendor = "apple", link_section = "__DATA,taubndl")]
// Marked as `mut` because it could get optimized away without it,
// see https://github.com/tauri-apps/tauri/pull/13812
static mut __TAURI_BUNDLE_TYPE: &str = "UNK";
static mut __TAURI_BUNDLE_TYPE: &str = "__TAURI_BUNDLE_TYPE_VAR_UNK";

/// Get the type of the bundle current binary is packaged in.
/// If the bundle type is unknown, it returns [`Option::None`].
pub fn bundle_type() -> Option<BundleType> {
unsafe {
match __TAURI_BUNDLE_TYPE {
"DEB" => Some(BundleType::Deb),
"RPM" => Some(BundleType::Rpm),
"APP" => Some(BundleType::AppImage),
"MSI" => Some(BundleType::Msi),
"NSS" => Some(BundleType::Nsis),
"__TAURI_BUNDLE_TYPE_VAR_DEB" => Some(BundleType::Deb),
"__TAURI_BUNDLE_TYPE_VAR_RPM" => Some(BundleType::Rpm),
"__TAURI_BUNDLE_TYPE_VAR_APP" => Some(BundleType::AppImage),
"__TAURI_BUNDLE_TYPE_VAR_MSI" => Some(BundleType::Msi),
"__TAURI_BUNDLE_TYPE_VAR_NSS" => Some(BundleType::Nsis),
_ => {
if cfg!(target_os = "macos") {
Some(BundleType::App)
Expand Down
Loading