-
Notifications
You must be signed in to change notification settings - Fork 1.7k
new restriction lint: pointer_format #14792
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
Conversation
rustbot has assigned @samueltardieu. Use |
Lint example needs to be fixed. FCP thread created on Zulip. @rustbot label +S-final-comment-period |
Is there any chance this could be adapted to check for any |
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. |
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 |
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. |
clippy_lints/src/format_args.rs
Outdated
/// In kernel context, this might be vulnerable to misuse for exfiltrating | ||
/// stack or kernel function addresses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“…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,”?
Yeah, I'm working on some code to add that. I may not be able to look through manual |
Ideally, there would be a way to customize all pointer printing, i.e. including |
I'm currently trying to do that, but I hit a wall trying to find the |
|
Yeah, the problem is to get the impl's DefId. |
Whoops. Slightly misread your comment. |
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. |
This comment has been minimized.
This comment has been minimized.
Now I at least cached the result of the |
@rustbot ready |
Maybe |
201883c
to
9568bea
Compare
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). |
clippy_lints/src/format_args.rs
Outdated
ty::Ref(_, t, _) | ty::Slice(t) | ty::Array(t, _) => { | ||
open.push(tcx.normalize_erasing_regions(typing_env, *t)); | ||
}, | ||
ty::Tuple(ts) => open.extend(ts.iter().map(|ty| tcx.normalize_erasing_regions(typing_env, ty))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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 |
The cache for whether a pointer is printed. |
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. |
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. |
Ok, I reverted to a recursive solution. |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you make sure to include the example in #14792 (comment) and use the tcx.recursion_limit()
to stop the infinite looping?
Ok, the recursion limit is added. |
LGTM, just waiting for another team member which is not you or me to +1 the FCP. |
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