Skip to content
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.

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

Unaligned Pointer Read #274

Open
petesmc opened this issue Dec 5, 2023 · 6 comments
Open

Unaligned Pointer Read #274

petesmc opened this issue Dec 5, 2023 · 6 comments

Comments

@petesmc
Copy link

petesmc commented Dec 5, 2023

This test using a i8 results in an unaligned pointer read...

hdu.write_key(&mut f, "ONE", 1i8).unwrap();

... due to

&value as *const $t as *mut c_void, 

... in the below.

impl WritesKey for $t {
fn write_key(f: &mut FitsFile, name: &str, value: Self) -> Result<()> {
let c_name = ffi::CString::new(name)?;
let mut status = 0;
let datatype = u8::from($datatype);
unsafe {
fits_write_key(
f.fptr.as_mut() as *mut _,
datatype as _,
c_name.as_ptr(),
&value as *const $t as *mut c_void,
ptr::null_mut(),
&mut status,
);
}
check_status(status)
}
}
};
}
writes_key_impl_int!(i8, DataType::TSHORT);

Which calls ffpkyj in the cfitsio library like so:

ffpkyj(fptr, keyname, (LONGLONG) *(short *) value, comm, status); // <--- pointer to i8 gets converted into pointer to short.

I believe to fix this, you need to convert i8 -> i16 given you are using TSHORT, OR map i8 to TBYTE instead.

@simonrw
Copy link
Owner

simonrw commented Dec 5, 2023

Hi, thanks for reporting this issue, and I'm glad you're using the project! You are absolutely right, the types do not match up for u8.

Do you have a minimal example which I can use to reproduce this issue, and make sure it doesn't happen again?

@petesmc
Copy link
Author

petesmc commented Dec 6, 2023

Sorry nothing I can share at the moment. I detected it because I'm rewriting part of cfitsio in rust which in debug mode checks for misaligned pointer reads.

@simonrw
Copy link
Owner

simonrw commented Dec 6, 2023

Ok thanks. I've merged in a fix and am ok to not include a test for now. Can you use the latest main commit and test for me please?

@simonrw
Copy link
Owner

simonrw commented Apr 5, 2024

@petesmc have you had a chance to test my fix?

@petesmc
Copy link
Author

petesmc commented Apr 11, 2024

Looks good for bytes, but think you need to update the u16/i16 as well to TUSHORT/TSHORT

@simonrw
Copy link
Owner

simonrw commented Jul 15, 2024

@petesmc done, can you check again?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants