fix(ec): validate ShardId at gRPC boundary, reject >=32 (#11346)

This commit is contained in:
Eliah Rusin authored and GitHub committed 2026-09-16 08:33:22 -07:00
1 parent 701e397337
commit def25ca84d
8 files changed
+260 -60

No files matched your search

@@ -16,6 +16,20 @@ pub const ERASURE_CODING_SMALL_BLOCK_SIZE: usize = 1024 * 1024; // 1MB
pub type ShardId = u8;
/// Validate a wire shard id. `ShardId` is `u8` but only 0..MAX_SHARD_COUNT are valid.
/// Rejects 256 (would truncate to 0 and delete .ec00) and 270 (would alias 14).
pub fn shard_id_try_from(v: u32) -> Result<ShardId, String> {
if v < MAX_SHARD_COUNT as u32 {
Ok(v as ShardId)
} else {
Err(format!(
"invalid shard id {} (max {})",
v,
MAX_SHARD_COUNT - 1
))
}
}
/// A single erasure-coded shard file.
pub struct EcVolumeShard {
pub volume_id: VolumeId,
@@ -251,4 +265,42 @@ mod tests {
let shard = EcVolumeShard::new("/data", "", VolumeId(7), 13);
assert_eq!(shard.file_name(), "/data/7.ec13");
}
#[test]
fn test_shard_id_try_from_u32_rejects_overflow() {
use super::{MAX_SHARD_COUNT, shard_id_try_from};
assert_eq!(shard_id_try_from(0).unwrap(), 0u8);
assert_eq!(shard_id_try_from(14).unwrap(), 14u8);
assert_eq!(shard_id_try_from(31).unwrap(), 31u8);
assert!(shard_id_try_from(32).is_err());
assert!(shard_id_try_from(256).is_err());
assert!(shard_id_try_from(270).is_err());
assert!(shard_id_try_from(u32::MAX).is_err());
assert_eq!(MAX_SHARD_COUNT, 32);
}
#[test]
fn test_shard_batch_validation_is_atomic_rejects_without_partial_prefix() {
use super::shard_id_try_from;
// The mount/unmount handlers pre-validate the ENTIRE req.shard_ids into
// a Vec<ShardId> BEFORE acquiring the write lock or mutating any EC
// state. This test pins the validation half of that contract at the
// unit level: a batch like [0, 32] must fail as a whole, so by
// construction no validated prefix (e.g. shard 0) is ever applied.
// The handler-level tests below assert the no-state-change half.
let batch = vec![0u32, 32u32];
let validated: Result<Vec<_>, _> =
batch.iter().map(|&sid| shard_id_try_from(sid)).collect();
assert!(
validated.is_err(),
"batch {:?} must be rejected as a whole",
batch
);
// A fully-valid batch still validates cleanly.
let ok: Result<Vec<_>, _> = [0u32, 1u32, 13u32]
.iter()
.map(|&sid| shard_id_try_from(sid))
.collect();
assert_eq!(ok.unwrap(), vec![0u8, 1u8, 13u8]);
}
}
@@ -841,11 +841,23 @@ impl EcVolume {
}
/// Remove and close a shard.
pub fn remove_shard(&mut self, shard_id: ShardId) {
if let Some(ref mut shard) = self.shards[shard_id as usize] {
pub fn remove_shard(&mut self, shard_id: ShardId) -> io::Result<()> {
let idx = shard_id as usize;
if idx >= self.shards.len() {
return Err(io::Error::new(
io::ErrorKind::InvalidInput,
format!(
"invalid shard id {} (max {})",
shard_id,
self.shards.len().saturating_sub(1)
),
));
}
if let Some(ref mut shard) = self.shards[idx] {
shard.close();
}
self.shards[shard_id as usize] = None;
self.shards[idx] = None;
Ok(())
}
/// Get a ShardBits bitmap of locally available shards.
@@ -867,7 +879,7 @@ impl EcVolume {
/// Reports whether `shard_id` is currently registered to this
/// EcVolume (used by the cross-disk reconcile to skip already-
/// loaded shards).
pub fn has_shard(&self, shard_id: u8) -> bool {
pub fn has_shard(&self, shard_id: ShardId) -> bool {
self.shards
.get(shard_id as usize)
.map(|s| s.is_some())