Skip to content

Commit 51eddb6

Browse files
lukekimlukekim
committed
Fix all PR review comments from Copilot and CodeQL
- Mask SMB username in startup log (CodeQL cleartext sensitive info) - Restore NtStatus::Unknown(u32) variant for unknown SMB status codes - Fix oplock level to 0x00 (SMB2_OPLOCK_LEVEL_NONE) - Fix decode_read_response to read DataOffset as u16 not u8 - Mask NetBIOS length to 24 bits in frame_packet and send_compound - Add bounds validation in parse_compound_response - Restore Option return on RangeSpec::resolve, guard total==0 - Validate multipart parts before removing upload state - Re-read SMB metadata after put_object for consistent ETag/mtime - Remove duplicate mod/macro declarations from main.rs, use lib crate - Restore sccache warm-build assertions and zero-stats reset - Check Close status in compound create_close
1 parent 2b26d57 commit 51eddb6

7 files changed

Lines changed: 150 additions & 79 deletions

File tree

‎scripts/test-sccache.sh‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,6 +266,8 @@ echo "[test] === cold build (populating cache) ==="
266266
rm -rf "$TEST_TARGET_DIR"
267267
CARGO_TARGET_DIR="$TEST_TARGET_DIR" cargo build 2>&1
268268

269+
sccache --zero-stats 2>/dev/null || true
270+
269271
echo ""
270272
echo "[test] === warm build (should hit cache) ==="
271273
rm -rf "$TEST_TARGET_DIR"
@@ -277,3 +279,17 @@ echo "[test] sccache stats:"
277279
echo "======================================="
278280
sccache --show-stats
279281
echo "======================================="
282+
283+
# ── Verify cache hits ───────────────────────────────────────────────
284+
285+
STATS=$(sccache --show-stats 2>&1)
286+
CACHE_HITS=$(echo "$STATS" | grep -m1 "^Cache hits" | awk '{print $NF}' || echo "0")
287+
WRITE_ERRORS=$(echo "$STATS" | grep -m1 "Cache write errors" | awk '{print $NF}' || echo "0")
288+
289+
echo ""
290+
if [[ "${CACHE_HITS:-0}" -gt 0 && "${WRITE_ERRORS:-0}" -eq 0 ]]; then
291+
echo "[test] PASS: warm build got $CACHE_HITS cache hits, 0 write errors"
292+
else
293+
echo "[test] FAIL: expected cache hits > 0 (got ${CACHE_HITS:-0}) and write errors == 0 (got ${WRITE_ERRORS:-0})"
294+
exit 1
295+
fi

‎src/main.rs‎

Lines changed: 13 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,5 @@
11
//! spiceio — S3-compatible API proxy to SMB 3.1.1 file shares (macOS 26).
22
3-
mod crypto;
4-
mod log;
5-
mod s3;
6-
mod smb;
7-
83
use hyper::Request;
94
use hyper::body::Incoming;
105
use hyper_util::rt::TokioIo;
@@ -16,27 +11,17 @@ use std::sync::Arc;
1611
use tokio::net::TcpListener;
1712
use tokio::signal;
1813

14+
use spiceio::log;
15+
use spiceio::s3;
16+
use spiceio::serr;
17+
use spiceio::slog;
18+
use spiceio::smb;
19+
1920
use s3::multipart::MultipartStore;
2021
use s3::router::AppState;
2122
use smb::client::SmbConfig;
2223
use smb::ops::ShareSession;
2324

