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

Fix alignment issues (and a test-only buffer overflow) #81

Merged
merged 2 commits into from
May 11, 2023

Conversation

saethlin
Copy link
Contributor

Fixes #77

I ran this to find all the issues fixed here:

RUSTDOCFLAGS=-Zsanitizer=address ASAN_OPTIONS=detect_leaks=0 RUSTFLAGS=-Zsanitizer=address cargo +nightly test --target=x86_64-unknown-linux-gnu

ASan is still grumpy because JitMemory leaks the contents allocation. This is probably fine in practice I think because there won't be more than one JIT in a process? But if you want you could also add a Drop impl.

It would probably be good to use the above invocation when doing stuff in CI, but I'm awful with GitHub actions so I'll leave that to you if you want to do it.

Signed-off-by: Ben Kimock <kimockb@gmail.com>
Signed-off-by: Ben Kimock <kimockb@gmail.com>
Copy link
Owner

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks great, thanks a ton for fixing these!

@qmonnet
Copy link
Owner

qmonnet commented May 11, 2023

ASan is still grumpy because JitMemory leaks the contents allocation.

How do you observe these? I don't see any remaining complaints when running the command you provided.

This is probably fine in practice I think because there won't be more than one JIT in a process? But if you want you could also add a Drop impl.

Yes, at the moment there would be only one, I haven't looked at multithreading for rbpf at this stage. I guess I could look into the Drop in the future.

It would probably be good to use the above invocation when doing stuff in CI, but I'm awful with GitHub actions so I'll leave that to you if you want to do it.

Sounds good, and it's alright, I should be able to handle that when I can find a moment for it. Thanks!

@qmonnet qmonnet merged commit 14bdb38 into qmonnet:master May 11, 2023
@saethlin
Copy link
Contributor Author

How do you observe these? I don't see any remaining complaints when running the command you provided.

The command I provided sets ASAN_OPTIONS=detect_leaks=0 so it turns off leak reports. Delete that from the command and you should see complaints about leaks.

@qmonnet
Copy link
Owner

qmonnet commented May 19, 2023

Fixed the memory leak and added flags to CI in #82, thanks 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

Successfully merging this pull request may close these issues.

Tests panick with misaligned pointer dereference: address must be a multiple of ... but is ...
2 participants