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

Add diagnostic for stack allocations of 1 GB or more #119798

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open
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
2 changes: 2 additions & 0 deletions compiler/rustc_codegen_ssa/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ codegen_ssa_copy_path_buf = unable to copy {$source_file} to {$output_path}: {$e

codegen_ssa_create_temp_dir = couldn't create a temp dir: {$error}

codegen_ssa_dangerous_stack_allocation = dangerous stack allocation of size: {$output} exceeds most system architecture limits

codegen_ssa_dlltool_fail_import_library =
Dlltool could not create import library with {$dlltool_path} {$dlltool_args}:
{$stdout}
Expand Down
8 changes: 8 additions & 0 deletions compiler/rustc_codegen_ssa/src/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,14 @@ pub(crate) struct CheckInstalledVisualStudio;
#[diag(codegen_ssa_insufficient_vs_code_product)]
pub(crate) struct InsufficientVSCodeProduct;

#[derive(Diagnostic)]
#[diag(codegen_ssa_dangerous_stack_allocation)]
pub struct DangerousStackAllocation {
#[primary_span]
pub span: Span,
pub output: String,
}
Comment on lines +432 to +438
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the ftl change are no longer used, right?


#[derive(Diagnostic)]
#[diag(codegen_ssa_processing_dymutil_failed)]
#[note]
Expand Down
27 changes: 27 additions & 0 deletions compiler/rustc_codegen_ssa/src/mir/mod.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
use std::iter;

use rustc_hir::CRATE_HIR_ID;
use rustc_index::IndexVec;
use rustc_index::bit_set::BitSet;
use rustc_middle::middle::codegen_fn_attrs::CodegenFnAttrFlags;
use rustc_middle::mir::{UnwindTerminateReason, traversal};
use rustc_middle::ty::layout::{FnAbiOf, HasTyCtxt, TyAndLayout};
use rustc_middle::ty::{self, Instance, Ty, TyCtxt, TypeFoldable, TypeVisitableExt};
use rustc_middle::{bug, mir, span_bug};
use rustc_session::lint;
use rustc_target::abi::call::{FnAbi, PassMode};
use tracing::{debug, instrument};

Expand All @@ -29,6 +31,8 @@ use self::debuginfo::{FunctionDebugContext, PerLocalVarDebugInfo};
use self::operand::{OperandRef, OperandValue};
use self::place::PlaceRef;

const MIN_DANGEROUS_SIZE: u64 = 1024 * 1024 * 1024 * 1; // 1 GB
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const MIN_DANGEROUS_SIZE: u64 = 1024 * 1024 * 1024 * 1; // 1 GB
const MIN_DANGEROUS_ALLOC_SIZE: u64 = 1024 * 1024 * 1024 * 1; // 1 GB

Copy link
Contributor

Choose a reason for hiding this comment

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

Not to change, but to discuss: would making this lower than 1GB be worth it, so that people that we can guide people that are not hitting the limits, but are on their way to do so get some early warning?


// Used for tracking the state of generated basic blocks.
enum CachedLlbb<T> {
/// Nothing created yet.
Expand Down Expand Up @@ -234,6 +238,21 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
let layout = start_bx.layout_of(fx.monomorphize(decl.ty));
assert!(!layout.ty.has_erasable_regions());

if layout.size.bytes() >= MIN_DANGEROUS_SIZE {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if layout.size.bytes() >= MIN_DANGEROUS_SIZE {
if layout.size.bytes() >= MIN_DANGEROUS_ALLOC_SIZE {

let (size_quantity, size_unit) = human_readable_bytes(layout.size.bytes());
cx.tcx().node_span_lint(
lint::builtin::DANGEROUS_STACK_ALLOCATION,
CRATE_HIR_ID,
decl.source_info.span,
|lint| {
Copy link
Contributor

Choose a reason for hiding this comment

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

We could add more context, like in the lint description, about some common platform limits.

lint.primary_message(format!(
"allocation of size: {:.2} {} exceeds most system architecture limits",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"allocation of size: {:.2} {} exceeds most system architecture limits",
"allocation of {:.2} {} exceeds most system architecture limits",

size_quantity, size_unit
));
},
);
}

if local == mir::RETURN_PLACE {
match fx.fn_abi.ret.mode {
PassMode::Indirect { .. } => {
Expand Down Expand Up @@ -299,6 +318,14 @@ pub fn codegen_mir<'a, 'tcx, Bx: BuilderMethods<'a, 'tcx>>(
}
}

/// Formats a number of bytes into a human readable SI-prefixed size.
/// Returns a tuple of `(quantity, units)`.
pub fn human_readable_bytes(bytes: u64) -> (u64, &'static str) {
static UNITS: [&str; 7] = ["B", "KiB", "MiB", "GiB", "TiB", "PiB", "EiB"];
let i = ((bytes.checked_ilog2().unwrap_or(0) / 10) as usize).min(UNITS.len() - 1);
(bytes >> (10 * i), UNITS[i])
}
Comment on lines +321 to +327
Copy link
Contributor

Choose a reason for hiding this comment

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

Could have sworn that we did this already, but alas it is in cargo 😅


/// Produces, for each argument, a `Value` pointing at the
/// argument's value. As arguments are places, these are always
/// indirect.
Expand Down
45 changes: 45 additions & 0 deletions compiler/rustc_lint_defs/src/builtin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ declare_lint_pass! {
CONFLICTING_REPR_HINTS,
CONST_EVALUATABLE_UNCHECKED,
CONST_ITEM_MUTATION,
DANGEROUS_STACK_ALLOCATION,
DEAD_CODE,
DEPENDENCY_ON_UNIT_NEVER_TYPE_FALLBACK,
DEPRECATED,
Expand Down Expand Up @@ -709,6 +710,50 @@ declare_lint! {
"detect assignments that will never be read"
}

declare_lint! {
/// The `dangerous_stack_allocation` lint detects stack allocations that
/// are 1 GB or more.
///
/// ### Example
///
/// ``` rust
/// fn func() {
/// const CAP: usize = std::u32::MAX as usize;
/// let mut x: [u8; CAP>>1] = [0; CAP>>1];
/// x[2] = 123;
/// println!("{}", x[2]);
/// }
///
/// fn main() {
/// std::thread::Builder::new()
/// .stack_size(3 * 1024 * 1024 * 1024)
/// .spawn(func)
/// .unwrap()
/// .join()
/// .unwrap();
/// }
/// ```
///
/// {{produces}}
///
/// ```text
/// warning: allocation of size: 1 GiB exceeds most system architecture limits
/// --> $DIR/large-stack-size-issue-83060.rs:7:9
/// |
/// LL | let mut x: [u8; CAP>>1] = [0; CAP>>1];
/// | ^^^^^
/// |
/// = note: `#[warn(dangerous_stack_allocation)]` on by default
Comment on lines +740 to +746
Copy link
Contributor

Choose a reason for hiding this comment

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

This will likely fail because of the textual formatting not being correctly left aligned.

Comment on lines +740 to +746
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// warning: allocation of size: 1 GiB exceeds most system architecture limits
/// --> $DIR/large-stack-size-issue-83060.rs:7:9
/// |
/// LL | let mut x: [u8; CAP>>1] = [0; CAP>>1];
/// | ^^^^^
/// |
/// = note: `#[warn(dangerous_stack_allocation)]` on by default
/// warning: allocation of size: 1 GiB exceeds most system architecture limits
/// --> $DIR/large-stack-size-issue-83060.rs:7:9
/// |
/// LL | let mut x: [u8; CAP>>1] = [0; CAP>>1];
/// | ^^^^^
/// |
/// = note: `#[warn(dangerous_stack_allocation)]` on by default

/// ```
/// ### Explanation
///
/// Large arras may cause stack overflow due to the limited size of the
/// stack on most platforms.
Comment on lines +750 to +751
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Large arras may cause stack overflow due to the limited size of the
/// stack on most platforms.
/// Large arrays may cause stack overflow due to the limited size of the
/// stack on most platforms.

We should probably extend this to be more explicit about which platforms this is an issue in (no need to be exhaustive).

pub DANGEROUS_STACK_ALLOCATION,
Warn,
"Detects dangerous stack allocations at the limit of most architectures"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Detects dangerous stack allocations at the limit of most architectures"
"detects dangerously large stack allocations at the limit of most architectures"

}

declare_lint! {
/// The `dead_code` lint detects unused, unexported items.
///
Expand Down
21 changes: 21 additions & 0 deletions tests/ui/sanitizer/large-stack-size-issue-83060.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
// This test checks that allocating a stack size of 1GB or more results in a warning
//@build-pass
//@ only-64bit

fn func() {
const CAP: usize = std::u32::MAX as usize;
let mut x: [u8; CAP>>1] = [0; CAP>>1];
//~^ warning: allocation of size: 1 GiB exceeds most system architecture limits
//~| NOTE on by default
x[2] = 123;
println!("{}", x[2]);
}

fn main() {
std::thread::Builder::new()
.stack_size(3 * 1024 * 1024 * 1024)
.spawn(func)
.unwrap()
.join()
.unwrap();
}
10 changes: 10 additions & 0 deletions tests/ui/sanitizer/large-stack-size-issue-83060.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
warning: allocation of size: 1 GiB exceeds most system architecture limits
--> $DIR/large-stack-size-issue-83060.rs:7:9
|
LL | let mut x: [u8; CAP>>1] = [0; CAP>>1];
| ^^^^^
|
= note: `#[warn(dangerous_stack_allocation)]` on by default

warning: 1 warning emitted

Loading