Skip to content

Remove access to unaligned refs from bzImage tests #127

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 5, 2023
Merged

Remove access to unaligned refs from bzImage tests #127

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions src/loader/x86_64/bzimage/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,6 @@ mod tests {
v
}

#[allow(unaligned_references)]
#[allow(non_snake_case)]
#[test]
fn test_load_bzImage() {
Expand All @@ -238,10 +237,23 @@ mod tests {
Some(highmem_start_address),
)
.unwrap();
let setup_header = loader_result.setup_header.unwrap();

assert_eq!(loader_result.kernel_load.raw_value(), 0x200000);
assert_eq!(loader_result.setup_header.unwrap().header, 0x53726448);
assert_eq!(loader_result.setup_header.unwrap().version, 0x20d);
assert_eq!(
// SAFETY:
// Reading the value from an unaligned address is not considered safe.
// but this is not an issue since this is a test.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is a bit ambiguous. Rust allows you to read the contents of fields in an struct even if they are not aligned if that type is Copy. What you can't do is safely deference references to unaligned members.

Here is a little example:

#[repr(packed)]
struct Foo {
    a: u8,
    b: u64
}

// This won't compile due to unaligned access as the members are referenced
//fn bad(foo: &Foo) {
//    println!("{} {}", foo.a, foo.b)
//}

// This version is fine as we can copy the values from the struct
fn good(foo: &Foo) {
    let (a, b) = (foo.a, foo.b);
    println!("{} {}", a, b)
}

fn main() {
    let foo = Foo {a: 1, b: 2};
    good(&foo)
}

I would therefore advise against using the word "reading" here. Alternatively if the values setup_header are just copied to the stack then all this could be simplfied to:

        let setup_header = loader_result.setup_header.unwrap();
        let header = setup_header.header; 
        assert_eq!(header, 0x53726448);

I hope this is helpful as I went through the same journey to understand the details of this error.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AlexandruCihodaru do you mind sending an update with the suggestion from @rbradford? This is a good catch!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. Will create a new PR this week.

unsafe { std::ptr::addr_of!(setup_header.header).read_unaligned() },
0x53726448
);
assert_eq!(
// SAFETY:
// Reading the value from an unaligned address is not considered safe.
// but this is not an issue since this is a test.
unsafe { std::ptr::addr_of!(setup_header.version).read_unaligned() },
0x20d
);
assert_eq!(loader_result.setup_header.unwrap().loadflags, 1);
assert_eq!(loader_result.kernel_end, 0x60D320);

Expand All @@ -253,11 +265,20 @@ mod tests {
Some(highmem_start_address),
)
.unwrap();
let setup_header = loader_result.setup_header.unwrap();

assert_eq!(loader_result.kernel_load.raw_value(), 0x100000);

// load bzImage withouth himem_start
// load bzImage without himem_start
loader_result = BzImage::load(&gm, None, &mut Cursor::new(&image), None).unwrap();
assert_eq!(0x53726448, loader_result.setup_header.unwrap().header);
// Reading the value from an unaligned address is not considered safe.
assert_eq!(
0x53726448,
// SAFETY:
// Reading the value from an unaligned address is not considered safe.
// but this is not an issue since this is a test.
unsafe { std::ptr::addr_of!(setup_header.header).read_unaligned() }
);
assert_eq!(loader_result.kernel_load.raw_value(), 0x100000);

// load bzImage with a bad himem setting
Expand Down