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

patterns: reject raw pointers that are not just integers #116930

Merged
merged 5 commits into from
Nov 8, 2023
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
37 changes: 27 additions & 10 deletions compiler/rustc_const_eval/src/const_eval/valtrees.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,27 @@ pub(crate) fn const_to_valtree_inner<'tcx>(
Ok(ty::ValTree::Leaf(val.assert_int()))
}

// Raw pointers are not allowed in type level constants, as we cannot properly test them for
// equality at compile-time (see `ptr_guaranteed_cmp`).
ty::RawPtr(_) => {
// Not all raw pointers are allowed, as we cannot properly test them for
// equality at compile-time (see `ptr_guaranteed_cmp`).
// However we allow those that are just integers in disguise.
// (We could allow wide raw pointers where both sides are integers in the future,
// but for now we reject them.)
let Ok(val) = ecx.read_scalar(place) else {
return Err(ValTreeCreationError::Other);
};
// We are in the CTFE machine, so ptr-to-int casts will fail.
// This can only be `Ok` if `val` already is an integer.
let Ok(val) = val.try_to_int() else {
return Err(ValTreeCreationError::Other);
};
// It's just a ScalarInt!
Ok(ty::ValTree::Leaf(val))
}

// Technically we could allow function pointers (represented as `ty::Instance`), but this is not guaranteed to
// agree with runtime equality tests.
ty::FnPtr(_) | ty::RawPtr(_) => Err(ValTreeCreationError::NonSupportedType),
ty::FnPtr(_) => Err(ValTreeCreationError::NonSupportedType),

