Skip to content

Commit e482ca8

Browse files
authored
Add object size and bounds validation when reading journal files. (netdata#21697)
1. Validate object size is at least sizeof(ObjectHeader) 2. Check that objects are not located in the file header area 3. Verify object offset + size doesn't overflow 4. Ensure objects don't exceed the journal's arena bounds On the writer side we update `arena_size` immediately after writing each object (rather than at entry completion, similar to systemd). This ensures subsequent reads within the same write operation can find newly written objects.
1 parent f1394e0 commit e482ca8

File tree

4 files changed

+79
-15
lines changed

4 files changed

+79
-15
lines changed

src/crates/journal-core/src/error.rs

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,12 @@ pub enum JournalError {
1212
#[error("invalid object type")]
1313
InvalidObjectType,
1414

15+
#[error("invalid object size: {0}")]
16+
InvalidObjectSize(u64),
17+
18+
#[error("object exceeds file bounds")]
19+
ObjectExceedsFileBounds,
20+
1521
#[error("invalid object location")]
1622
InvalidObjectLocation,
1723

src/crates/journal-core/src/file/file.rs

Lines changed: 42 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -420,9 +420,15 @@ impl<M: MemoryMap> JournalFile<M> {
420420
{
421421
validate_offset_alignment(offset)?;
422422

423-
let is_compact = self
424-
.journal_header_ref()
425-
.has_incompatible_flag(HeaderIncompatibleFlags::Compact);
423+
let journal_header = self.journal_header_ref();
424+
let is_compact = journal_header.has_incompatible_flag(HeaderIncompatibleFlags::Compact);
425+
let header_size = journal_header.header_size;
426+
let arena_end = header_size + journal_header.arena_size;
427+
428+
// Objects cannot be located in the file header
429+
if offset.get() < header_size {
430+
return Err(JournalError::ObjectExceedsFileBounds);
431+
}
426432

427433
self.window_manager.with_guarded(offset, |wm| {
428434
// Get the object header to determine size
@@ -431,9 +437,18 @@ impl<M: MemoryMap> JournalFile<M> {
431437
wm.get_slice(offset.get(), std::mem::size_of::<ObjectHeader>() as u64)?;
432438
let header = ObjectHeader::ref_from_bytes(header_slice)
433439
.map_err(|_| JournalError::ZerocopyFailure)?;
434-
header.size
440+
header.validated_size()?
435441
};
436442

443+
// Validate that the object doesn't exceed the journal's arena bounds
444+
let end_offset = offset
445+
.get()
446+
.checked_add(size_needed)
447+
.ok_or(JournalError::ObjectExceedsFileBounds)?;
448+
if end_offset > arena_end {
449+
return Err(JournalError::ObjectExceedsFileBounds);
450+
}
451+
437452
// Get the full object data
438453
let data = wm.get_slice(offset.get(), size_needed)?;
439454

@@ -869,15 +884,22 @@ impl<M: MemoryMapMut> JournalFile<M> {
869884
{
870885
validate_offset_alignment(offset)?;
871886

872-
let is_compact = self
873-
.journal_header_ref()
874-
.has_incompatible_flag(HeaderIncompatibleFlags::Compact);
887+
let journal_header = self.journal_header_ref();
888+
let is_compact = journal_header.has_incompatible_flag(HeaderIncompatibleFlags::Compact);
889+
let header_size = journal_header.header_size;
890+
let arena_end = header_size + journal_header.arena_size;
891+
892+
// Objects cannot be located in the file header
893+
if offset.get() < header_size {
894+
return Err(JournalError::ObjectExceedsFileBounds);
895+
}
875896

876897
self.window_manager.with_guarded(offset, |wm| {
877898
// Get or set the size
878899
let size_needed = match size {
879900
Some(size) => {
880-
// Setting object header for a new object
901+
// Setting object header for a new object (no bounds check needed,
902+
// the file will be extended as necessary)
881903
let header_slice =
882904
wm.get_slice_mut(offset.get(), std::mem::size_of::<ObjectHeader>() as u64)?;
883905
let header = ObjectHeader::mut_from_bytes(header_slice)
@@ -895,7 +917,18 @@ impl<M: MemoryMapMut> JournalFile<M> {
895917
if header.type_ != type_ as u8 {
896918
return Err(JournalError::InvalidObjectType);
897919
}
898-
header.size
920+
let size = header.validated_size()?;
921+
922+
// Validate that the object doesn't exceed the journal's arena bounds
923+
let end_offset = offset
924+
.get()
925+
.checked_add(size)
926+
.ok_or(JournalError::ObjectExceedsFileBounds)?;
927+
if end_offset > arena_end {
928+
return Err(JournalError::ObjectExceedsFileBounds);
929+
}
930+
931+
size
899932
}
900933
};
901934

src/crates/journal-core/src/file/object.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,21 @@ impl ObjectHeader {
419419
pub fn aligned_size(&self) -> u64 {
420420
(self.size + 7) & !7
421421
}
422+
423+
/// Validates that the object size is sane.
424+
///
425+
/// Returns the size if valid, or an error if the size is invalid.
426+
/// This should be called when reading an ObjectHeader from a journal file
427+
/// to protect against corrupted data.
428+
pub fn validated_size(&self) -> crate::error::Result<u64> {
429+
let min_size = std::mem::size_of::<ObjectHeader>() as u64;
430+
431+
if self.size < min_size {
432+
return Err(crate::error::JournalError::InvalidObjectSize(self.size));
433+
}
434+
435+
Ok(self.size)
436+
}
422437
}
423438

424439
#[derive(Debug, Copy, Clone, FromBytes, IntoBytes, KnownLayout, Immutable)]

src/crates/journal-core/src/file/writer.rs

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ impl JournalWriter {
146146

147147
entry_guard.header.object_header.aligned_size()
148148
};
149-
self.object_added(entry_offset, entry_size);
149+
self.object_added(journal_file, entry_offset, entry_size);
150150

151151
self.append_to_entry_array(journal_file, entry_offset)?;
152152
for entry_item_index in 0..self.entry_items.len() {
@@ -158,10 +158,20 @@ impl JournalWriter {
158158
Ok(())
159159
}
160160

161-
fn object_added(&mut self, object_offset: NonZeroU64, object_size: u64) {
161+
fn object_added(
162+
&mut self,
163+
journal_file: &mut JournalFile<MmapMut>,
164+
object_offset: NonZeroU64,
165+
object_size: u64,
166+
) {
162167
self.tail_object_offset = object_offset;
163168
self.append_offset = object_offset.saturating_add(object_size);
164169
self.num_written_objects += 1;
170+
171+
// Update arena_size immediately after writing, so subsequent reads
172+
// within the same write operation can find the newly written object.
173+
let header = journal_file.journal_header_mut();
174+
header.arena_size = self.append_offset.get() - header.header_size;
165175
}
166176

167177
fn entry_added(&mut self, header: &mut JournalHeader, realtime: u64, monotonic: u64) {
@@ -211,7 +221,7 @@ impl JournalWriter {
211221
data_guard.header.object_header.aligned_size()
212222
};
213223

214-
self.object_added(data_offset, data_size);
224+
self.object_added(journal_file, data_offset, data_size);
215225

216226
// Update hash table
217227
journal_file.data_hash_table_set_tail_offset(hash, data_offset)?;
@@ -264,7 +274,7 @@ impl JournalWriter {
264274
field_guard.set_payload(payload);
265275
field_guard.header.object_header.aligned_size()
266276
};
267-
self.object_added(field_offset, field_size);
277+
self.object_added(journal_file, field_offset, field_size);
268278

269279
// Update hash table
270280
journal_file.field_hash_table_set_tail_offset(hash, field_offset)?;
@@ -277,7 +287,7 @@ impl JournalWriter {
277287

278288
fn allocate_new_array(
279289
&mut self,
280-
journal_file: &JournalFile<MmapMut>,
290+
journal_file: &mut JournalFile<MmapMut>,
281291
capacity: NonZeroU64,
282292
) -> Result<NonZeroU64> {
283293
// let new_capacity = previous_capacity.saturating_mul(NonZeroU64::new(2).unwrap());
@@ -288,7 +298,7 @@ impl JournalWriter {
288298

289299
array_guard.header.object_header.aligned_size()
290300
};
291-
self.object_added(array_offset, array_size);
301+
self.object_added(journal_file, array_offset, array_size);
292302

293303
Ok(array_offset)
294304
}

0 commit comments

Comments
 (0)