-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Use "read" to fetch misaligned 64bit values from bundle header #63431
Conversation
Tagging subscribers to this area: @agocke, @vitek-karas, @VSadov Issue DetailsFixes: #62273 Misalligned reads may cause crashes on ARM32, depending on system config.
|
if (bundle_major_version >= 6) | ||
{ | ||
reader.read(&fixed_data.compressedSize, sizeof(int64_t)); | ||
} | ||
|
||
fixed_data.type = *(file_type_t*)reader.read_direct(sizeof(file_type_t)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the size of file_type_t
here and how is it guaranteed to be aligned? or do reads of its size not need aligning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is an enum with base uint8_t, defined here:
enum file_type_t : uint8_t |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the read_direct of header_fixed_v2_t wouldn't potentially suffer from the same issue. It has 64 bit members. Maybe it would be safer and more future proof to drop the read_direct completely and always use just the read.
src/native/corehost/bundle/reader.h
Outdated
// Return a pointer to the requested bytes within the memory-mapped file. | ||
// Skip over len bytes. | ||
const char* read_direct(int64_t len) | ||
uint8_t read_byte() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: since this is only used from one place, is it worth adding? Why not just use the read
even for the single byte file type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed the same thing - there is read()
, which is the same as read_byte()
I've removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
Just FYI. I have got Raspbian installed on my PI and I was able to reproduce the failure. (after setting cpu/alignment handler to 4) Now I am at the stage where I want to make sure that the fix actually fixes the problem - i.e. whether after the fix there is no Bus error and apps are able to run. |
I have validated that an app built with singlefilehost that contains the fix runs on Raspbian (bullseye arm32) regardless of |
/backport to release/6.0 |
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1669737088 |
Fixes: #62273
Misalligned reads may cause crashes on ARM32, depending on system config.