24-
/// Log to stdout and optionally to a file (non-blocking).
25-
#[macro_export]
26-
macro_rules! slog {
27-
($($arg:tt)*) => {
28-
$crate::log::emit(format_args!($($arg)*))
29-
};
30-
}
31-
32-
/// Log to stderr and optionally to a file (non-blocking).
33-
#[macro_export]
34-
macro_rules! serr {
35-
($($arg:tt)*) => {
36-
$crate::log::emit_err(format_args!($($arg)*))
37-
};
38-
}
39-
4025
/// Runtime configuration parsed from environment variables.
4126
struct Config {
4227
/// Address to bind the HTTP server to
@@ -94,9 +79,15 @@ async fn main() {
9479

9580
let config = Config::from_env();
9681

82+
let masked_user = if config.smb_username.is_empty() {
83+
"***".to_string()
84+
} else {
85+
let first = &config.smb_username[..config.smb_username.chars().next().unwrap().len_utf8()];
86+
format!("{first}***")
87+
};
9788
slog!(
9889
"[spiceio] connecting to smb://{}@{}:{}/{}",
99-
config.smb_username,
90+
masked_user,
10091
config.smb_server,
10192
config.smb_port,
10293
config.smb_share

‎src/s3/headers.rs‎

Lines changed: 42 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -60,12 +60,26 @@ pub fn parse_range(header: &str) -> Option<RangeSpec> {
6060

6161
impl RangeSpec {
6262
/// Resolve to absolute byte positions given total file size.
63-
pub fn resolve(&self, total: u64) -> (u64, u64) {
63+
/// Returns `None` if `total == 0` or `start >= total`.
64+
pub fn resolve(&self, total: u64) -> Option<(u64, u64)> {
65+
if total == 0 {
66+
return None;
67+
}
6468
match (self.start, self.end) {
65-
(Some(s), Some(e)) => (s, e.min(total - 1)),
66-
(Some(s), None) => (s, total - 1),
67-
(None, Some(suffix)) => (total.saturating_sub(suffix), total - 1),
68-
(None, None) => (0, total - 1),
69+
(Some(s), Some(e)) => {
70+
if s >= total {
71+
return None;
72+
}
73+
Some((s, e.min(total - 1)))
74+
}
75+
(Some(s), None) => {
76+
if s >= total {
77+
return None;
78+
}
79+
Some((s, total - 1))
80+
}
81+
(None, Some(suffix)) => Some((total.saturating_sub(suffix), total - 1)),
82+
(None, None) => Some((0, total - 1)),
6983
}
7084
}
7185
}
@@ -199,7 +213,7 @@ mod tests {
199213
start: Some(0),
200214
end: Some(99),
201215
};
202-
assert_eq!(r.resolve(100), (0, 99));
216+
assert_eq!(r.resolve(100), Some((0, 99)));
203217
}
204218

205219
#[test]
@@ -208,7 +222,7 @@ mod tests {
208222
start: Some(0),
209223
end: Some(999),
210224
};
211-
assert_eq!(r.resolve(100), (0, 99));
225+
assert_eq!(r.resolve(100), Some((0, 99)));
212226
}
213227

214228
#[test]
@@ -217,7 +231,7 @@ mod tests {
217231
start: Some(50),
218232
end: None,
219233
};
220-
assert_eq!(r.resolve(100), (50, 99));
234+
assert_eq!(r.resolve(100), Some((50, 99)));
221235
}
222236

223237
#[test]
@@ -226,7 +240,7 @@ mod tests {
226240
start: None,
227241
end: Some(10),
228242
};
229-
assert_eq!(r.resolve(100), (90, 99));
243+
assert_eq!(r.resolve(100), Some((90, 99)));
230244
}
231245

232246
#[test]
@@ -235,7 +249,25 @@ mod tests {
235249
start: None,
236250
end: Some(200),
237251
};
238-
assert_eq!(r.resolve(100), (0, 99));
252+
assert_eq!(r.resolve(100), Some((0, 99)));
253+
}
254+
255+
#[test]
256+
fn resolve_zero_total() {
257+
let r = RangeSpec {
258+
start: Some(0),
259+
end: Some(99),
260+
};
261+
assert_eq!(r.resolve(0), None);
262+
}
263+
264+
#[test]
265+
fn resolve_start_past_end() {
266+
let r = RangeSpec {
267+
start: Some(200),
268+
end: None,
269+
};
270+
assert_eq!(r.resolve(100), None);
239271
}
240272

241273
// ── parse_http_date ──────────────────────────────────────────────

