From 7e18d85f89475f68d125f05b2968afb545346708 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 16:34:51 +0000 Subject: [PATCH 1/3] fix(auth): keep the token file's permissions from being inherited The refresh token is written to `tokens.tmp` and renamed into place. Both platforms apply the permissions we ask for when the file is created and only then: `CREATE_ALWAYS` over an existing file on Windows keeps that file's access-control list and ignores the descriptor passed to it, and `OpenOptions::mode` on unix applies to a create, not to an open. A temporary left behind by a crashed run -- or one another local account pre-created, which the working-directory config fallback makes reachable -- therefore lent its own permissions to the secret, and the rename carried them onto the store. Remove the temporary first, then create exclusively, so a race fails with an error instead of a readable token. Also in this crate: - Conditional access-control entries were dropped when a descriptor was read back, because the parser cut each entry at its first `)` and a conditional entry carries brackets of its own. A file shared with everyone through one read as private. Count brackets instead, and treat an entry that never closes as an entry rather than as nothing. - `libraries()` looked only at `HOME`, which Windows does not set, and gave up before trying the Program Files roots. It now asks the same way `discover()` does and keeps going without a home directory. - Bound the `TOKEN_USER` read by the buffer Windows filled. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01K3w2xdTR76kYV6RA52J4Ag --- crates/tapline-auth/src/local.rs | 57 +++++++++++------ crates/tapline-auth/src/sddl.rs | 85 ++++++++++++++++++++++++-- crates/tapline-auth/src/store.rs | 60 +++++++++++++++++- crates/tapline-auth/src/windows_acl.rs | 25 ++++++-- 4 files changed, 199 insertions(+), 28 deletions(-) diff --git a/crates/tapline-auth/src/local.rs b/crates/tapline-auth/src/local.rs index f79f5b3..d570f3e 100644 --- a/crates/tapline-auth/src/local.rs +++ b/crates/tapline-auth/src/local.rs @@ -30,11 +30,28 @@ pub fn home_relative_roots() -> &'static [&'static str] { } } -fn roots(home: &Path) -> Vec { - let mut roots: Vec = home_relative_roots() - .iter() - .map(|relative| home.join(relative)) - .collect(); +/// Where the account's home directory is. +/// +/// Windows does not set `HOME`; it sets `USERPROFILE`. Anything that looks for +/// a Steam install has to ask for both, which is what this is for -- looking +/// for only one of them is how `libraries()` used to come back empty on every +/// Windows machine. +fn home() -> Option { + std::env::var_os("HOME") + .or_else(|| std::env::var_os("USERPROFILE")) + .map(PathBuf::from) +} + +fn roots(home: Option<&Path>) -> Vec { + let mut roots: Vec = match home { + Some(home) => home_relative_roots() + .iter() + .map(|relative| home.join(relative)) + .collect(), + // A machine with no home directory named in the environment can still + // have Steam installed where the installer puts it. + None => Vec::new(), + }; for key in ["ProgramFiles(x86)", "ProgramFiles"] { if let Some(base) = std::env::var_os(key) { roots.push(PathBuf::from(base).join("Steam")); @@ -45,13 +62,7 @@ fn roots(home: &Path) -> Vec { #[must_use] pub fn discover() -> Vec { - let Some(home) = std::env::var_os("HOME") - .or_else(|| std::env::var_os("USERPROFILE")) - .map(PathBuf::from) - else { - return Vec::new(); - }; - discover_in(&roots(&home)) + discover_in(&roots(home().as_deref())) } #[must_use] @@ -127,11 +138,7 @@ fn sort_most_recent_first(accounts: &mut [LocalAccount]) { #[must_use] pub fn libraries() -> Vec { - let Some(home) = std::env::var_os("HOME").map(PathBuf::from) else { - return Vec::new(); - }; - - for root in roots(&home) { + for root in roots(home().as_deref()) { let path = root.join("config/libraryfolders.vdf"); let Ok(text) = std::fs::read_to_string(&path) else { continue; @@ -188,7 +195,7 @@ mod tests { #[test] fn a_home_yields_one_root_per_known_layout() { let home = std::path::Path::new("/home/someone"); - let roots = roots(home); + let roots = roots(Some(home)); assert!(roots.len() >= home_relative_roots().len()); assert!( roots @@ -196,6 +203,20 @@ mod tests { .all(|root| root.starts_with(home) || root.is_absolute()) ); } + + #[test] + fn no_home_still_leaves_the_install_directories_to_look_in() { + // `libraries()` used to give up here, which on Windows was every time: + // it asked only for HOME, which Windows does not set. + let roots = roots(None); + assert_eq!( + roots.len(), + ["ProgramFiles(x86)", "ProgramFiles"] + .iter() + .filter(|key| std::env::var_os(key).is_some()) + .count(), + ); + } use super::*; const TWO_ACCOUNTS: &str = r#" diff --git a/crates/tapline-auth/src/sddl.rs b/crates/tapline-auth/src/sddl.rs index 3447972..70a9301 100644 --- a/crates/tapline-auth/src/sddl.rs +++ b/crates/tapline-auth/src/sddl.rs @@ -65,11 +65,52 @@ fn dacl_body(descriptor: &str) -> Option<&str> { Some(body) } -fn aces(body: &str) -> impl Iterator { - body.split('(').skip(1).filter_map(|ace| { - let end = ace.find(')')?; - ace.get(..end) - }) +/// The access-control entries in a DACL body, each without its brackets. +/// +/// Splitting on `(` and cutting at the first `)` looks like it would do, and +/// does for an ordinary ACE, but a conditional one carries a bracketed +/// expression of its own: `(XA;;FA;;;WD;(Member_of {SID}))`. The naive split +/// left the grant itself unterminated, so it was dropped, and a file shared +/// with everyone through a conditional ACE read back as private. `XA` and `ZA` +/// are in `ALLOW_ACE_TYPES`, so those are entries this is meant to see. +/// Counting brackets keeps each ACE whole. +fn aces(body: &str) -> Vec<&str> { + let mut found = Vec::new(); + let mut depth: usize = 0; + let mut start: Option = None; + for (at, byte) in body.bytes().enumerate() { + match byte { + b'(' => { + if depth == 0 { + start = Some(at.saturating_add(1)); + } + depth = depth.saturating_add(1); + } + b')' => { + if depth == 0 { + continue; + } + depth -= 1; + if depth == 0 + && let Some(from) = start.take() + && let Some(ace) = body.get(from..at) + { + found.push(ace); + } + } + _ => {} + } + } + // A bracket that never closes is a descriptor we do not understand. Keeping + // what it opened means the grantee inside it is still read and can still be + // reported, rather than a malformed list quietly reading as private. + if depth > 0 + && let Some(from) = start + && let Some(ace) = body.get(from..) + { + found.push(ace); + } + found } #[cfg(test)] @@ -180,3 +221,37 @@ mod tests { assert_eq!(shared_with("D:P", owned_by(OWNER)), None); } } + +#[cfg(test)] +mod conditional_ace_tests { + use super::{aces, shared_with}; + + const OWNER: &str = "S-1-5-21-1111111111-2222222222-3333333333-1001"; + + fn owned_by(owner: &str) -> impl Fn(&str) -> bool + '_ { + move |holder| holder.eq_ignore_ascii_case(owner) + } + + #[test] + fn a_conditional_ace_keeps_its_own_brackets() { + let body = "P(A;;FA;;;S-1-5-21-1)(XA;;FA;;;WD;(Member_of {SID(BA)}))"; + assert_eq!( + aces(body), + vec!["A;;FA;;;S-1-5-21-1", "XA;;FA;;;WD;(Member_of {SID(BA)})"] + ); + } + + #[test] + fn a_conditional_grant_to_everyone_is_reported() { + // Before brackets were counted this came back `None`: the grant was cut + // in half, dropped, and the file read as the owner's alone. + let dacl = format!("D:P(A;;FA;;;{OWNER})(XA;;FA;;;WD;(Member_of {{SID(BA)}}))"); + assert_eq!(shared_with(&dacl, owned_by(OWNER)), Some("WD".to_owned())); + } + + #[test] + fn an_unterminated_ace_is_not_silently_dropped() { + let dacl = format!("D:P(A;;FA;;;{OWNER})(A;;FA;;;WD"); + assert_eq!(shared_with(&dacl, owned_by(OWNER)), Some("WD".to_owned())); + } +} diff --git a/crates/tapline-auth/src/store.rs b/crates/tapline-auth/src/store.rs index 1b130ce..ceb74f1 100644 --- a/crates/tapline-auth/src/store.rs +++ b/crates/tapline-auth/src/store.rs @@ -256,11 +256,31 @@ fn resolve_config_dir(configured: Option, home: Option) -> Pat configured.unwrap_or_else(|| home.unwrap_or_else(|| PathBuf::from(".")).join(".config")) } +/// Remove whatever is sitting at the temporary path before the token is +/// written there. +/// +/// Both `create_private` arms below ask the operating system to create a new +/// file with permissions of our choosing, and on both platforms those +/// permissions are applied at creation and only at creation. Writing into a +/// file that is already there -- a temporary a crashed run left behind, or one +/// another local account put there first -- would put the refresh token under +/// whatever permissions that file already carries. So the old one goes first, +/// and the create that follows refuses to open anything it did not make, which +/// turns a race into an error rather than into a readable secret. +fn clear_temporary(temporary: &Path) -> Result<(), TokenStoreError> { + match std::fs::remove_file(temporary) { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(TokenStoreError::Backend(e.to_string())), + } +} + #[cfg(windows)] fn create_private(path: &Path, contents: &str) -> Result<(), TokenStoreError> { use std::io::Write; let temporary = path.with_extension("tmp"); + clear_temporary(&temporary)?; let owner = crate::windows_acl::current_user_sid() .map_err(|e| TokenStoreError::Backend(e.to_string()))?; let mut file = @@ -281,8 +301,15 @@ fn create_private(path: &Path, contents: &str) -> Result<(), TokenStoreError> { use std::io::Write; let temporary = path.with_extension("tmp"); + clear_temporary(&temporary)?; let mut options = std::fs::OpenOptions::new(); - options.write(true).create(true).truncate(true); + // `create_new` rather than `create` + `truncate`: the mode below is applied + // when the file is created and not when an existing one is opened, and + // `open` follows symlinks, so opening whatever is already there could put + // the token in a world-readable file or somewhere else entirely. O_CREAT + // with O_EXCL, which is what `create_new` is, refuses a symlink at the last + // component as well, so this closes both. + options.write(true).create_new(true); #[cfg(unix)] { use std::os::unix::fs::OpenOptionsExt; @@ -457,6 +484,37 @@ mod tests { assert_eq!(names, vec!["one".to_owned(), "two".to_owned()]); } + #[cfg(unix)] + #[test] + fn a_temporary_left_behind_does_not_lend_the_token_its_permissions() { + use std::os::unix::fs::PermissionsExt; + + let scratch = Scratch::new("tokens-stale-temporary"); + let store = scratch.store(); + let TokenStore::File { path } = &store else { + unreachable!("the scratch store is a file store") + }; + + // A crashed run -- or another account that can write here -- leaves a + // readable file where the next save wants to put the token. The save + // used to open it as it was, keeping mode 0644, and then rename it over + // the real store, so the refresh token ended up world-readable. + let temporary = path.with_extension("tmp"); + std::fs::write(&temporary, "left behind").expect("stale temporary"); + std::fs::set_permissions(&temporary, std::fs::Permissions::from_mode(0o644)) + .expect("make it readable"); + + store.save(&token("someone")).expect("save"); + + let mode = std::fs::metadata(path) + .expect("metadata") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600, "the token file must be the owner's alone"); + assert!(store.load("someone").expect("load").is_some()); + } + #[test] fn forget_all_clears_the_store() { let scratch = Scratch::new("tokens-forget-all"); diff --git a/crates/tapline-auth/src/windows_acl.rs b/crates/tapline-auth/src/windows_acl.rs index b296888..ad44659 100644 --- a/crates/tapline-auth/src/windows_acl.rs +++ b/crates/tapline-auth/src/windows_acl.rs @@ -20,7 +20,7 @@ use windows_sys::Win32::Security::{ DACL_SECURITY_INFORMATION, EqualSid, GetTokenInformation, PSECURITY_DESCRIPTOR, PSID, SECURITY_ATTRIBUTES, TOKEN_QUERY, TOKEN_USER, TokenUser, }; -use windows_sys::Win32::Storage::FileSystem::{CREATE_ALWAYS, CreateFileW, FILE_ATTRIBUTE_NORMAL}; +use windows_sys::Win32::Storage::FileSystem::{CREATE_NEW, CreateFileW, FILE_ATTRIBUTE_NORMAL}; use windows_sys::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; const ATTRIBUTES_SIZE: u32 = size_of::() as u32; @@ -62,6 +62,15 @@ pub fn current_user_sid() -> io::Result { // the allocation happened to land unaligned, so copy the header out first. // The SID it points at stays where it is, inside `buffer`, which outlives // the call below. + if buffer.len() < size_of::() { + // Windows says how much room it wants before it writes, so this cannot + // happen; reading a short buffer as a TOKEN_USER would be out of + // bounds, which is worth one comparison to rule out rather than trust. + return Err(io::Error::new( + io::ErrorKind::InvalidData, + "the token's user information was shorter than a TOKEN_USER", + )); + } let mut text: *mut u16 = ptr::null_mut(); let converted = unsafe { let user = ptr::read_unaligned(buffer.as_ptr().cast::()); @@ -73,12 +82,20 @@ pub fn current_user_sid() -> io::Result { Ok(unsafe { take_wide(text) }) } -/// Creates (or replaces) a file carrying `dacl`, an SDDL access-control list, -/// from the moment it exists. +/// Creates a file carrying `dacl`, an SDDL access-control list, from the moment +/// it exists. /// /// Setting the list afterwards would leave a window in which the file is /// readable by whoever the parent directory says, so it is passed to the /// creation call instead. +/// +/// The disposition is `CREATE_NEW`, and that is load-bearing: Windows applies +/// `lpSecurityAttributes` only when the call actually creates the file. Asking +/// it to replace an existing one instead truncates that file and keeps the list +/// it already had, so a temporary left behind by a crashed run -- or one +/// another account pre-created in a writable directory -- would have taken the +/// secret under its own permissions. Failing with `AlreadyExists` puts that +/// decision on the caller, which removes the previous file first. pub fn create_with_dacl(path: &Path, dacl: &str) -> io::Result { let mut descriptor: PSECURITY_DESCRIPTOR = ptr::null_mut(); let wide_dacl = wide(OsStr::new(dacl)); @@ -106,7 +123,7 @@ pub fn create_with_dacl(path: &Path, dacl: &str) -> io::Result { GENERIC_WRITE, 0, &attributes, - CREATE_ALWAYS, + CREATE_NEW, FILE_ATTRIBUTE_NORMAL, ptr::null_mut(), ) From e78e20e1f92701f77e254ca8a1b1b64cefb3e400 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 16:34:51 +0000 Subject: [PATCH 2/3] fix(ffi): stop losing events and fix the concurrency latch Three things across the C surface: - `tapline_job_next` took the held message, read the queue and put the message back in three separate critical sections, so two threads polling one job could both find the slot empty, both take a message, and both write the slot -- dropping one. The header promises every function is safe to call from any thread, so hold one lock for the whole call. - `tapline_set_total_concurrency` latched on a flag set when a job is spawned, but the budget is frozen the first time the pool is built, which asking for the current concurrency already does. Calling `tapline_total_concurrency()` first therefore made every later `set_total_concurrency` answer OK and change nothing. Latch where the budget is actually frozen, and check before storing so a rejected call leaves the value alone. - Asking `tapline_last_error` for the length with a null buffer answered TAPLINE_BUFFER_TOO_SMALL when there was no error to report. Out-pointer writes now go through `write_unaligned`, which costs nothing and removes an alignment precondition C has no way to promise us, and the safety comments say what is actually checked. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01K3w2xdTR76kYV6RA52J4Ag --- crates/tapline-ffi/src/lib.rs | 60 ++++++++++++++++++++++++++++------- 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/crates/tapline-ffi/src/lib.rs b/crates/tapline-ffi/src/lib.rs index af3e040..8209704 100644 --- a/crates/tapline-ffi/src/lib.rs +++ b/crates/tapline-ffi/src/lib.rs @@ -61,6 +61,12 @@ fn shared() -> Option<&'static std::sync::Arc> { fn pool() -> Option<&'static std::sync::Arc> { static POOL: OnceLock> = OnceLock::new(); Some(POOL.get_or_init(|| { + // This is the moment the budget stops being changeable, so it is where + // the latch belongs. Setting it in `spawn_job` instead meant that + // asking for the current budget -- which builds the pool -- left the + // latch down, and the next `tapline_set_total_concurrency` answered + // TAPLINE_OK while the pool went on running at the old size. + STARTED.store(true, std::sync::atomic::Ordering::Relaxed); let configured = TOTAL_CONCURRENCY.load(std::sync::atomic::Ordering::Relaxed) as usize; let budget = if configured == 0 { Shared::new(InstallOptions::default().concurrency) @@ -73,11 +79,13 @@ fn pool() -> Option<&'static std::sync::Arc> { #[unsafe(no_mangle)] pub extern "C" fn tapline_set_total_concurrency(chunks: u32) -> i32 { - TOTAL_CONCURRENCY.store(chunks, std::sync::atomic::Ordering::Relaxed); + // Checked before the store, so a rejected call leaves the value alone + // rather than recording a size that will never be used. if STARTED.load(std::sync::atomic::Ordering::Relaxed) { set_error("the concurrency budget is already in use and cannot be resized"); return TAPLINE_BAD_ARGUMENT; } + TOTAL_CONCURRENCY.store(chunks, std::sync::atomic::Ordering::Relaxed); TAPLINE_OK } @@ -174,8 +182,6 @@ where return TAPLINE_BAD_ARGUMENT; }; - STARTED.store(true, std::sync::atomic::Ordering::Relaxed); - let (sender, receiver) = tokio::sync::mpsc::unbounded_channel(); let handle = runtime.spawn(build(sender)); @@ -184,7 +190,11 @@ where held: std::sync::Mutex::new(None), handle, }); - unsafe { *out = Box::into_raw(job) }; + // SAFETY: `out` was null-checked above and the header promises it is + // writable for one pointer. `write_unaligned` so a caller whose out-slot is + // inside a packed struct is not undefined behaviour; C has no way to + // promise us the alignment and we have no way to check it. + unsafe { out.write_unaligned(Box::into_raw(job)) }; TAPLINE_OK } @@ -777,8 +787,17 @@ pub unsafe extern "C" fn tapline_job_next( return TAPLINE_BAD_ARGUMENT; }; - let held = job.held.lock().ok().and_then(|mut slot| slot.take()); - let message = match held { + // One lock for the whole function. Taking the held slot, reading the queue + // and putting the message back used to be three separate critical + // sections, so two threads calling this on one job could both find the slot + // empty, both take a message, and both write the slot -- losing one of + // them. The header says every function here is safe to call from any + // thread, so this has to serialise rather than interleave. + let Ok(mut slot) = job.held.lock() else { + set_error("the job's event queue was poisoned"); + return TAPLINE_BAD_ARGUMENT; + }; + let message = match slot.take() { Some(message) => Some(message), None => { let Ok(mut receiver) = job.events.lock() else { @@ -808,15 +827,23 @@ pub unsafe extern "C" fn tapline_job_next( }; if !out_len.is_null() { - unsafe { *out_len = message.len() }; + // SAFETY: null-checked, and the header promises a writable `size_t`. + // Unaligned for the same reason as `spawn_job`'s out-pointer. + unsafe { out_len.write_unaligned(message.len()) }; } if buf.is_null() || cap < message.len() { - if let Ok(mut slot) = job.held.lock() { - *slot = Some(message); - } + // The message is kept rather than dropped, so a caller that asked for + // the length first can ask again with a big enough buffer. + *slot = Some(message); return TAPLINE_BUFFER_TOO_SMALL; } + if message.is_empty() { + return TAPLINE_OK; + } + // SAFETY: `buf` is non-null and `cap` is at least `message.len()`, both + // checked immediately above, and the header promises `buf` is writable for + // `cap` bytes. A `String`'s bytes cannot overlap a buffer the caller owns. unsafe { std::ptr::copy_nonoverlapping(message.as_ptr(), buf, message.len()) }; TAPLINE_OK } @@ -849,11 +876,20 @@ pub unsafe extern "C" fn tapline_last_error(buf: *mut u8, cap: usize, out_len: * LAST_ERROR.with(|slot| { let message = slot.borrow(); if !out_len.is_null() { - unsafe { *out_len = message.len() }; + // SAFETY: null-checked; the header promises a writable `size_t`. + unsafe { out_len.write_unaligned(message.len()) }; } - if buf.is_null() || cap < message.len() { + if cap < message.len() || (buf.is_null() && !message.is_empty()) { return TAPLINE_BUFFER_TOO_SMALL; } + if message.is_empty() { + // Nothing to copy, and no error set is the ordinary case -- asking + // for the length with a null buffer used to answer + // TAPLINE_BUFFER_TOO_SMALL for the zero bytes it needed. + return TAPLINE_OK; + } + // SAFETY: `buf` is non-null and writable for `cap` bytes per the + // header, and `cap` is at least `message.len()`, checked above. unsafe { std::ptr::copy_nonoverlapping(message.as_ptr(), buf, message.len()) }; TAPLINE_OK }) From b1949ade2095650758259c266983ec0268a214ce Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 20 Sep 2026 16:34:51 +0000 Subject: [PATCH 3/3] fix(steamworks): say what setting SteamAppId actually requires `Steam::connect` set `SteamAppId` with a comment claiming the process was still single-threaded. A library linked into someone else's game-server host cannot know that, and on glibc `setenv` can reallocate `environ` under a concurrent `getenv`. There is no safe way to set an environment variable in Rust and no way to hand the app id to the Steam client except through one, so the call stays -- but it is now skipped whenever the host has already set the variable, and `connect` documents the requirement rather than asserting something it cannot know. Two more in the same function: the error buffer was `[i8; 1024]`, which does not compile where `c_char` is unsigned, and it was read with `CStr::from_ptr`, which would scan past the end if Steam ever filled it without a terminator. Both are now bounded and target-independent, in safe Rust. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01K3w2xdTR76kYV6RA52J4Ag --- crates/tapline-steamworks/src/lib.rs | 75 ++++++++++++++++++++++++---- 1 file changed, 64 insertions(+), 11 deletions(-) diff --git a/crates/tapline-steamworks/src/lib.rs b/crates/tapline-steamworks/src/lib.rs index 7f0d2d5..679a863 100644 --- a/crates/tapline-steamworks/src/lib.rs +++ b/crates/tapline-steamworks/src/lib.rs @@ -1,4 +1,4 @@ -use std::ffi::{CStr, c_char, c_int, c_void}; +use std::ffi::{c_char, c_int, c_void}; use std::path::{Path, PathBuf}; use std::time::{Duration, Instant}; use tapline_ids::{AppId, PublishedFileId}; @@ -169,7 +169,63 @@ fn hunt(root: &Path, depth: usize) -> Option { None } +/// Make sure the Steam client can see which app this is. +/// +/// `SteamAPI_InitFlat` reads `SteamAppId` from the process environment, and +/// there is no other way to tell it. Skipping the call whenever the variable is +/// already set means a host that set it up front never reaches the `unsafe` +/// below. +fn announce_app_id(app: AppId) { + static ONCE: std::sync::Once = std::sync::Once::new(); + ONCE.call_once(|| { + for name in ["SteamAppId", "SteamGameId"] { + if std::env::var_os(name).is_some_and(|value| !value.is_empty()) { + continue; + } + // SAFETY: `set_var` is unsound while another thread may be reading + // the environment -- glibc can reallocate `environ` underneath a + // concurrent `getenv` -- and this crate, being a library, cannot + // prove on its own that nothing else is running. That is why + // `Steam::connect` documents the requirement that it is called + // before the host starts other threads, and why this is skipped + // entirely when the host has already set the variable. There is no + // safe way to set an environment variable in Rust 1.94 and no way + // to hand the value to the Steam client except through it. + unsafe { std::env::set_var(name, app.get().to_string()) }; + } + }); +} + +/// Whatever Steam wrote into an error buffer, read as text. +/// +/// Bounded by the buffer rather than by a terminator: `CStr::from_ptr` would +/// scan past the end if Steam ever filled all of it without writing a NUL. No +/// `unsafe` is needed for any of it -- a `c_char` is one byte, whichever sign +/// the target gives it. +fn message_in(buffer: &[c_char]) -> String { + let end = buffer + .iter() + .position(|byte| *byte == 0) + .unwrap_or(buffer.len()); + let bytes: Vec = buffer + .get(..end) + .unwrap_or_default() + .iter() + .map(|byte| *byte as u8) + .collect(); + String::from_utf8_lossy(&bytes).trim().to_owned() +} + impl Steam { + /// Connect to a running Steam client. + /// + /// Call this before the host starts any other thread. The Steam client + /// reads `SteamAppId` out of the process environment while it initialises, + /// so if it is not already set this has to set it, and setting an + /// environment variable is only sound while nothing else can be reading + /// one. If the host cannot promise that, it should put `SteamAppId` and + /// `SteamGameId` in the environment itself before starting threads, and + /// this will find them there and leave them alone. pub fn connect(app: AppId) -> Result { let roots = steam_roots(); let path = find_library(&roots).ok_or_else(|| { @@ -182,11 +238,7 @@ impl Steam { ) })?; - // SAFETY: still single-threaded; no other thread observes the environment. - unsafe { - std::env::set_var("SteamAppId", app.get().to_string()); - std::env::set_var("SteamGameId", app.get().to_string()); - } + announce_app_id(app); // SAFETY: every signature below is the documented flat-API one. unsafe { @@ -212,13 +264,14 @@ impl Steam { let init_flat = std::mem::transmute:: c_int>( sym(b"SteamAPI_InitFlat\0")?, ); - let mut err = [0_i8; 1024]; + // `c_char` is `i8` on x86-64 and on Apple silicon but `u8` on + // aarch64 Linux, so the element type has to follow it rather than + // be spelled out, or this crate stops compiling on a Steam-capable + // ARM server. + let mut err = [0 as c_char; 1024]; let rc = init_flat(err.as_mut_ptr()); if rc != 0 { - let why = CStr::from_ptr(err.as_ptr()) - .to_string_lossy() - .trim() - .to_owned(); + let why = message_in(&err); return Err(SteamError::InitFailed(if why.is_empty() { format!("code {rc}") } else {