UB-check for alignment of ptr to Box::from_raw{,_in} #137325

hkBst · 2025-02-20T17:13:06Z

This adds a missing UB-check for proper alignment of pointers to Box::from_raw{,_in}. See #69283 (comment) for some background.

rustbot · 2025-02-20T17:13:17Z

r? @joboet

rustbot has assigned @joboet.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

saethlin · 2025-02-20T17:36:03Z

library/alloc/src/boxed.rs

+            "Box::from_raw_in requires that the pointer is properly aligned",
+            (ptr: *mut () = ptr as *mut (), align: usize = align_of::<T>()) => ptr.is_aligned_to(align)
+        );
+        Box(unsafe { Unique::new_unchecked(ptr) }, alloc)


Just in terms of UX, it's better that all preconditions of an unsafe fn be asserted in it directly, instead of falling through to callees.

So this should also check for non-null-ness? What about Box::from_raw (which calls Box::from_raw_in)? Should it get its own checks as well?

So this should also check for non-null-ness?

Yes

What about Box::from_raw (which calls Box::from_raw_in)? Should it get its own checks as well?

These checks sometimes impose small but widespread compile time overhead. What I'd like is for the diagnostic to be clear and actionable, and reporting the name of the function that the user called seems like a pretty basic first step. So yes I would prefer for Box::from_raw to also have basically a copy+paste of this check, but we may have to balance that against compile-time overhead. I can kick off a perf run to measure once you and I think this PR is in good order and it passes CI.

Sounds like a plan :D

saethlin · 2025-02-20T17:38:18Z

Can you add a test to tests/ui/precondition-checks/ that covers this? You should be able to crib from https://github.com/rust-lang/rust/blob/28b83ee59698ae069f5355b8e03f976406f410f5/tests/ui/precondition-checks/slice-from-raw-parts.rs for example. These test should at least validate that when any one of the preconditions of an unsafe fn is violated that we detect it and mention the right function.

hkBst · 2025-02-20T19:08:04Z

Can you add a test to tests/ui/precondition-checks/ that covers this? You should be able to crib from https://github.com/rust-lang/rust/blob/28b83ee59698ae069f5355b8e03f976406f410f5/tests/ui/precondition-checks/slice-from-raw-parts.rs for example. These test should at least validate that when any one of the preconditions of an unsafe fn is violated that we detect it and mention the right function.

Yes can do, and thanks for the pointer.

rustbot · 2025-02-23T13:52:37Z

Some changes occurred to the intrinsics. Make sure the CTFE / Miri interpreter
gets adapted for the changes, if necessary.

cc @rust-lang/miri, @rust-lang/wg-const-eval

RalfJung · 2025-02-23T14:54:24Z

library/alloc/src/boxed.rs

+                @capture { ptr: *const (), align: usize } -> bool:
+                if const {
+                    !ptr.is_null()
+                } else {
+                    ptr.is_aligned_to(align) && !ptr.is_null()
+                }
+            )


We have maybe_is_aligned_and_not_null as a helper for this, why not use that?

I can't use that, because impl<T: ?Sized> Box<T> means that I cannot use T::IS_ZST, which means I don't know how to call maybe_is_aligned_and_not_null.

RalfJung · 2025-02-23T14:55:40Z

Oh, seems you are still experimenting. Please make this a draft so rustbot doesn't ping people.

hkBst · 2025-02-23T15:15:09Z

Oh, seems you are still experimenting. Please make this a draft so rustbot doesn't ping people.

Sorry about that; I'm still finding my way around, but this is now a draft again.

rust-log-analyzer · 2025-02-23T17:42:05Z

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

#21 exporting to docker image format
#21 sending tarball 28.2s done
#21 DONE 40.7s
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
debug: `DISABLE_CI_RUSTC_IF_INCOMPATIBLE` configured.
---
sccache: Listening on address 127.0.0.1:4226
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
   Compiling unwind v0.0.0 (/checkout/library/unwind)
error[E0658]: use of unstable library feature `pointer_is_aligned_to`
    --> library/alloc/src/boxed.rs:1061:25
     |
1061 |                     ptr.is_aligned_to(align) && !ptr.is_null()
     |
     = note: see issue #96284 <https://github.com/rust-lang/rust/issues/96284> for more information
     = note: see issue #96284 <https://github.com/rust-lang/rust/issues/96284> for more information
     = help: add `#![feature(pointer_is_aligned_to)]` to the crate attributes to enable


error: const function that might be (indirectly) exposed to stable cannot use `#[feature(const_eval_select)]`
     |
3835 |   pub macro const_eval_select {
     |   ---------------------------
     |   |
     |   |
     |   in this expansion of `core::intrinsics::const_eval_select!` (#1)
     |   in this expansion of `$crate::intrinsics::const_eval_select!` (#2)
     |   in this expansion of `$crate::intrinsics::const_eval_select!` (#3)
...
3844 | /         $crate::intrinsics::const_eval_select!(
3845 | |             @capture$([$($binders)*])? { $($arg : $ty = $val),* } $(-> $ret)? :
3846 | |             #[noinline]
3847 | |             if const
3854 | |                 $runtime
3855 | |         )
     | |_________- in this macro invocation (#3)
...
...
3879 |           const_eval_select(($($val,)*), compiletime, runtime)
...
3890 | /         $crate::intrinsics::const_eval_select!(
3890 | /         $crate::intrinsics::const_eval_select!(
3891 | |             @capture$([$($binders)*])? { $($arg : $ty = $arg),* } $(-> $ret)? :
3892 | |             if const
3893 | |                 $(#[$compiletime_attr])* $compiletime
3894 | |             else
3895 | |                 $(#[$runtime_attr])* $runtime
     | |_________- in this macro invocation (#2)
     |
    ::: library/alloc/src/boxed.rs:1056:18
     |
     |
1056 |               ) => core::intrinsics::const_eval_select!(
     |  __________________-
1057 | |                 @capture { ptr: *const (), align: usize } -> bool:
1058 | |                 if const {
1059 | |                     !ptr.is_null()
1063 | |             )
     | |_____________- in this macro invocation (#1)
     |
     |
help: if the function is not (yet) meant to be exposed to stable, add `#[rustc_const_unstable]` (this is what you probably want to do)
    -->  /checkout/library/core/src/ub_checks.rs:66:13
66   +             #[rustc_const_unstable(feature = "...", issue = "...")]
66   +             #[rustc_const_unstable(feature = "...", issue = "...")]
67   |             const fn precondition_check($($name:$ty),*) {
     |
help: otherwise, as a last resort `#[rustc_allow_const_fn_unstable]` can be used to bypass stability checks (this requires team approval)
    -->  /checkout/library/core/src/ub_checks.rs:66:13
66   +             #[rustc_allow_const_fn_unstable(const_eval_select)]
66   +             #[rustc_allow_const_fn_unstable(const_eval_select)]
67   |             const fn precondition_check($($name:$ty),*) {

For more information about this error, try `rustc --explain E0658`.
error: could not compile `alloc` (lib) due to 2 previous errors
warning: build failed, waiting for other jobs to finish...

bors · 2025-02-25T06:41:17Z

☔ The latest upstream changes (presumably #137573) made this pull request unmergeable. Please resolve the merge conflicts.

rustbot assigned joboet Feb 20, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Feb 20, 2025