Skip to content

Support linting c-like fn calls that take pointer-to-slice and length #148664

@estebank

Description

@estebank

Given code like:

use std::env; use std::process; fn main() { let args: Vec<String> = env::args().collect(); if args.len() != 3 { eprintln!("Usage: {} <string> <length>", args[0]); process::exit(1); } let buffer = &args[1]; let Ok(length) = args[2].parse::<usize>() else { eprintln!("Error: length must be a valid integer"); process::exit(1); }; unsafe { libc::write(1, buffer.as_ptr() as *const libc::c_void, length); } }

you can cause undefined behavior. It does get caught by miri:

error: Undefined Behavior: memory access failed: attempting to access 1000 bytes, but got alloc1 which is only 4 bytes from the end of the allocation --> src/main.rs:21:9 | 21 | libc::write(1, buffer.as_ptr() as *const libc::c_void, length); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Undefined Behavior occurred here | = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information 

But we could leverage the #[diagnostic::*] namespace to annotate functions that have this kind of C API with independent pointer and length, and then lint if the length argument isn't directly derived from buffer. I would go as far as making the lint simple in its analysis of that and check that length is either buffer.length() or std::cmp::max(buffer.len(), length) and trigger otherwise (and ask people to enable it for anything more complex, at least at the start?).

The annotation would likely look something like:

#[diagnostic::c_buffer_length(arg2, arg3)] pub fn write(arg1: c_int, arg2: *const c_void, arg3: c_int) -> c_int;

and the output be something like

error: when calling `write`, `length` must be derived from `buffer` to avoid accidental out-of-bounds access --> src/main.rs:21:9 | LL | let buffer = &args[1]; | ------ `buffer` created here LL | let Ok(length) = args[2].parse::<usize>() else { | ------ `length` created here, unrelated to `buffer` .. LL | libc::write(1, buffer.as_ptr() as *const libc::c_void, length); | --------------- ^^^^^^ must be related to `buffer` | | | a pointer to `buffer` was captured here | = help: this can cause a bug in the program: `write` could perform an invalid operation, and cause Undefined Behavior = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information help: consider using the length of the buffer being passed in directly | LL - libc::write(1, buffer.as_ptr() as *const libc::c_void, length); LL + libc::write(1, buffer.as_ptr() as *const libc::c_void, buffer.length()); | help: alternatively, if you desire to constrain the buffer up to `length`, then ensure that you account for `length` being longer than the buffer | LL | libc::write(1, buffer.as_ptr() as *const libc::c_void, std::cmp::max(length, buffer.length())); | ++++++++++++++ ++++++++++++++++++ | 

This lint would is not a reason not to use miri, just a way to catch a possible mistake earlier.

Inspired by https://nitter.net/filpizlo/status/1984366437390303265

Metadata

Metadata

Assignees

No one assigned

    Labels

    A-diagnosticsArea: Messages for errors, warnings, and lintsA-lintsArea: Lints (warnings about flaws in source code) such as unused_mut.D-papercutDiagnostics: An error or lint that needs small tweaks.T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions