-
Notifications
You must be signed in to change notification settings - Fork 12.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
fix some uses of pointer intrinsics with invalid pointers #53804
Conversation
r? @shepmaster (rust_highfive has picked a reviewer for you, use r? to override) |
@@ -2410,9 +2410,8 @@ impl<T> Iterator for IntoIter<T> { | |||
// same pointer. | |||
self.ptr = arith_offset(self.ptr as *const i8, 1) as *mut T; | |||
|
|||
// Use a non-null pointer value | |||
// (self.ptr might be null because of wrapping) | |||
Some(ptr::read(1 as *mut 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.
Would it be possible for ptr::read
to contain a debug_assert_eq!(ptr.align_offset(align_of::<T>()), 0)
?
If that assert fails, it would currently be UB, so we might as well just panic in debug builds to at least catch these inside of std
on CI. Maybe someday users will benefit from these without having to use miri to detect these issues.
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.
My plan was to open an E-easy issue (once #53783 lands) to decorate all these functions with a debug_assert!
testing non-NULL and being aligned.
I would however use ptr as usize % mem::align_of<T>() == 0
, because your test will always fail on miri. Remember, align_offset
can spuriously "fail" (return usize::max
).
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.
That's a good idea.
Remember, align_offset can spuriously fail.
I didn't know this :/ whyyyy
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.
Because we cannot implement it completely in miri/CTFE -- allocations do not actually have "real" base addresses there.
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.
Ah, that makes sense. This is getting off topic, but couldn't miri track the alignment requested for an allocation and use that in align_offset
?
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 could, and it could give a better return value in that case.
But align_offset
is also used e.g. on a large u8
allocation to find a subslice that is 256bit-aligned for use with SSE. There is no way for miri to provide an alignment stronger than what the allocation promises.
So maybe it could be made good enough for your test, not sure... the %
works today though.
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 a bit worrying that we might prefer slower run-time code because it works on miri to fast run-time code. Does the %
lower to the same LLVM-IR than align_offset
, at least after running opt
?
An alternative is to use conditional compilation, and use %
for miri
and align_offset
for run-time rust.
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 don't see how %
could be any slower than the intrinsic. If anything, align_offset
is doing the harder job (computing which offset to add, not just testing it for 0) so I'd expect it to be slower.
Currently, LLVM optimizes the to the same code to the extend that they get deduplicated.^^
@@ -742,7 +742,9 @@ impl<K, V> RawTable<K, V> { | |||
) -> Result<RawTable<K, V>, CollectionAllocErr> { | |||
unsafe { | |||
let ret = RawTable::new_uninitialized_internal(capacity, fallibility)?; | |||
ptr::write_bytes(ret.hashes.ptr(), 0, capacity); | |||
if capacity > 0 { |
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.
Instead of adding this workaround, I would prefer if here:
rust/src/libstd/collections/hash/table.rs
Line 35 in 9f9f2c0
const EMPTY: usize = 1; |
we would change EMPTY
from a const
to a const fn empty<T>() -> *mut T;
that just returns NonNull::dangling().as_ptr()
. I think EMPTY
is only used twice in the whole module.
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.
/// Note: when the pointer is initialized to EMPTY `.ptr()` will return
/// null and the tag functions shouldn't be used.
The table states this, but there are no debug_assert!
s preventing misuse AFAICT, might be worth it to add these as well.
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 am afraid of touching HashMap
, so I'd prefer if someone who knows anything about that code does those changes...
The pointer here was previously NULL, so I don't think it came from EMPTY
.
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.
If capacity == 0, then new_uninitialized_internal
can only return (
rust/src/libstd/collections/hash/table.rs
Line 688 in 9f9f2c0
if capacity == 0 { |
)
hashes
with this value:
hashes: TaggedHashUintPtr::new(EMPTY as *mut HashUint),
which basically calls (
rust/src/libstd/collections/hash/table.rs
Line 45 in 9f9f2c0
unsafe fn new(ptr: *mut HashUint) -> Self { |
Unique::new_unchecked(EMPTY as *mut HashUint)
Unique::ptr()
will then just return EMPTY as *mut HashUint
which is never null, so I have no idea how this pointer there could ever be NULL
. It should be EMPTY
. That is still wrong because of incorrect alignment, but that's a different type of wrong. Or what am I missing?
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.
The table states this, but there are no debug_assert!s preventing misuse AFAICT, might be worth it to add these as well.
Well, the tag functions cannot assert anything reasonable because the last bit can be either 0 or 1.
I think this also uses values as sentinel values in ways it should not. But that really needs someone to actually dig in the code. I can open an issue about that if you want, right now I only have time to do the most obvious thing.
I have no idea how this pointer there could ever be NULL. It should be EMPTY.
As the comment you quoted says, ptr()
will return NULL for EMPTY
.
If EMPTY
was properly aligned and ptr
did not do anything, we would not even need any test here on write_bytes
because non-NULL aligned zero-sized writes are perfectly fine.
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 can open an issue about that if you want, right now I only have time to do the most obvious thing.
Yeah, this is already an improvement.
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.
Done, I opened #53853.
Anything left to do in this PR?
This LGTM. @shepmaster ? |
Sounds like a good idea. It may be worth adding some instructions for anyone interested in reviewing with Miri in the tracking issue. Was it similar to the steps taken in #53564 (comment) and #53566? |
TBH I think the review has to mostly proceed by looking at the code. And we should find a way to run the libstd tests (and doctests) in miri. Doing that manually seems not worthwhile? But the steps are essentially, run some code in miri and see what happens.^^ miri only detects UB that actually runs, so you basically need an exhaustive test suite to be sure. Also, which tracking issue? What seems much more tractable at this point is to add |
Sorry for the somewhat unrelated question, but what is the point of using |
👍 would miri fit in a custom test framework? #53410 then allow any crate mark a function as a miri test.
I meant if an issue was opened for tracking reviews all over std, it could be added there. |
I opened an issue to add
Good question. No idea. Again, my intention was to do the most obvious thing and only that.
Maybe? But really what I mean is that it should run the same test as what currently runs with |
|
@shepmaster this awaits your review :) |
r? @nagisa |
@bors r+ |
📌 Commit 357c5da has been approved by |
fix some uses of pointer intrinsics with invalid pointers [Found by miri](rust-lang/miri#446): * `Vec::into_iter` calls `ptr::read` (and the underlying `copy_nonoverlapping`) with an unaligned pointer to a ZST. [According to LLVM devs](https://bugs.llvm.org/show_bug.cgi?id=38583), this is UB because it contradicts the metadata we are attaching to that pointer. * `HashMap` creation calls `ptr:.write_bytes` on a NULL pointer with a count of 0. This is likely not currently UB *currently*, but it violates the rules we are setting in #53783, and we might want to exploit those rules later (e.g. with more `nonnull` attributes for LLVM). Probably what `HashMap` really should do is use `NonNull::dangling()` instead of 0 for the empty case, but that would require a more careful analysis of the code. It seems like ideally, we should do a review of usage of such intrinsics all over libstd to ensure that they use valid pointers even when the size is 0. Is it worth opening an issue for that?
☀️ Test successful - status-appveyor, status-travis |
Fix UB in hashglobe <!-- Please describe your changes on the following line: --> This is a backport of rust-lang/rust#53804 Currently, this bug cause Firefox crash with Rust 1.56 ( LLVM 13 ) <details> <summary>backtrace of Firefox</summary> ``` (lldb) bt * thread #1, name = 'GeckoMain', stop reason = signal SIGSEGV: invalid address (fault address: 0x0) * frame #0: 0x00007ffff4ff7bde libxul.so`::RustMozCrash(const char *, int, const char *) [inlined] MOZ_Crash(aLine=2220, aReason="attempt to write to unaligned or null pointer") at Assertions.h:256:3 frame #1: 0x00007ffff4ff7bd4 libxul.so`::RustMozCrash(aFilename="/rustc/1.56.0/library/core/src/intrinsics.rs", aLine=2220, aReason="attempt to write to unaligned or null pointer") at wrappers.cpp:18:3 frame #2: 0x00007ffff4ff7b53 libxul.so`mozglue_static::panic_hook::h91947f48d75eb4dd(info=<unavailable>) at lib.rs:91:9 frame #3: 0x00007ffff4ff6e19 libxul.so`core::ops::function::Fn::call::h2f4e62c593234181((null)=<unavailable>, (null)=<unavailable>) at function.rs:70:5 frame #4: 0x00007ffff5bd055b libxul.so`std::panicking::rust_panic_with_hook::h41696e81832261ff(payload=&mut dyn core::panic::BoxMeUp @ 0x00007f24a7e5fc70, message=Option<&core::fmt::Arguments> @ r13, location=<unavailable>) at panicking.rs:628:17 frame #5: 0x00007ffff5bd00a2 libxul.so`std::panicking::begin_panic_handler::_$u7b$$u7b$closure$u7d$$u7d$::hea2a534982472bd3 at panicking.rs:519:13 frame #6: 0x00007ffff5bcc494 libxul.so`std::sys_common::backtrace::__rust_end_short_backtrace::h793de5eec3283122(f=<unavailable>) at backtrace.rs:141:18 frame #7: 0x00007ffff5bd0039 libxul.so`rust_begin_unwind(info=0x00007fffffff7a78) at panicking.rs:517:5 frame #8: 0x00007ffff5c2ece1 libxul.so`core::panicking::panic_fmt::h43c4759d9f1ef313(fmt=<unavailable>) at panicking.rs:101:14 frame #9: 0x00007ffff5c2ebbd libxul.so`core::panicking::panic::hb6dc0edf878703a5(expr=<unavailable>) at panicking.rs:50:5 frame #10: 0x00007ffff56dd397 libxul.so`core::intrinsics::write_bytes::h481ad0b8372e9e0a(dst=0x0000000000000000, val='\0', count=0) at intrinsics.rs:2220:5 frame #11: 0x00007ffff589e749 libxul.so`hashglobe::table::RawTable$LT$K$C$V$GT$::new::h04532bdf928a2865(capacity=0) at table.rs:839:13 frame #12: 0x00007ffff58b38f0 libxul.so`hashglobe::hash_map::HashMap$LT$K$C$V$C$S$GT$::try_with_hasher::h7086fbc016a9427d(hash_builder=<unavailable>) at hash_map.rs:622:20 frame #13: 0x00007ffff58b3077 libxul.so`hashglobe::hash_map::HashMap$LT$K$C$V$C$S$GT$::with_hasher::h9ee840b6d255f9fa(hash_builder=<unavailable>) at hash_map.rs:628:9 frame #14: 0x00007ffff5812c99 libxul.so`_$LT$hashglobe..hash_map..HashMap$LT$K$C$V$C$S$GT$$u20$as$u20$core..default..Default$GT$::default::h7a34c6ba884b9658 at hash_map.rs:1329:9 frame #15: 0x00007ffff58dfb3a libxul.so`_$LT$style..selector_map..MaybeCaseInsensitiveHashMap$LT$style..gecko_string_cache..Atom$C$V$GT$$u20$as$u20$core..default..Default$GT$::default::h2c19828653342158 at selector_map.rs:704:37 frame #16: 0x00007ffff5978919 libxul.so`_$LT$style..invalidation..stylesheets..StylesheetInvalidationSet$u20$as$u20$core..default..Default$GT$::default::h16e0d0431f387b3d at stylesheets.rs:103:5 frame #17: 0x00007ffff58d54b9 libxul.so`style::invalidation::stylesheets::StylesheetInvalidationSet::new::h4eedeb3b15c2c2c5 at stylesheets.rs:112:9 frame #18: 0x00007ffff58e43a6 libxul.so`style::stylesheet_set::DocumentStylesheetSet$LT$S$GT$::new::hf80ba16d4d55a4ca at stylesheet_set.rs:516:28 frame #19: 0x00007ffff58f269a libxul.so`style::stylist::StylistStylesheetSet::new::h66b5d09ea8a90d6e at stylist.rs:462:30 frame #20: 0x00007ffff58f26f0 libxul.so`style::stylist::Stylist::new::h4732ca5247e85cd7(device=<unavailable>, quirks_mode=Quirks) at stylist.rs:562:26 frame #21: 0x00007ffff593d755 libxul.so`style::gecko::data::PerDocumentStyleData::new::h9dc814d46fec8d6c(document=<unavailable>) at data.rs:145:22 frame #22: 0x00007ffff56781d9 libxul.so`Servo_StyleSet_Init(doc=<unavailable>) at glue.rs:4175:25 frame #23: 0x00007ffff2b58416 libxul.so`mozilla::ServoStyleSet::ServoStyleSet(this=0x00007fffe3c5ba90, aDocument=0x00007fffd391d560) at ServoStyleSet.cpp:120:17 frame #24: 0x00007ffff128ba5e libxul.so`mozilla::dom::Document::Init() [inlined] mozilla::detail::UniqueSelector<mozilla::ServoStyleSet>::SingleObject mozilla::MakeUnique<mozilla::ServoStyleSet, mozilla::dom::Document&>(aArgs=0x00007fffd391d560) at UniquePtr.h:609:27 frame #25: 0x00007ffff128ba46 libxul.so`mozilla::dom::Document::Init(this=0x00007fffd391d560) at Document.cpp:2657:15 frame #26: 0x00007ffff20847d9 libxul.so`nsHTMLDocument::Init(this=0x00007fffd391d560) at nsHTMLDocument.cpp:146:27 frame #27: 0x00007ffff208462a libxul.so`NS_NewHTMLDocument(aInstancePtrResult=0x00007fffffff9c60, aLoadedAsData=false) at nsHTMLDocument.cpp:112:22 frame #28: 0x00007ffff2ea18cf libxul.so`nsContentDLF::CreateBlankDocument(aLoadGroup=0x00007fffc2ec87a0, aPrincipal=0x00007fffe476cdb0, aPartitionedPrincipal=0x00007fffe476cdb0, aContainer=0x00007fffe34752c0) at nsContentDLF.cpp:212:22 frame #29: 0x00007ffff32d1ea1 libxul.so`nsDocShell::CreateAboutBlankContentViewer(this=0x00007fffe34752c0, aPrincipal=0x00007fffe476cdb0, aPartitionedPrincipal=0x00007fffe476cdb0, aCSP=0x0000000000000000, aBaseURI=0x0000000000000000, aIsInitialDocument=true, aCOEP=0x00007fffffff9d86, aTryToSaveOldPresentation=<unavailable>, aCheckPermitUnload=<unavailable>, aActor=0x0000000000000000) at nsDocShell.cpp:6588:16 frame #30: 0x00007ffff332380c libxul.so`nsAppShellService::JustCreateTopWindow(this=<unavailable>, aParent=0x0000000000000000, aUrl=<unavailable>, aChromeMask=4161799686, aInitialWidth=<unavailable>, aInitialHeight=<unavailable>, aIsHiddenWindow=<unavailable>, aResult=<unavailable>) at nsAppShellService.cpp:760:22 frame #31: 0x00007ffff3323b03 libxul.so`nsAppShellService::CreateTopLevelWindow(this=<unavailable>, aParent=0x0000000000000000, aUrl=<unavailable>, aChromeMask=4161799686, aInitialWidth=<unavailable>, aInitialHeight=<unavailable>, aResult=<unavailable>) at nsAppShellService.cpp:173:8 frame #32: 0x00007ffff35aad11 libxul.so`nsAppStartup::CreateChromeWindow(this=<unavailable>, aParent=<unavailable>, aChromeFlags=4161799686, aOpenWindowInfo=0x0000000000000000, aCancel=<unavailable>, _retval=0x00007fffffff9ef8) at nsAppStartup.cpp:750:15 frame #33: 0x00007ffff3627118 libxul.so`nsWindowWatcher::CreateChromeWindow(this=<unavailable>, aParentChrome=<unavailable>, aChromeFlags=<unavailable>, aOpenWindowInfo=<unavailable>, aResult=0x00007fffffff9fd0) at nsWindowWatcher.cpp:419:33 frame #34: 0x00007ffff3626ae6 libxul.so`nsWindowWatcher::OpenWindowInternal(this=<unavailable>, aParent=0x0000000000000000, aUrl=0x00007fffffffa2d8, aName=0x00007fffffffa288, aFeatures=0x00007fffffffa278, aCalledFromJS=<unavailable>, aDialog=<unavailable>, aNavigate=<unavailable>, aArgv=<unavailable>, aIsPopupSpam=<unavailable>, aForceNoOpener=<unavailable>, aForceNoReferrer=<unavailable>, aPrintKind=<unavailable>, aLoadState=<unavailable>, aResult=<unavailable>) at nsWindowWatcher.cpp:947:12 frame #35: 0x00007ffff3624d83 libxul.so`nsWindowWatcher::OpenWindow(this=0x00007fffe3f1bbe0, aParent=0x0000000000000000, aUrl=0x00007fffffffa2d8, aName=0x00007fffffffa288, aFeatures=0x00007fffffffa278, aArguments=<unavailable>, aResult=<unavailable>) at nsWindowWatcher.cpp:293:3 frame #36: 0x00007ffff365c15b libxul.so`ShowProfileManager(aProfileSvc=<unavailable>, aNative=0x00007fffe8ce8ec0) at nsAppRunner.cpp:2553:27 frame #37: 0x00007ffff365ad8f libxul.so`XREMain::XRE_mainStartup(bool*) [inlined] SelectProfile(aProfileSvc=<unavailable>, aNative=<unavailable>, aRootDir=<unavailable>, aLocalDir=<unavailable>, aProfile=<unavailable>, aWasDefaultSelection=<unavailable>) at nsAppRunner.cpp:0:7 frame #38: 0x00007ffff365ab56 libxul.so`XREMain::XRE_mainStartup(this=<unavailable>, aExitFlag=<unavailable>) at nsAppRunner.cpp:4501:8 frame #39: 0x00007ffff365fd00 libxul.so`XREMain::XRE_main(this=0x00007fffffffa500, argc=2, argv=0x00007fffffffb6f8, aConfig=0x00007fffffffa690) at nsAppRunner.cpp:5465:12 frame #40: 0x00007ffff3660175 libxul.so`XRE_main(argc=<unavailable>, argv=<unavailable>, aConfig=<unavailable>) at nsAppRunner.cpp:5536:21 frame #41: 0x00007ffff3665ff1 libxul.so`mozilla::BootstrapImpl::XRE_main(this=<unavailable>, argc=<unavailable>, argv=<unavailable>, aConfig=<unavailable>) at Bootstrap.cpp:45:12 frame #42: 0x0000555555579140 firefox`main [inlined] do_main(argc=<unavailable>, argv=0x00007fffffffb6f8, envp=<unavailable>) at nsBrowserApp.cpp:225:22 frame #43: 0x0000555555579076 firefox`main(argc=<unavailable>, argv=<unavailable>, envp=<unavailable>) at nsBrowserApp.cpp:392:16 ``` </details> --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: --> - [ ] `./mach build -d` does not report any errors - [ ] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable) <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___
Found by miri:
Vec::into_iter
callsptr::read
(and the underlyingcopy_nonoverlapping
) with an unaligned pointer to a ZST. According to LLVM devs, this is UB because it contradicts the metadata we are attaching to that pointer.HashMap
creation callsptr:.write_bytes
on a NULL pointer with a count of 0. This is likely not currently UB currently, but it violates the rules we are setting in Rewrite docs for pointer methods #53783, and we might want to exploit those rules later (e.g. with morenonnull
attributes for LLVM).Probably what
HashMap
really should do is useNonNull::dangling()
instead of 0 for the empty case, but that would require a more careful analysis of the code.It seems like ideally, we should do a review of usage of such intrinsics all over libstd to ensure that they use valid pointers even when the size is 0. Is it worth opening an issue for that?