new restriction lint: pointer_format

[llogiq]

I read a blog post about kernel security, and how various features might get lost while porting to Rust. In kernel C, they have some guardrails against divulging pointers. An easy way to replicate that in Rust is a lint for pointer formatting. So that's what this lint does.

---

changelog: new [pointer_format] lint

[rustbot]

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[samueltardieu]

Lint example needs to be fixed. FCP thread created on Zulip.

@rustbot label +S-final-comment-period

[tgross35]

Is there any chance this could be adapted to check for any fmt trait being used on a pointer, rather than just the :p formatting? @nbdd0121 pointed out that a likely accident is printing via Debug, possibly through derives.

[llogiq]

Do you mean debug-formatting a function pointer? That should be workable for the direct case but might become pretty hard for the general case.

[tgross35]

I think a specific example would be something like this:

#[derive(Debug)]
pub struct S {
    pub p: *const u32,
}

pub fn foo(a: *const u32) {
    println!("{a:?}");
}

pub fn bar(a: S) {
    println!("{a:?}");
}

fn main() {
    foo(&0u32);
    bar(S{p: &0u32});
}

I'm not sure how this would be done in clippy since it would need to check whether a crate calls the Pointer / Debug implementations for raw pointers in the crate. If this isn't easy then checking directly in the format string still does help.

[llogiq]

A thing we might do there is to walk the types of the arguments (as far as they derive Debug, we should not go into manual implementations) and see if there's any raw pointer or reference.

[kpreid]

“…misuse for exfiltrating…” makes it sound like this is protection against attempts to insert malicious code into a project. Is auditing such code within scope for clippy? If not, it might be good to change the wording.

Also, “kernel context” isn’t very clear to people not familiar with what it’s talking about and sounds like it might be a formal or common Rust term, which it is not. How about something more like “In (projects|codebases|crates) such as operating system kernels,”?

[llogiq]

Yeah, I'm working on some code to add that. I may not be able to look through manual Debug impls, though, so it's a best-effort thing at best.

[ojeda]

Ideally, there would be a way to customize all pointer printing, i.e. including :p and :?, so that we can get them hashed as expected, and thus at that point we wouldn't need to lint for it. Meanwhile, this lint can be useful (and may be for other projects that do not want any printing whatsoever, too).

[llogiq]

I'm currently trying to do that, but I hit a wall trying to find the Debug impl to check if it was derived.

[Jarcho]

TyCtxt::is_automatically_derived should return true on the impl's DefId. It's not necessarily std's derive, but it should work for an initial implementation.

[llogiq]

Yeah, the problem is to get the impl's DefId.

[Jarcho]

Whoops. Slightly misread your comment. Instance::try_resolve with the Debug trait as the DefId and the expression's type as the generic arguments should get you that. You might need to use Debug::fmt instead, not sure.

[llogiq]

Ok, thanks to @compiler-errors' help, I was able to fix the problem. I will need to rebase and also I should add a lot of caching, because we're currently giving trait lookup a real workout.

[llogiq]

Now I at least cached the result of the Debug derive check. There may be more perf wins to be had by caching the result of the check and percolating it upward across the type tree, but that requires more bookkeeping, so I'm leaving it for later.

[llogiq]

@rustbot ready

[samueltardieu]

Now I at least cached the result of the Debug derive check. There may be more perf wins to be had by caching the result of the check and percolating it upward across the type tree, but that requires more bookkeeping, so I'm leaving it for later.

Maybe has_derived_debug and has_pointer_debug could become compiler queries.

[llogiq]

I normalized all types and added some simple caching of the pointer debug check (doesn't take in-between types into account yet, for that I would need to keep another stack).

[Jarcho]

Since the type is already normalized you don't need to normalize the contained types here. It's only ADT fields that normalization doesn't affect.

[Jarcho]

I think a recursive solution is better for this since it will allow you to stop as soon as you get a cache hit. Basically:

match cache.entry(ty) {
  Vacant(e) => e.insert(false),
  Occupied(res) => return res,
}
let res = calculate(ty);
cache.insert(res);
res

Right now your most common cache hit (false) still needs to walk the type.

[llogiq]

Which cache hit do you mean? I have two caches: One is the cache that stores whether a type has derived Debug. The other is whether a debug format shows a pointer. For the latter, I only store the outermost type. Doing all types would require storing the indices of the "stack frames" in our open stack. With that, I could walk the stack, adding the result to the last element of each frame.

[Jarcho]

The cache for whether a pointer is printed.

[llogiq]

Ok, I'll do the bookkeeping approach. I'd also like to note that I switched from the recursive variant to the loop because I got a stack overflow in dogfood, even with the visited set. Apparently there are some very deep types in the Rust compiler.

[Jarcho]

That's weird. You shouldn't be nesting anywhere near deep enough to do that.

If you weren't normalizing then that would probably be why. It's possible to end up with an infinite number of different projections that all resolve to the same type.

[llogiq]

Ok, I reverted to a recursive solution.

Implemented the suggestions

[y21]

I think there's still some stack overflows possible with truly infinitely recursive types (although that's not recursion at fault, it'd still lead to a hang with a loop), e.g. this crashes

#![warn(clippy::pointer_format)]
use core::marker::PhantomData;
use core::fmt::Debug;

trait Foo { type Assoc: Foo + Debug; }

#[derive(Debug)]
struct S<T: Foo + 'static>(&'static S<T::Assoc>, PhantomData<T>);

fn f<T: Foo + Debug + 'static>(s: &S<T>) {
    format!("{s:?}");
}

(Admittedly it's a weird/arbitrary repro, but the root issue is similar to a crash that was reported before: #13544)

Other lints have solved this using tcx.recursion_limit() and a depth parameter and just bailing out with false if recursing too deeply

[samueltardieu]

Can you make sure to include the example in #14792 (comment) and use the tcx.recursion_limit() to stop the infinite looping?

[llogiq]

Ok, the recursion limit is added.

applied requested changes

[samueltardieu]

LGTM, just waiting for another team member which is not you or me to +1 the FCP.