fix(agent): fail to open a store whose /meta has an unreadable attribute
Group::attrs now leaves out an attribute it cannot decode. The agent read its settings through it, so a store whose float16 (or compression, quantized_index, WAL mark, signature...) attribute could not be decoded opened with the default in its place, and no error. /meta is now read with attrs_with_errors and any unreadable attribute is a Schema error, as it was before attrs became tolerant. Found by the adversarial review. Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
This commit is contained in:
@@ -477,10 +477,34 @@ fn write_string_dataset(
|
||||
}
|
||||
}
|
||||
|
||||
/// `/meta`'s attributes, failing if any of them cannot be read.
|
||||
///
|
||||
/// `Group::attrs` leaves out an attribute it cannot decode. For the store's
|
||||
/// settings that would silently fall back to defaults (e.g. `float16`, the
|
||||
/// WAL mark), so an unreadable attribute is an error here, as it was before
|
||||
/// `attrs` became tolerant.
|
||||
fn meta_attrs(
|
||||
file: &clawhdf5::File,
|
||||
) -> Result<std::collections::HashMap<String, AttrValue>, MemoryError> {
|
||||
let meta = file
|
||||
.group("meta")
|
||||
.map_err(|e| MemoryError::Schema(format!("missing /meta group: {e}")))?;
|
||||
let (attrs, errors) = meta
|
||||
.attrs_with_errors()
|
||||
.map_err(|e| MemoryError::Schema(format!("cannot read /meta attrs: {e}")))?;
|
||||
if let Some(e) = errors.first() {
|
||||
return Err(MemoryError::Schema(format!(
|
||||
"cannot read /meta attrs: {} unreadable, first: {e}",
|
||||
errors.len()
|
||||
)));
|
||||
}
|
||||
Ok(attrs)
|
||||
}
|
||||
|
||||
/// Validate an HDF5 file has the correct schema and load all data.
|
||||
/// Read the checkpoint's [`WalMark`] from `/meta`, if it has one.
|
||||
pub fn read_wal_mark(file: &clawhdf5::File) -> Option<WalMark> {
|
||||
let attrs = file.group("meta").ok()?.attrs().ok()?;
|
||||
let attrs = meta_attrs(file).ok()?;
|
||||
let len = match attrs.get(WAL_APPLIED_LEN_ATTR)? {
|
||||
AttrValue::I64(v) => u64::try_from(*v).ok()?,
|
||||
_ => return None,
|
||||
@@ -498,10 +522,7 @@ pub fn read_signature(
|
||||
file: &clawhdf5::File,
|
||||
) -> Result<Option<crate::signing::StoredSignature>, MemoryError> {
|
||||
use crate::signing::{Manifest, StoredSignature, from_hex};
|
||||
let attrs = file
|
||||
.group("meta")
|
||||
.and_then(|g| g.attrs())
|
||||
.map_err(|e| MemoryError::Schema(format!("cannot read /meta attrs: {e}")))?;
|
||||
let attrs = meta_attrs(file)?;
|
||||
let version = match attrs.get(SIG_VERSION_ATTR) {
|
||||
None => return Ok(None),
|
||||
Some(AttrValue::I64(v)) => *v,
|
||||
@@ -552,18 +573,14 @@ pub fn read_signature(
|
||||
|
||||
/// Read the checkpoint bookkeeping from `/meta`.
|
||||
pub fn read_checkpoint_meta(file: &clawhdf5::File) -> CheckpointMeta {
|
||||
let ann_generation = file
|
||||
.group("meta")
|
||||
.ok()
|
||||
.and_then(|g| g.attrs().ok())
|
||||
.and_then(|attrs| match attrs.get(ANN_GENERATION_ATTR) {
|
||||
Some(AttrValue::I64(v)) => Some(*v as u64),
|
||||
_ => None,
|
||||
});
|
||||
let signed = file
|
||||
.group("meta")
|
||||
.and_then(|g| g.attrs())
|
||||
.is_ok_and(|attrs| attrs.contains_key(SIG_VERSION_ATTR));
|
||||
let ann_generation =
|
||||
meta_attrs(file)
|
||||
.ok()
|
||||
.and_then(|attrs| match attrs.get(ANN_GENERATION_ATTR) {
|
||||
Some(AttrValue::I64(v)) => Some(*v as u64),
|
||||
_ => None,
|
||||
});
|
||||
let signed = meta_attrs(file).is_ok_and(|attrs| attrs.contains_key(SIG_VERSION_ATTR));
|
||||
CheckpointMeta {
|
||||
wal_applied: read_wal_mark(file),
|
||||
ann_generation,
|
||||
@@ -575,12 +592,7 @@ pub fn validate_and_load(
|
||||
file: &clawhdf5::File,
|
||||
) -> Result<(MemoryConfig, MemoryCache, SessionCache, KnowledgeCache), MemoryError> {
|
||||
// Read /meta group attributes
|
||||
let meta = file
|
||||
.group("meta")
|
||||
.map_err(|e| MemoryError::Schema(format!("missing /meta group: {e}")))?;
|
||||
let attrs = meta
|
||||
.attrs()
|
||||
.map_err(|e| MemoryError::Schema(format!("cannot read /meta attrs: {e}")))?;
|
||||
let attrs = meta_attrs(file)?;
|
||||
|
||||
let schema_version = match attrs.get("schema_version") {
|
||||
Some(AttrValue::String(s)) => s.clone(),
|
||||
|
||||
@@ -258,3 +258,43 @@ fn an_existing_f32_store_stays_f32() {
|
||||
assert_eq!(&values[..before.1.len()], before.1.as_slice());
|
||||
assert_eq!(&values[before.1.len()..], odd.as_slice());
|
||||
}
|
||||
|
||||
/// `Group::attrs` leaves out an attribute it cannot decode. A store whose
|
||||
/// `float16` setting is unreadable must not open as `float16 = false` (or with
|
||||
/// any other default in place of a setting it has): it is an error.
|
||||
#[test]
|
||||
fn unreadable_meta_attribute_fails_open_instead_of_defaulting() {
|
||||
let dir = TempDir::new().unwrap();
|
||||
let path = dir.path().join("store.h5");
|
||||
{
|
||||
let mut m = HDF5Memory::create(config(&dir, "store.h5", true)).unwrap();
|
||||
m.save(entry(1)).unwrap();
|
||||
m.flush_wal().unwrap();
|
||||
}
|
||||
assert!(HDF5Memory::open_read_only(&path).is_ok());
|
||||
|
||||
// Give the `float16` attribute message an unknown version (the name is
|
||||
// at +8 in a version-1 message and +9 in a version-3 one).
|
||||
let mut bytes = std::fs::read(&path).unwrap();
|
||||
let name = b"float16\0";
|
||||
let mut hit = false;
|
||||
let positions: Vec<usize> = (9..bytes.len() - name.len())
|
||||
.filter(|&p| &bytes[p..p + name.len()] == name)
|
||||
.collect();
|
||||
for pos in positions {
|
||||
for (back, version) in [(8, 1u8), (9, 3u8)] {
|
||||
if bytes[pos - back] == version {
|
||||
bytes[pos - back] = 0x7f;
|
||||
hit = true;
|
||||
}
|
||||
}
|
||||
}
|
||||
assert!(hit, "float16 attribute message not found");
|
||||
std::fs::write(&path, &bytes).unwrap();
|
||||
|
||||
match HDF5Memory::open_read_only(&path) {
|
||||
Err(MemoryError::Schema(msg)) => assert!(msg.contains("/meta"), "{msg}"),
|
||||
Err(e) => panic!("unexpected error: {e}"),
|
||||
Ok(_) => panic!("store opened with an unreadable float16 setting"),
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user