Skip to content

librasan: Fix unaligned memory access in memset #3173

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

Closed
wants to merge 1 commit into from

Conversation

wfdewith
Copy link
Contributor

@wfdewith wfdewith commented Apr 24, 2025

Description

When memset is used with 0x00 or 0xFF, the implementation takes an optimized path and sets the destination per word instead of per byte. However, this assumes that the destination pointer is aligned for word level access, which is not guaranteed. This PR ensures that the destination is set per byte until the pointer is aligned, after which it continues with setting the destination per word.

Checklist

  • I have run ./scripts/precommit.sh and addressed all comments

@wfdewith wfdewith marked this pull request as draft April 24, 2025 21:05
@wfdewith
Copy link
Contributor Author

Converted to draft until I've added unit tests for all functions in mem.rs.

@WorksButNotTested
Copy link
Collaborator

My assumption was that even unaligned word sized access would be quicker than single byte access. But if aligned access is faster still then certainly worth an update. Alternatively, if there are any cross-platform crates that achieve the same, then that's great too. It might be worth checking if this is a performance bottleneck in your instance though. Disabling this feature may also help you.

@wfdewith
Copy link
Contributor Author

I should've clarified it's not about performance but about correctness. Rust does not actually allow unaligned pointer dereferences, but this is a debug assert so it does not fail with the release profile. You can check for yourself by running the following in debug and release mode: https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=7f29761f2234c9c6341d923d189f0258, which works in release mode but panics in debug mode.

I assume that Rust decided on these semantics for all architectures because not all architectures allow unaligned memory access. x86_64 allows it at the cost of performance, but I'm working with some ARMv7 firmware and ARM does not allow unaligned memory access and will trap execution when it happens.

Weirdly enough, I never actually got a trap when testing my harness for this firmware with QEMU, instead I got all kinds of weird bugs that were hard to pinpoint, such as some segfault in ld.so because some internal linker memory structures suddenly became null bytes or an issue where the single thread got stuck waiting for the Mutex in libgasan.so, which should be impossible. After way too much debugging, I only realized what the problem was when I compiled libgasan.so with the debug profile because of the debug assertion. With the fix in this PR, everything suddenly started working with both debug and release profiles.

@WorksButNotTested
Copy link
Collaborator

OK fair enough then. Maybe worth a check it doesn't cause a significant performance regression on platforms which do allow unaligned access like x64.

@wfdewith
Copy link
Contributor Author

OK fair enough then. Maybe worth a check it doesn't cause a significant performance regression on platforms which do allow unaligned access like x64.

I cannot imagine that would be the case, but it shouldn't be relevant anyway since dereferencing misaligned pointers is just straight up undefined behavior, which means there is no guarantee that the Rust compiler outputs valid code at all with the release profile, even for architectures that support misaligned memory access.

@wfdewith
Copy link
Contributor Author

Honest question, why don't we just copy the musl implementations of the six functions in mem.rs (memmove, memcpy, memset, memcmp, bcmp and strlen)? We should be allowed to do that license-wise, and these implementations have a number of optimizations already. There is already some infrastructure for compiling C files in librasan, so it wouldn't complicate the build system. It also makes it very clear that these functions are not allowed to call other libc functions.

@WorksButNotTested
Copy link
Collaborator

That sounds like a great solution. I did look for some C implementations to use, but didn't have any luck finding something. Worth adding a comment saying where they came from though to make future maintenance a bit easier.

@wfdewith
Copy link
Contributor Author

Great, I'll open a new PR for that when it's done.

@wfdewith wfdewith closed this Apr 26, 2025
@WorksButNotTested
Copy link
Collaborator

Awesome. Thanks.

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

Successfully merging this pull request may close these issues.

2 participants