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

Taking a raw ref (&raw (const|mut)) of a deref of pointer (*ptr) is always safe #129248

Merged
merged 2 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 17 additions & 9 deletions compiler/rustc_lint/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2657,8 +2657,8 @@ declare_lint! {
///
/// ### Explanation
///
/// Dereferencing a null pointer causes [undefined behavior] even as a place expression,
/// like `&*(0 as *const i32)` or `addr_of!(*(0 as *const i32))`.
/// Dereferencing a null pointer causes [undefined behavior] if it is accessed
/// (loaded from or stored to).
///
/// [undefined behavior]: https://doc.rust-lang.org/reference/behavior-considered-undefined.html
pub DEREF_NULLPTR,
Expand All @@ -2673,14 +2673,14 @@ impl<'tcx> LateLintPass<'tcx> for DerefNullPtr {
/// test if expression is a null ptr
fn is_null_ptr(cx: &LateContext<'_>, expr: &hir::Expr<'_>) -> bool {
match &expr.kind {
rustc_hir::ExprKind::Cast(expr, ty) => {
if let rustc_hir::TyKind::Ptr(_) = ty.kind {
hir::ExprKind::Cast(expr, ty) => {
if let hir::TyKind::Ptr(_) = ty.kind {
return is_zero(expr) || is_null_ptr(cx, expr);
}
}
// check for call to `core::ptr::null` or `core::ptr::null_mut`
rustc_hir::ExprKind::Call(path, _) => {
if let rustc_hir::ExprKind::Path(ref qpath) = path.kind {
hir::ExprKind::Call(path, _) => {
if let hir::ExprKind::Path(ref qpath) = path.kind {
if let Some(def_id) = cx.qpath_res(qpath, path.hir_id).opt_def_id() {
return matches!(
cx.tcx.get_diagnostic_name(def_id),
Expand All @@ -2697,7 +2697,7 @@ impl<'tcx> LateLintPass<'tcx> for DerefNullPtr {
/// test if expression is the literal `0`
fn is_zero(expr: &hir::Expr<'_>) -> bool {
match &expr.kind {
rustc_hir::ExprKind::Lit(lit) => {
hir::ExprKind::Lit(lit) => {
if let LitKind::Int(a, _) = lit.node {
return a == 0;
}
Expand All @@ -2707,8 +2707,16 @@ impl<'tcx> LateLintPass<'tcx> for DerefNullPtr {
false
}

if let rustc_hir::ExprKind::Unary(rustc_hir::UnOp::Deref, expr_deref) = expr.kind {
if is_null_ptr(cx, expr_deref) {
if let hir::ExprKind::Unary(hir::UnOp::Deref, expr_deref) = expr.kind
&& is_null_ptr(cx, expr_deref)
{
if let hir::Node::Expr(hir::Expr {
kind: hir::ExprKind::AddrOf(hir::BorrowKind::Raw, ..),
..
}) = cx.tcx.parent_hir_node(expr.hir_id)
{
// `&raw *NULL` is ok.
} else {
cx.emit_span_lint(DEREF_NULLPTR, expr.span, BuiltinDerefNullptr {
label: expr.span,
});
Expand Down
14 changes: 3 additions & 11 deletions compiler/rustc_mir_build/src/check_unsafety.rs
Original file line number Diff line number Diff line change
Expand Up @@ -509,20 +509,12 @@ impl<'a, 'tcx> Visitor<'a, 'tcx> for UnsafetyVisitor<'a, 'tcx> {
}
ExprKind::RawBorrow { arg, .. } => {
if let ExprKind::Scope { value: arg, .. } = self.thir[arg].kind
// THIR desugars UNSAFE_STATIC into *UNSAFE_STATIC_REF, where
// UNSAFE_STATIC_REF holds the addr of the UNSAFE_STATIC, so: take two steps
&& let ExprKind::Deref { arg } = self.thir[arg].kind
// FIXME(workingjubiee): we lack a clear reason to reject ThreadLocalRef here,
// but we also have no conclusive reason to allow it either!
&& let ExprKind::StaticRef { .. } = self.thir[arg].kind
{
// A raw ref to a place expr, even an "unsafe static", is okay!
// We short-circuit to not recursively traverse this expression.
// Taking a raw ref to a deref place expr is always safe.
// Make sure the expression we're deref'ing is safe, though.
visit::walk_expr(self, &self.thir[arg]);
return;
// note: const_mut_refs enables this code, and it currently remains unsafe:
// static mut BYTE: u8 = 0;
// static mut BYTE_PTR: *mut u8 = unsafe { addr_of_mut!(BYTE) };
// static mut DEREF_BYTE_PTR: *mut u8 = unsafe { addr_of_mut!(*BYTE_PTR) };
}
}
ExprKind::Deref { arg } => {
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/lint/lint-deref-nullptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,9 @@ fn f() {
let ub = &*ptr::null_mut::<i32>();
//~^ ERROR dereferencing a null pointer
ptr::addr_of!(*ptr::null::<i32>());
//~^ ERROR dereferencing a null pointer
// ^^ OKAY
ptr::addr_of_mut!(*ptr::null_mut::<i32>());
//~^ ERROR dereferencing a null pointer
// ^^ OKAY
let offset = ptr::addr_of!((*ptr::null::<Struct>()).field);
//~^ ERROR dereferencing a null pointer
}
Expand Down
14 changes: 1 addition & 13 deletions tests/ui/lint/lint-deref-nullptr.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -46,23 +46,11 @@ error: dereferencing a null pointer
LL | let ub = &*ptr::null_mut::<i32>();
| ^^^^^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed

error: dereferencing a null pointer
--> $DIR/lint-deref-nullptr.rs:29:23
|
LL | ptr::addr_of!(*ptr::null::<i32>());
| ^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed

error: dereferencing a null pointer
--> $DIR/lint-deref-nullptr.rs:31:27
|
LL | ptr::addr_of_mut!(*ptr::null_mut::<i32>());
| ^^^^^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed

error: dereferencing a null pointer
--> $DIR/lint-deref-nullptr.rs:33:36
|
LL | let offset = ptr::addr_of!((*ptr::null::<Struct>()).field);
| ^^^^^^^^^^^^^^^^^^^^^^^^ this code causes undefined behavior when executed

error: aborting due to 10 previous errors
error: aborting due to 8 previous errors

9 changes: 5 additions & 4 deletions tests/ui/static/raw-ref-deref-with-unsafe.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
//@ check-pass
use std::ptr;

// This code should remain unsafe because of the two unsafe operations here,
// even if in a hypothetical future we deem all &raw (const|mut) *ptr exprs safe.

static mut BYTE: u8 = 0;
static mut BYTE_PTR: *mut u8 = ptr::addr_of_mut!(BYTE);

// This code should remain unsafe because reading from a static mut is *always* unsafe.

// An unsafe static's ident is a place expression in its own right, so despite the above being safe
// (it's fine to create raw refs to places!) the following derefs the ptr before creating its ref
// (it's fine to create raw refs to places!) the following *reads* from the static mut place before
// derefing it explicitly with the `*` below.
static mut DEREF_BYTE_PTR: *mut u8 = unsafe { ptr::addr_of_mut!(*BYTE_PTR) };

fn main() {
Expand Down
7 changes: 3 additions & 4 deletions tests/ui/static/raw-ref-deref-without-unsafe.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
use std::ptr;

// This code should remain unsafe because of the two unsafe operations here,
// even if in a hypothetical future we deem all &raw (const|mut) *ptr exprs safe.

static mut BYTE: u8 = 0;
static mut BYTE_PTR: *mut u8 = ptr::addr_of_mut!(BYTE);

// This code should remain unsafe because reading from a static mut is *always* unsafe.

// An unsafe static's ident is a place expression in its own right, so despite the above being safe
// (it's fine to create raw refs to places!) the following derefs the ptr before creating its ref!
static mut DEREF_BYTE_PTR: *mut u8 = ptr::addr_of_mut!(*BYTE_PTR);
//~^ ERROR: use of mutable static
//~| ERROR: dereference of raw pointer

fn main() {
let _ = unsafe { DEREF_BYTE_PTR };
Expand Down
10 changes: 1 addition & 9 deletions tests/ui/static/raw-ref-deref-without-unsafe.stderr
Original file line number Diff line number Diff line change
@@ -1,11 +1,3 @@
error[E0133]: dereference of raw pointer is unsafe and requires unsafe function or block
--> $DIR/raw-ref-deref-without-unsafe.rs:10:56
|
LL | static mut DEREF_BYTE_PTR: *mut u8 = ptr::addr_of_mut!(*BYTE_PTR);
| ^^^^^^^^^ dereference of raw pointer
|
= note: raw pointers may be null, dangling or unaligned; they can violate aliasing rules and cause data races: all of these are undefined behavior

error[E0133]: use of mutable static is unsafe and requires unsafe function or block
--> $DIR/raw-ref-deref-without-unsafe.rs:10:57
|
Expand All @@ -14,6 +6,6 @@ LL | static mut DEREF_BYTE_PTR: *mut u8 = ptr::addr_of_mut!(*BYTE_PTR);
|
= note: mutable statics can be mutated by multiple threads: aliasing violations or data races will cause undefined behavior

error: aborting due to 2 previous errors
error: aborting due to 1 previous error

For more information about this error, try `rustc --explain E0133`.
14 changes: 14 additions & 0 deletions tests/ui/unsafe/place-expr-safe.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
//@ check-pass

fn main() {
let ptr = std::ptr::null_mut::<i32>();
let addr = &raw const *ptr;

let local = 1;
let ptr = &local as *const i32;
let addr = &raw const *ptr;

let boxed = Box::new(1);
let ptr = &*boxed as *const i32;
let addr = &raw const *ptr;
}
Loading