ty::Ref(_, _, _) => {
let Ok(derefd_place)= ecx.deref_pointer(place) else {
Expand Down Expand Up @@ -222,12 +238,14 @@ pub fn valtree_to_const_value<'tcx>(
assert!(valtree.unwrap_branch().is_empty());
mir::ConstValue::ZeroSized
}
ty::Bool | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Char => match valtree {
ty::ValTree::Leaf(scalar_int) => mir::ConstValue::Scalar(Scalar::Int(scalar_int)),
ty::ValTree::Branch(_) => bug!(
"ValTrees for Bool, Int, Uint, Float or Char should have the form ValTree::Leaf"
),
},
ty::Bool | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Char | ty::RawPtr(_) => {
match valtree {
ty::ValTree::Leaf(scalar_int) => mir::ConstValue::Scalar(Scalar::Int(scalar_int)),
ty::ValTree::Branch(_) => bug!(
"ValTrees for Bool, Int, Uint, Float, Char or RawPtr should have the form ValTree::Leaf"
),
}
}
ty::Ref(_, inner_ty, _) => {
let mut ecx = mk_eval_cx(tcx, DUMMY_SP, param_env, CanAccessStatics::No);
let imm = valtree_to_ref(&mut ecx, valtree, *inner_ty);
Expand Down Expand Up @@ -281,7 +299,6 @@ pub fn valtree_to_const_value<'tcx>(
| ty::Coroutine(..)
| ty::CoroutineWitness(..)
| ty::FnPtr(_)
| ty::RawPtr(_)
| ty::Str
| ty::Slice(_)
| ty::Dynamic(..) => bug!("no ValTree should have been created for type {:?}", ty.kind()),
Expand Down
7 changes: 4 additions & 3 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2217,15 +2217,16 @@ declare_lint! {
///
/// ### Explanation
///
/// Previous versions of Rust allowed function pointers and wide raw pointers in patterns.
/// Previous versions of Rust allowed function pointers and all raw pointers in patterns.
/// While these work in many cases as expected by users, it is possible that due to
/// optimizations pointers are "not equal to themselves" or pointers to different functions
/// compare as equal during runtime. This is because LLVM optimizations can deduplicate
/// functions if their bodies are the same, thus also making pointers to these functions point
/// to the same location. Additionally functions may get duplicated if they are instantiated
/// in different crates and not deduplicated again via LTO.
/// in different crates and not deduplicated again via LTO. Pointer identity for memory
/// created by `const` is similarly unreliable.
pub POINTER_STRUCTURAL_MATCH,
Allow,
Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't realize this lint is still allow-by-default oO. Seems high time we make it warn-by-default?

Warn,
"pointers are not structural-match",
@future_incompatible = FutureIncompatibleInfo {
reason: FutureIncompatibilityReason::FutureReleaseErrorDontReportInDeps,
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ mir_build_overlapping_range_endpoints = multiple patterns overlap on their endpo
mir_build_pattern_not_covered = refutable pattern in {$origin}
.pattern_ty = the matched value is of type `{$pattern_ty}`

mir_build_pointer_pattern = function pointers and unsized pointers in patterns behave unpredictably and should not be relied upon. See https://github.com/rust-lang/rust/issues/70861 for details.
mir_build_pointer_pattern = function pointers and raw pointers not derived from integers in patterns behave unpredictably and should not be relied upon. See https://github.com/rust-lang/rust/issues/70861 for details.

mir_build_privately_uninhabited = pattern `{$witness_1}` is currently uninhabited, but this variant contains private fields which may become inhabited in the future

Expand Down
61 changes: 31 additions & 30 deletions compiler/rustc_mir_build/src/thir/pattern/const_to_pat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,8 @@ impl<'tcx> ConstToPat<'tcx> {
});
debug!(?check_body_for_struct_match_violation, ?mir_structural_match_violation);

let have_valtree =
matches!(cv, mir::Const::Ty(c) if matches!(c.kind(), ty::ConstKind::Value(_)));
let inlined_const_as_pat = match cv {
mir::Const::Ty(c) => match c.kind() {
ty::ConstKind::Param(_)
Expand Down Expand Up @@ -209,16 +211,6 @@ impl<'tcx> ConstToPat<'tcx> {
} else if !self.saw_const_match_lint.get() {
if let Some(mir_structural_match_violation) = mir_structural_match_violation {
match non_sm_ty.kind() {
ty::RawPtr(pointee)
if pointee.ty.is_sized(self.tcx(), self.param_env) => {}
ty::FnPtr(..) | ty::RawPtr(..) => {
self.tcx().emit_spanned_lint(
lint::builtin::POINTER_STRUCTURAL_MATCH,
self.id,
self.span,
PointerPattern,
);
}
ty::Adt(..) if mir_structural_match_violation => {
self.tcx().emit_spanned_lint(
lint::builtin::INDIRECT_STRUCTURAL_MATCH,
Expand All @@ -236,19 +228,15 @@ impl<'tcx> ConstToPat<'tcx> {
}
}
}
} else if !self.saw_const_match_lint.get() {
match cv.ty().kind() {
ty::RawPtr(pointee) if pointee.ty.is_sized(self.tcx(), self.param_env) => {}
ty::FnPtr(..) | ty::RawPtr(..) => {
self.tcx().emit_spanned_lint(
lint::builtin::POINTER_STRUCTURAL_MATCH,
self.id,
self.span,
PointerPattern,
);
}
_ => {}
}
} else if !have_valtree && !self.saw_const_match_lint.get() {
// The only way valtree construction can fail without the structural match
// checker finding a violation is if there is a pointer somewhere.
self.tcx().emit_spanned_lint(
lint::builtin::POINTER_STRUCTURAL_MATCH,
self.id,
self.span,
PointerPattern,
);
}

// Always check for `PartialEq`, even if we emitted other lints. (But not if there were
Expand Down Expand Up @@ -389,11 +377,19 @@ impl<'tcx> ConstToPat<'tcx> {
subpatterns: self
.field_pats(cv.unwrap_branch().iter().copied().zip(fields.iter()))?,
},
ty::Adt(def, args) => PatKind::Leaf {
subpatterns: self.field_pats(cv.unwrap_branch().iter().copied().zip(
def.non_enum_variant().fields.iter().map(|field| field.ty(self.tcx(), args)),
))?,
},
ty::Adt(def, args) => {
assert!(!def.is_union()); // Valtree construction would never succeed for unions.
PatKind::Leaf {
subpatterns: self.field_pats(
cv.unwrap_branch().iter().copied().zip(
def.non_enum_variant()
.fields
.iter()
.map(|field| field.ty(self.tcx(), args)),
),
)?,
}
}
ty::Slice(elem_ty) => PatKind::Slice {
prefix: cv
.unwrap_branch()
Expand Down Expand Up @@ -480,10 +476,15 @@ impl<'tcx> ConstToPat<'tcx> {
}
}
},
ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) => {
ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::RawPtr(..) => {
// The raw pointers we see here have been "vetted" by valtree construction to be
// just integers, so we simply allow them.
PatKind::Constant { value: mir::Const::Ty(ty::Const::new_value(tcx, cv, ty)) }
}
ty::FnPtr(..) | ty::RawPtr(..) => unreachable!(),
ty::FnPtr(..) => {
// Valtree construction would never succeed for these, so this is unreachable.
unreachable!()
}
_ => {
let err = InvalidPattern { span, non_sm_ty: ty };
let e = tcx.sess.emit_err(err);
Expand Down
1 change: 1 addition & 0 deletions library/core/src/marker.rs
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,7 @@ marker_impls! {
///
/// const CFN: Wrap<fn(&())> = Wrap(higher_order);
///
/// #[allow(pointer_structural_match)]
Copy link
Member Author

Choose a reason for hiding this comment

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

FWIW these entire trait docs are outdated; that's tracked in #115881.

/// fn main() {
/// match CFN {
/// CFN => {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ pub fn edge_case_str(event: String) {
pub fn edge_case_raw_ptr(event: *const i32) {
let _ = || {
match event {
NUMBER_POINTER => (),
NUMBER_POINTER => (), //~WARN behave unpredictably
//~| previously accepted
_ => (),
};
};
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
warning: function pointers and raw pointers not derived from integers in patterns behave unpredictably and should not be relied upon. See https://github.com/rust-lang/rust/issues/70861 for details.
--> $DIR/match-edge-cases_1.rs:29:13
|
LL | NUMBER_POINTER => (),
| ^^^^^^^^^^^^^^
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #62411 <https://github.com/rust-lang/rust/issues/70861>
= note: `#[warn(pointer_structural_match)]` on by default

warning: 1 warning emitted

Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#![deny(pointer_structural_match)]
#![allow(dead_code)]

const C: *const u8 = &0;
// Make sure we also find pointers nested in other types.
const C_INNER: (*const u8, u8) = (C, 0);

fn foo(x: *const u8) {
match x {
C => {} //~ERROR: behave unpredictably
//~| previously accepted
_ => {}
}
}

fn foo2(x: *const u8) {
match (x, 1) {
C_INNER => {} //~ERROR: behave unpredictably
//~| previously accepted
_ => {}
}
}

const D: *const [u8; 4] = b"abcd";

fn main() {
match D {
D => {} //~ERROR: behave unpredictably
//~| previously accepted
_ => {}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
error: function pointers and raw pointers not derived from integers in patterns behave unpredictably and should not be relied upon. See https://github.com/rust-lang/rust/issues/70861 for details.
--> $DIR/issue-34784-match-on-non-int-raw-ptr.rs:10:9
|
LL | C => {}
| ^
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #62411 <https://github.com/rust-lang/rust/issues/70861>
note: the lint level is defined here
--> $DIR/issue-34784-match-on-non-int-raw-ptr.rs:1:9
|
LL | #![deny(pointer_structural_match)]
| ^^^^^^^^^^^^^^^^^^^^^^^^

error: function pointers and raw pointers not derived from integers in patterns behave unpredictably and should not be relied upon. See https://github.com/rust-lang/rust/issues/70861 for details.
--> $DIR/issue-34784-match-on-non-int-raw-ptr.rs:18:9
|
LL | C_INNER => {}
| ^^^^^^^
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #62411 <https://github.com/rust-lang/rust/issues/70861>

error: function pointers and raw pointers not derived from integers in patterns behave unpredictably and should not be relied upon. See https://github.com/rust-lang/rust/issues/70861 for details.
--> $DIR/issue-34784-match-on-non-int-raw-ptr.rs:28:9
|
LL | D => {}
| ^
|
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #62411 <https://github.com/rust-lang/rust/issues/70861>

error: aborting due to 3 previous errors

4 changes: 2 additions & 2 deletions tests/ui/consts/const_in_pattern/issue-44333.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ const BAR: Func = bar;

fn main() {
match test(std::env::consts::ARCH.len()) {
FOO => println!("foo"), //~ WARN pointers in patterns behave unpredictably
FOO => println!("foo"), //~ WARN behave unpredictably
//~^ WARN will become a hard error
BAR => println!("bar"), //~ WARN pointers in patterns behave unpredictably
BAR => println!("bar"), //~ WARN behave unpredictably
//~^ WARN will become a hard error
_ => unreachable!(),
}
Expand Down
4 changes: 2 additions & 2 deletions tests/ui/consts/const_in_pattern/issue-44333.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
warning: function pointers and unsized pointers in patterns behave unpredictably and should not be relied upon. See https://github.com/rust-lang/rust/issues/70861 for details.
warning: function pointers and raw pointers not derived from integers in patterns behave unpredictably and should not be relied upon. See https://github.com/rust-lang/rust/issues/70861 for details.
--> $DIR/issue-44333.rs:19:9
|
LL | FOO => println!("foo"),
Expand All @@ -12,7 +12,7 @@ note: the lint level is defined here
LL | #![warn(pointer_structural_match)]
| ^^^^^^^^^^^^^^^^^^^^^^^^

warning: function pointers and unsized pointers in patterns behave unpredictably and should not be relied upon. See https://github.com/rust-lang/rust/issues/70861 for details.
warning: function pointers and raw pointers not derived from integers in patterns behave unpredictably and should not be relied upon. See https://github.com/rust-lang/rust/issues/70861 for details.
--> $DIR/issue-44333.rs:21:9
|
LL | BAR => println!("bar"),
Expand Down
21 changes: 0 additions & 21 deletions tests/ui/consts/issue-34784.rs

This file was deleted.

24 changes: 16 additions & 8 deletions tests/ui/pattern/usefulness/consts-opaque.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,10 @@ fn main() {
const QUUX: Quux = quux;

match QUUX {
QUUX => {}
QUUX => {}
QUUX => {} //~WARN behave unpredictably
//~| previously accepted
QUUX => {} //~WARN behave unpredictably
//~| previously accepted
_ => {}
}

Expand All @@ -105,14 +107,17 @@ fn main() {
const WRAPQUUX: Wrap<Quux> = Wrap(quux);

match WRAPQUUX {
WRAPQUUX => {}
WRAPQUUX => {}
WRAPQUUX => {} //~WARN behave unpredictably
//~| previously accepted
WRAPQUUX => {} //~WARN behave unpredictably
//~| previously accepted
Wrap(_) => {}
}

match WRAPQUUX {
Wrap(_) => {}
WRAPQUUX => {}
WRAPQUUX => {} //~WARN behave unpredictably
//~| previously accepted
}

match WRAPQUUX {
Expand All @@ -121,7 +126,8 @@ fn main() {

match WRAPQUUX {
//~^ ERROR: non-exhaustive patterns: `Wrap(_)` not covered
WRAPQUUX => {}
WRAPQUUX => {} //~WARN behave unpredictably
//~| previously accepted
}

#[derive(PartialEq, Eq)]
Expand All @@ -132,9 +138,11 @@ fn main() {
const WHOKNOWSQUUX: WhoKnows<Quux> = WhoKnows::Yay(quux);

match WHOKNOWSQUUX {
WHOKNOWSQUUX => {}
WHOKNOWSQUUX => {} //~WARN behave unpredictably
//~| previously accepted
WhoKnows::Yay(_) => {}
WHOKNOWSQUUX => {}
WHOKNOWSQUUX => {} //~WARN behave unpredictably
//~| previously accepted
WhoKnows::Nope => {}
}
}
Loading
Loading