Detect expressions where size_of::<T>() is an argument to ptr::copy/ptr::copy_nonoverlapping and T is the type of the pointee.

[thomcc]

What it does

What does this lint do?

Detects expressions where size_of::<T>() is used in the size argument to ptr::copy / ptr::copy_nonoverlapping / etc, where T is the type of the pointers passed in as arguments.

It should not fire if T is a different type, as it's common and correct to use this for byte copies.

Note:

This can happen because the docs for this function say:

copy_nonoverlapping is semantically equivalent to C's memcpy, but with the argument order swapped.

Which is slightly inaccurate, since memcpy takes the size in bytes despite accepting pointers of any type (in C, anyway — in C++ you'll have to explicitly cast to coerce to void*).

(TODO(me): file a docs bug about this) rust-lang/rust#79430

Categories (optional)

• Kind: clippy::correctness probably.

What is the advantage of the recommended code over the original code

copy_nonoverlapping already incorporates size_of::<T>() into the offset calculation, so it avoids reads/writes out of bounds, probably.

Drawbacks

It's possible the original code isn't wrong, but if so it would be highly confusing.

Example

Assuming a and b are pointers to T:

ptr::copy_nonoverlapping(a, b, std::mem::size_of::<T>() * N);

Could be written as:

ptr::copy_nonoverlapping(a, b, N);

[nico-abram]

Should this also check for std::mem::size_of_val?

[nico-abram]

Can I take this issue? I'm not sure how complex it is but I feel like I can probably do it

[flip1995]

@nico-abram Yeah sure go ahead. Make sure you take a look at the documentation in the doc/ dir. If you have any questions, Zulip is probably the fastest way to get an answer. Otherwise open a WIP PR or ask here.

[nico-abram]

I have a basic form of this that just checks for the count parameter being size_of or size_of_val, or a series of multiplications which involves size_of or size_of_val (With the pointee type of the copy) seemingly working. Is checking for multiplications fine? Should I also check for any other binary expressions? (Like divisions)

[thomcc]

It's probably dubious to use it in any case but you should definitely check for copy(a, b, size_of::<T>()) (e.g. implicit multiplication by 1).

[nico-abram]

Yeah that's the first case I looked for. This is what I have right now for checking that:

fn get_size_of_ty(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<TyM<'tcx>> {
    match &expr.kind {
        ExprKind::Call(ref count_func, func_args) => {
            if_chain! {
                if let ExprKind::Path(ref count_func_qpath) = count_func.kind;
                if let Some(def_id) = cx.qpath_res(count_func_qpath, count_func.hir_id).opt_def_id();
                if match_def_path(cx, def_id, &paths::MEM_SIZE_OF) || match_def_path(cx, def_id, &paths::MEM_SIZE_OF_VAL);
                then {
                    cx.typeck_results().node_substs(count_func.hir_id).types().next()
                } else {
                    None
                }
            }
        },
        ExprKind::Binary(op, left, right) if &BinOpKind::Mul == &op.node || &BinOpKind::Div == &op.node => {
            get_size_of_ty(cx, &*left).or_else(|| get_size_of_ty(cx, &*right))
        },
        _ => None,
    }
}

With these test cases:

    const SIZE: usize = 128;
    const HALF_SIZE: usize = SIZE / 2;
    const DOUBLE_SIZE: usize = SIZE * 2;
    let mut x = [2u8; SIZE];
    let mut y = [2u8; SIZE];

    // Count is size_of (Should trigger the lint)
    unsafe { copy_nonoverlapping::<u8>(x.as_ptr(), y.as_mut_ptr(), size_of::<u8>()) };
    unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0])) };

    unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of::<u8>()) };
    unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0])) };

    // Count expression involving multiplication of size_of (Should trigger the lint)
    unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::<u8>() * SIZE) };
    unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0]) * SIZE) };

    unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of::<u8>() * SIZE) };
    unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0]) * SIZE) };

    // Count expression involving nested multiplications of size_of (Should trigger the lint)
    unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::<u8>() * HALF_SIZE * 2) };
    unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), HALF_SIZE * size_of_val(&x[0]) * 2) };

    unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of::<u8>() * SIZE * HALF_SIZE) };
    unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0]) * HALF_SIZE * 2) };

    // Count expression involving divisions of size_of (Should trigger the lint)
    unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::<u8>() * DOUBLE_SIZE / 2) };
    unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), DOUBLE_SIZE / 2 * size_of_val(&x[0])) };

    unsafe { copy(x.as_ptr(), y.as_mut_ptr(), DOUBLE_SIZE * size_of::<u8>() / 2) };
    unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&x[0]) * DOUBLE_SIZE / 2) };

    // No size_of calls (Should not trigger the lint)
    unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), SIZE) };
    unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), SIZE) };

    unsafe { copy(x.as_ptr(), y.as_mut_ptr(), SIZE) };
    unsafe { copy(x.as_ptr(), y.as_mut_ptr(), SIZE) };

    // Different types for pointee and size_of (Should not trigger the lint)
    unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of::<u16>() / 2 * SIZE) };
    unsafe { copy_nonoverlapping(x.as_ptr(), y.as_mut_ptr(), size_of_val(&0u16) / 2 * SIZE) };

    unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of::<u16>() / 2 * SIZE) };
    unsafe { copy(x.as_ptr(), y.as_mut_ptr(), size_of_val(&0u16) / 2 * SIZE) };

I think I'll open a PR soon