‎src/s3/router.rs‎

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -571,8 +571,17 @@ async fn handle_get_object(
571571
// Determine read range
572572
let (start, end, is_range) = if let Some(ref range_str) = range_header {
573573
if let Some(range) = parse_range(range_str) {
574-
let (s, e) = range.resolve(file_size);
575-
(s, e, true)
574+
match range.resolve(file_size) {
575+
Some((s, e)) => (s, e, true),
576+
None => {
577+
let _ = handle.close().await;
578+
return error_response(
579+
StatusCode::RANGE_NOT_SATISFIABLE,
580+
"InvalidRange",
581+
"The requested range is not satisfiable",
582+
);
583+
}
584+
}
576585
} else {
577586
(0, file_size.saturating_sub(1), false)
578587
}
@@ -1042,7 +1051,7 @@ async fn handle_complete_multipart_upload(
10421051
key: &str,
10431052
upload_id: &str,
10441053
) -> Response<SpiceioBody> {
1045-
let upload = match state.multipart.complete(upload_id).await {
1054+
let upload = match state.multipart.get(upload_id).await {
10461055
Some(u) => u,
10471056
None => {
10481057
return error_response(
@@ -1062,14 +1071,24 @@ async fn handle_complete_multipart_upload(
10621071
})
10631072
.collect();
10641073

1074+
// Validate all requested parts exist before assembly
1075+
for pn in &part_numbers {
1076+
if !upload.parts.contains_key(pn) {
1077+
return error_response(
1078+
StatusCode::BAD_REQUEST,
1079+
"InvalidPart",
1080+
&format!("Part {pn} not found"),
1081+
);
1082+
}
1083+
}
1084+
10651085
// Concatenate parts in order and write the final object
10661086
let mut final_data = Vec::new();
10671087
for pn in &part_numbers {
10681088
if let Some(part) = upload.parts.get(pn) {
10691089
match state.share.read_temp(&part.temp_path).await {
10701090
Ok(data) => final_data.extend_from_slice(&data),
10711091
Err(e) => {
1072-
// Re-register the upload since we consumed it
10731092
return io_to_s3_error(&e);
10741093
}
10751094
}
@@ -1082,9 +1101,14 @@ async fn handle_complete_multipart_upload(
10821101
Err(e) => return io_to_s3_error(&e),
10831102
};
10841103

1104+
// Only now remove the upload from the store
1105+
let upload = state.multipart.complete(upload_id).await;
1106+
10851107
// Clean up temp files (best effort)
1086-
for part in upload.parts.values() {
1087-
state.share.delete_temp(&part.temp_path).await;
1108+
if let Some(upload) = upload {
1109+
for part in upload.parts.values() {
1110+
state.share.delete_temp(&part.temp_path).await;
1111+
}
10881112
}
10891113
let marker_path = format!("{}\\marker", MultipartStore::temp_dir(upload_id));
10901114
state.share.delete_temp(&marker_path).await;

‎src/smb/client.rs‎

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -518,7 +518,7 @@ impl SmbClient {
518518

519519
let total: usize = sizes.iter().sum();
520520
let mut buf = BytesMut::with_capacity(4 + total);
521-
buf.put_u32(total as u32); // NetBIOS length (big-endian)
521+
buf.put_u32((total as u32) & 0x00FF_FFFF); // NetBIOS length (big-endian, masked to 24 bits)
522522

523523
for (i, (mut header, body)) in requests.into_iter().enumerate() {
524524
let body_len = body.len();
@@ -620,6 +620,12 @@ impl SmbClient {
620620
}
621621
let cr = decode_create_response(&resp[0].1)
622622
.ok_or_else(|| io::Error::new(io::ErrorKind::InvalidData, "invalid create response"))?;
623+
if NtStatus::from_u32(resp[1].0.status).is_error() {
624+
crate::serr!(
625+
"[spiceio] smb compound close failed: 0x{:08X}",
626+
resp[1].0.status
627+
);
628+
}
623629
let cl = decode_close_response(&resp[1].1).unwrap_or(CloseResponse {
624630
last_write_time: cr.last_write_time,
625631
file_size: cr.file_size,
@@ -899,7 +905,18 @@ fn parse_compound_response(msg: &[u8]) -> Vec<(Header, Vec<u8>)> {
899905

900906
let next = header.next_command as usize;
901907
let body_start = offset + SMB2_HEADER_SIZE;
902-
let body_end = if next > 0 { offset + next } else { msg.len() };
908+
let body_end = if next > 0 {
909+
let end = offset + next;
910+
if end > msg.len() || end < body_start {
911+
break;
912+
}
913+
end
914+
} else {
915+
msg.len()
916+
};
917+
if body_start > body_end || body_end > msg.len() {
918+
break;
919+
}
903920

904921
let body = msg[body_start..body_end].to_vec();
905922
results.push((header, body));

‎src/smb/ops.rs‎

Lines changed: 3 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -302,13 +302,11 @@ impl ShareSession {
302302

303303
let _ = self.client.close(self.tree_id, &file.file_id).await;
304304

305+
let meta = self.head_object(key).await?;
305306
Ok(ObjectMeta {
306307
size: data.len() as u64,
307-
last_modified: std::time::SystemTime::now()
308-
.duration_since(std::time::UNIX_EPOCH)
309-
.unwrap_or_default()
310-
.as_secs(),
311-
etag: format!("{:016x}", simple_hash(data)),
308+
last_modified: meta.last_modified,
309+
etag: meta.etag,
312310
content_type: guess_content_type(key),
313311
})
314312
}
@@ -609,17 +607,6 @@ fn guess_content_type(key: &str) -> String {
609607
.into()
610608
}
611609

612-
/// Simple non-cryptographic hash for ETags.
613-
fn simple_hash(data: &[u8]) -> u64 {
614-
// FNV-1a
615-
let mut hash: u64 = 0xcbf29ce484222325;
616-
for &byte in data {
617-
hash ^= byte as u64;
618-
hash = hash.wrapping_mul(0x100000001b3);
619-
}
620-
hash
621-
}
622-
623610
#[cfg(test)]
624611
mod tests {
625612
use super::*;

0 commit comments

Comments
 (0)