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.

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

Jump to bottom

memrchr uses unaligned loads #35967

Closed
bluss opened this issue Aug 24, 2016 · 5 comments
Closed

memrchr uses unaligned loads #35967

bluss opened this issue Aug 24, 2016 · 5 comments
Labels
I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness

Comments

@bluss
Copy link
Member

bluss commented Aug 24, 2016

Due to an arithmetic error, the fallback memrchr (used for non-linux) in libstd uses unaligned loads, the intention was to produce only well aligned pointers of course.

This was suspected to cause a crash using ARMv7, found by @tomaka

Original code also had the same issue: BurntSushi/memchr/pull/9

I believe that on x86 non-linux platforms, the bug was harmless and the function still produced the correct return value.
@bluss bluss mentioned this issue Aug 24, 2016
Merged
@bluss bluss added I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness A-libs labels Aug 24, 2016
@tomaka
Copy link
Contributor

tomaka commented Aug 24, 2016 •
edited
Loading

This was suspected to cause a crash using ARMv7, found by @tomaka

The crash is a SIGBUS (which is usually caused by unaligned loads), so this is likely the cause.

If anyone want to find more, the exact location is _ZN3std6memchr7memrchr17hbaa18487c977cd24E+164, using the rustc nightly from 2016-08-18. I don't know how to introspect the debug infos to find what exactly is the instruction at this location. EDIT: Oh, since libstd is compiled in release there's probably no debug info anyway.

@bluss
Copy link
Member Author

bluss commented Aug 24, 2016

If you have the time, you can try with the posted PR #35969 to verify the fix.

bors added a commit that referenced this issue Aug 27, 2016
@bors
Auto merge of #35969 - bluss:memrchr-alignment, r=nagisa
1194695 
memrchr: Correct aligned offset computation

The memrchr fallback did not compute the offset correctly. It was
intentioned to land on usize-aligned addresses but did not.
This was suspected to have resulted in a crash on ARMv7!

This bug affected non-linux platforms.

I think like this, if we have a slice with pointer `ptr` and length
`len`, we want to find the last usize-aligned offset in the slice.
The correct computation should be:

For example if ptr = 1 and len = 6, and `size_of::<usize>()` is 4:

```
[ x x x x x x ]
  1 2 3 4 5 6
        ^-- last aligned address at offset 3 from the start.
```

The last aligned address is ptr + len - (ptr + len) % usize_size.

Compute offset from the start as:

offset = len - (ptr + len) % usize_size = 6 - (1 + 6) % 4 = 6 - 3 = 3.

I believe the function's return value was always correct previously, if
the platform supported unaligned addresses.

Fixes #35967
@bluss
Copy link
Member Author

bluss commented Aug 27, 2016

@tomaka how relevant is this platform for stable rust? Should we beta-nominate the fix?

@tomaka
Copy link
Contributor

tomaka commented Aug 28, 2016

The crash disappeared after the latest nightly.

@bluss
Copy link
Member Author

bluss commented Aug 29, 2016

Thank you

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
I-unsound Issue: A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@tomaka @bluss