Skip to content

Commit 52d2a75

Browse files
authored
Fix Witty memory allocation alignment (#3104)
## Motivation <!-- Briefly describe the goal(s) of this PR. --> When allocating heap memory in Witty, the alignment was incorrectly hard-coded to `1`. This causes issues when the allocated memory does not start at an aligned address. The code to store types will automatically add padding, which offsets the start of the write and can then overflow the allocated region. ## Proposal <!-- Summarize the proposed changes and how they address the goal(s) stated above. --> Specify the allocation alignment whenever needed. ## Test Plan <!-- Explain how you made sure that the changes are correct and that they perform as intended. Please describe testing protocols (CI, manual tests, benchmarks, etc) in a way that others can reproduce the results. --> Unit tests were added, including one that reproduces the issue `wit_store::test_list_fields` and now passes with the fix. ## Release Plan <!-- If this PR targets the `main` branch, **keep the applicable lines** to indicate if you recommend the changes to be picked in release branches, SDKs, and hotfixes. This generally concerns only bug fixes. Note that altering the public protocol (e.g. transaction format, WASM syscalls) or storage formats requires a new deployment. --> - Backporting is not possible but we may want to deploy a new `devnet` and release a new SDK soon. ## Links <!-- Optional section for related PRs, related issues, and other references. If needed, please create issues to track future improvements and link them here. --> - [reviewer checklist](https://github.com/linera-io/linera-protocol/blob/main/CONTRIBUTING.md#reviewer-checklist)
1 parent d6825b7 commit 52d2a75

File tree

11 files changed

+173
-23
lines changed

11 files changed

+173
-23
lines changed

linera-witty/src/imported_function_interface/parameters.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,8 @@ where
114114
Instance: InstanceWithMemory,
115115
<Instance::Runtime as Runtime>::Memory: RuntimeMemory<Instance>,
116116
{
117-
let location = memory.allocate(Parameters::SIZE)?;
117+
let location =
118+
memory.allocate(Parameters::SIZE, <Parameters::Layout as Layout>::ALIGNMENT)?;
118119

119120
parameters.store(memory, location)?;
120121
location.lower(memory)

linera-witty/src/runtime/error.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,10 @@ pub enum RuntimeError {
1414
#[error("Requested allocation size is too large")]
1515
AllocationTooLarge,
1616

17+
/// Attempt to allocate a buffer that's aligned to an invalid boundary.
18+
#[error("Requested allocation alignment is invalid")]
19+
InvalidAlignment,
20+
1721
/// Call to `cabi_realloc` returned a negative value instead of a valid address.
1822
#[error("Memory allocation failed")]
1923
AllocationFailed,

linera-witty/src/runtime/memory.rs

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -120,11 +120,12 @@ where
120120
self.memory.write(&mut *self.instance, location, bytes)
121121
}
122122

123-
/// Returns a newly allocated buffer of `size` bytes in the guest module's memory.
123+
/// Returns a newly allocated buffer of `size` bytes in the guest module's memory
124+
/// aligned to the requested `alignment`.
124125
///
125126
/// Calls the guest module to allocate the memory, so the resulting allocation is managed by
126127
/// the guest.
127-
pub fn allocate(&mut self, size: u32) -> Result<GuestPointer, RuntimeError> {
128+
pub fn allocate(&mut self, size: u32, alignment: u32) -> Result<GuestPointer, RuntimeError> {
128129
if self.cabi_realloc.is_none() {
129130
self.cabi_realloc = Some(<Instance as InstanceWithFunction<
130131
HList![i32, i32, i32, i32],
@@ -133,14 +134,16 @@ where
133134
}
134135

135136
let size = i32::try_from(size).map_err(|_| RuntimeError::AllocationTooLarge)?;
137+
let alignment = i32::try_from(alignment).map_err(|_| RuntimeError::InvalidAlignment)?;
136138

137139
let cabi_realloc = self
138140
.cabi_realloc
139141
.as_ref()
140142
.expect("`cabi_realloc` function was not loaded before it was called");
141143

142-
let hlist_pat![allocation_address] =
143-
self.instance.call(cabi_realloc, hlist![0, 0, 1, size])?;
144+
let hlist_pat![allocation_address] = self
145+
.instance
146+
.call(cabi_realloc, hlist![0, 0, alignment, size])?;
144147

145148
Ok(GuestPointer(
146149
allocation_address

linera-witty/src/test.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ where
2222
let mut first_instance = MockInstance::<()>::default();
2323
let mut first_memory = first_instance.memory()?;
2424

25-
let first_address = first_memory.allocate(T::SIZE)?;
25+
let first_address = first_memory.allocate(T::SIZE, <T::Layout as Layout>::ALIGNMENT)?;
2626

2727
input.store(&mut first_memory, first_address)?;
2828

@@ -34,11 +34,11 @@ where
3434
let mut second_instance = MockInstance::<()>::default();
3535
let mut second_memory = second_instance.memory()?;
3636

37-
let second_address = second_memory.allocate(T::SIZE)?;
37+
let second_address = second_memory.allocate(T::SIZE, <T::Layout as Layout>::ALIGNMENT)?;
3838

3939
loaded_instance.store(&mut second_memory, second_address)?;
4040

41-
let total_allocated_memory = first_memory.allocate(0)?.0;
41+
let total_allocated_memory = first_memory.allocate(0, 1)?.0;
4242

4343
assert_eq!(
4444
first_memory.read(first_address, total_allocated_memory)?,
@@ -57,7 +57,7 @@ where
5757
{
5858
let mut first_instance = MockInstance::<()>::default();
5959
let mut first_memory = first_instance.memory()?;
60-
let first_start_address = first_memory.allocate(0)?;
60+
let first_start_address = first_memory.allocate(0, 1)?;
6161

6262
let first_lowered_layout = input.lower(&mut first_memory)?;
6363
let lifted_instance = T::lift_from(first_lowered_layout, &first_memory)?;
@@ -67,13 +67,13 @@ where
6767
// Create a clean separate memory instance
6868
let mut second_instance = MockInstance::<()>::default();
6969
let mut second_memory = second_instance.memory()?;
70-
let second_start_address = second_memory.allocate(0)?;
70+
let second_start_address = second_memory.allocate(0, 1)?;
7171

7272
let second_lowered_layout = lifted_instance.lower(&mut second_memory)?;
7373

7474
assert_eq!(first_lowered_layout, second_lowered_layout);
7575

76-
let total_allocated_memory = first_memory.allocate(0)?.0;
76+
let total_allocated_memory = first_memory.allocate(0, 1)?.0;
7777

7878
assert_eq!(
7979
first_memory.read(first_start_address, total_allocated_memory)?,

linera-witty/src/type_traits/implementations/std/string.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ impl WitStore for String {
7373
<Instance::Runtime as Runtime>::Memory: RuntimeMemory<Instance>,
7474
{
7575
let length = u32::try_from(self.len())?;
76-
let destination = memory.allocate(length)?;
76+
let destination = memory.allocate(length, 1)?;
7777

7878
destination.store(memory, location)?;
7979
length.store(memory, location.after::<GuestPointer>())?;
@@ -90,7 +90,7 @@ impl WitStore for String {
9090
<Instance::Runtime as Runtime>::Memory: RuntimeMemory<Instance>,
9191
{
9292
let length = u32::try_from(self.len())?;
93-
let destination = memory.allocate(length)?;
93+
let destination = memory.allocate(length, 1)?;
9494

9595
memory.write(destination, self.as_bytes())?;
9696

linera-witty/src/type_traits/implementations/std/vec.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,7 @@ where
8484
let length = u32::try_from(self.len())?;
8585
let size = length * T::SIZE;
8686

87-
let destination = memory.allocate(size)?;
87+
let destination = memory.allocate(size, <T::Layout as Layout>::ALIGNMENT)?;
8888

8989
destination.store(memory, location)?;
9090
length.store(memory, location.after::<GuestPointer>())?;
@@ -105,7 +105,7 @@ where
105105
let length = u32::try_from(self.len())?;
106106
let size = length * T::SIZE;
107107

108-
let destination = memory.allocate(size)?;
108+
let destination = memory.allocate(size, <T::Layout as Layout>::ALIGNMENT)?;
109109

110110
self.iter().zip(0..).try_for_each(|(element, index)| {
111111
element.store(memory, destination.index::<T>(index))

linera-witty/src/type_traits/implementations/tests.rs

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,7 +228,9 @@ where
228228

229229
assert_eq!(layout_length, T::SIZE);
230230

231-
let layout_address = memory.allocate(layout_length).unwrap();
231+
let layout_address = memory
232+
.allocate(layout_length, <T::Layout as Layout>::ALIGNMENT)
233+
.unwrap();
232234
let heap_address = layout_address.after::<T>();
233235

234236
input.store(&mut memory, layout_address).unwrap();

linera-witty/tests/common/types.rs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,3 +78,10 @@ pub struct StructWithHeapFields {
7878
pub rced: Rc<Leaf>,
7979
pub arced: Arc<Enum>,
8080
}
81+
82+
/// A struct that contains fields that should all be represented as lists.
83+
#[derive(Clone, Debug, Eq, PartialEq, WitType, WitLoad, WitStore)]
84+
pub struct StructWithLists {
85+
pub vec: Vec<SimpleWrapper>,
86+
pub second_vec: Vec<TupleWithPadding>,
87+
}

linera-witty/tests/wit_load.rs

Lines changed: 51 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,8 @@ use linera_witty::{hlist, InstanceWithMemory, Layout, MockInstance, RuntimeError
1313

1414
use self::types::{
1515
Branch, Enum, Leaf, RecordWithDoublePadding, SimpleWrapper, SpecializedGenericEnum,
16-
SpecializedGenericStruct, StructWithHeapFields, TupleWithPadding, TupleWithoutPadding,
16+
SpecializedGenericStruct, StructWithHeapFields, StructWithLists, TupleWithPadding,
17+
TupleWithoutPadding,
1718
};
1819

1920
/// Check that a wrapper type is properly loaded from memory and lifted from its flat layout.
@@ -272,7 +273,7 @@ fn test_invalid_discriminant() {
272273
17,
273274
];
274275

275-
let address = memory.allocate(memory_bytes.len() as u32).unwrap();
276+
let address = memory.allocate(memory_bytes.len() as u32, 1).unwrap();
276277

277278
memory.write(address, &memory_bytes).unwrap();
278279

@@ -353,6 +354,52 @@ fn test_heap_allocated_fields() {
353354
);
354355
}
355356

357+
/// Check that a [`Vec`] type is properly loaded from memory and lifted from its flat
358+
/// layout.
359+
#[test]
360+
fn test_vec() {
361+
let expected = vec![SimpleWrapper(false), SimpleWrapper(true)];
362+
363+
test_load_from_memory(&[8, 0, 0, 0, 2, 0, 0, 0, 0, 1], expected.clone());
364+
test_lift_from_flat_layout(hlist![0_i32, 2_i32], expected, &[0, 1]);
365+
}
366+
367+
/// Check that a type with list fields is properly loaded from memory and lifted from its
368+
/// flat layout.
369+
#[test]
370+
fn test_list_fields() {
371+
let expected = StructWithLists {
372+
vec: vec![
373+
SimpleWrapper(true),
374+
SimpleWrapper(true),
375+
SimpleWrapper(false),
376+
],
377+
second_vec: vec![
378+
TupleWithPadding(0x1a19, 0x201f_1e1d, 0x2827_2625_2423_2221),
379+
TupleWithPadding(0x2a29, 0x302f_2e2d, 0x3837_3635_3433_3231),
380+
],
381+
};
382+
383+
test_load_from_memory(
384+
&[
385+
16, 0, 0, 0, 3, 0, 0, 0, 24, 0, 0, 0, 2, 0, 0, 0, 1, 1, 0, 20, 21, 22, 23, 24, 0x19,
386+
0x1a, 27, 28, 0x1d, 0x1e, 0x1f, 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28,
387+
0x29, 0x2a, 43, 44, 0x2d, 0x2e, 0x2f, 0x30, 0x31, 0x32, 0x33, 0x34, 0x35, 0x36, 0x37,
388+
0x38,
389+
],
390+
expected.clone(),
391+
);
392+
test_lift_from_flat_layout(
393+
hlist![0_i32, 3_i32, 8_i32, 2_i32],
394+
expected,
395+
&[
396+
1, 1, 0, 0, 0, 0, 0, 0, 0x19, 0x1a, 0, 0, 0x1d, 0x1e, 0x1f, 0x20, 0x21, 0x22, 0x23,
397+
0x24, 0x25, 0x26, 0x27, 0x28, 0x29, 0x2a, 0, 0, 0x2d, 0x2e, 0x2f, 0x30, 0x31, 0x32,
398+
0x33, 0x34, 0x35, 0x36, 0x37, 0x38,
399+
],
400+
);
401+
}
402+
356403
/// Tests that the type `T` and wrapped versions of it can be loaded from an `input` sequence of
357404
/// bytes in memory and that it matches the `expected` value.
358405
fn test_load_from_memory<T>(input: &[u8], expected: T)
@@ -374,7 +421,7 @@ where
374421
let mut instance = MockInstance::<()>::default();
375422
let mut memory = instance.memory().unwrap();
376423

377-
let address = memory.allocate(input.len() as u32).unwrap();
424+
let address = memory.allocate(input.len() as u32, 1).unwrap();
378425

379426
memory.write(address, input).unwrap();
380427

@@ -409,7 +456,7 @@ fn test_single_lift_from_flat_layout<T>(
409456
let mut instance = MockInstance::<()>::default();
410457
let mut memory = instance.memory().unwrap();
411458

412-
let start_address = memory.allocate(initial_memory.len() as u32).unwrap();
459+
let start_address = memory.allocate(initial_memory.len() as u32, 1).unwrap();
413460

414461
memory.write(start_address, initial_memory).unwrap();
415462

linera-witty/tests/wit_store.rs

Lines changed: 47 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,8 @@ use linera_witty::{hlist, InstanceWithMemory, Layout, MockInstance, WitStore};
1212

1313
use self::types::{
1414
Branch, Enum, Leaf, RecordWithDoublePadding, SimpleWrapper, SpecializedGenericEnum,
15-
SpecializedGenericStruct, StructWithHeapFields, TupleWithPadding, TupleWithoutPadding,
15+
SpecializedGenericStruct, StructWithHeapFields, StructWithLists, TupleWithPadding,
16+
TupleWithoutPadding,
1617
};
1718

1819
/// Check that a wrapper type is properly stored in memory and lowered into its flat layout.
@@ -400,6 +401,50 @@ fn test_heap_allocated_fields() {
400401
);
401402
}
402403

404+
/// Check that a [`Vec`] type is properly stored in memory and lowered into its flat layout.
405+
#[test]
406+
fn test_vec() {
407+
let data = vec![
408+
SimpleWrapper(true),
409+
SimpleWrapper(false),
410+
SimpleWrapper(true),
411+
];
412+
413+
test_store_in_memory(data.clone(), &[8, 0, 0, 0, 3, 0, 0, 0], &[1, 0, 1]);
414+
test_lower_to_flat_layout(data, hlist![0_i32, 3_i32,], &[1, 0, 1]);
415+
}
416+
417+
/// Check that a type with list fields is properly stored in memory and lowered into its
418+
/// flat layout.
419+
#[test]
420+
fn test_list_fields() {
421+
let data = StructWithLists {
422+
vec: vec![
423+
SimpleWrapper(false),
424+
SimpleWrapper(true),
425+
SimpleWrapper(false),
426+
SimpleWrapper(true),
427+
],
428+
second_vec: vec![TupleWithPadding(1, 0, -1), TupleWithPadding(10, 11, 12)],
429+
};
430+
431+
let expected_heap = [0, 1, 0, 1]
432+
.into_iter()
433+
.chain([0; 4])
434+
.chain([
435+
1, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
436+
])
437+
.chain([10, 0, 0, 0, 11, 0, 0, 0, 12, 0, 0, 0, 0, 0, 0, 0])
438+
.collect::<Vec<_>>();
439+
440+
test_store_in_memory(
441+
data.clone(),
442+
&[16, 0, 0, 0, 4, 0, 0, 0, 24, 0, 0, 0, 2, 0, 0, 0],
443+
&expected_heap,
444+
);
445+
test_lower_to_flat_layout(data, hlist![0_i32, 4_i32, 8_i32, 2_i32], &expected_heap);
446+
}
447+
403448
/// Tests that the `data` of type `T` and wrapped versions of it can be stored as a sequence of
404449
/// bytes in memory and that they match the `expected` bytes.
405450
fn test_store_in_memory<T>(
@@ -439,7 +484,7 @@ fn test_single_store_in_memory<T>(
439484
let mut memory = instance.memory().unwrap();
440485

441486
let length = expected_without_allocation.len() as u32;
442-
let address = memory.allocate(length).unwrap();
487+
let address = memory.allocate(length, 1).unwrap();
443488

444489
data.store(&mut memory, address).unwrap();
445490

0 commit comments

Comments
 (0)