Add FCW for invalid C variadic arguments - #162478
theemathas wants to merge 3 commits into
Conversation
|
rustbot has assigned @dingxiangfei2009. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| @future_incompatible = FutureIncompatibleInfo { | ||
| reason: fcw!(FutureReleaseError #61275), | ||
| }; |
There was a problem hiding this comment.
Ah, people changed the syntax of this macro again and now one cannot easily tell whether this will be reported in dependencies or not. :/
That's a side-effect of #141936. @WaffleLapkin why is report_in_depds an optional field? The default is far from obvious. (When I introduced FutureReleaseErrorDontReportInDeps many people were surprised that FCW do not report-in-deps by default. That's why I introduced this name that makes it so obvious. IMO it is a step backwards that now we again have syntax where this is not obvious.)
There was a problem hiding this comment.
Based on this, it seems that the field being optional is intentional:
rust/compiler/rustc_lint_defs/src/lib.rs
Lines 324 to 335 in 745de6e
There was a problem hiding this comment.
I think that's a bad choice. It certainly could have warranted a bit more discussion, given that this effectively reverted changes I made previously (#116049), in terms of what is and is not explicit in the API.
I guess people weren't aware of the prior discussion and didn't realize the downsides of the new API choice. Time to make another PR to make report_in_depds mandatory I guess... except I don't know how to make it mandatory just for FutureReleaseError; we don't need it mandatory for edition errors as those "obviously" are not reported in dependencies. That's the downside of the new structure...
There was a problem hiding this comment.
I was indeed not aware of the previous discussion, ugh =_=
I think the justification from #141936, "It gets especially unruly if you want to add non-FutureReleaseError* warnings which are included in the reports." was targeted at EditionAndFutureReleaseError which I was working with at the time, in the process of stabilizing never.
Looking at the current structure, I'd say we can put report_in_deps in ReleaseFcw. That adds the assumption that we only want to report warnings in dependencies if we plan to change something in a future release, but I guess that's fine (and is at the very least currently true).
I'll make a PR for this.
There was a problem hiding this comment.
Looking at the current structure, I'd say we can put report_in_deps in ReleaseFcw. That adds the assumption that we only want to report warnings in dependencies if we plan to change something in a future release, but I guess that's fine (and is at the very least currently true).
I was assuming you'd not want that since it seems to partially revert your PR #141936, by coupling report_in_deps with the reason again. But it sounds great to me so if you can also live with it, all good. :)
I'll make a PR for this.
❤️
There was a problem hiding this comment.
Presumably we'd then add ReleaseFcw to EditionAndFutureReleaseError and to EditionAndFutureReleaseSemanticsChange? (Currently these only include EditionFcw.)
If helpful to factoring, note that lang has been following the policy of setting report_in_deps = true exactly when we make an FCW deny-by-default (and otherwise setting report_in_deps = false). Possibly, after cleaning up any lingering exceptions, it could be OK to lean on that.
There was a problem hiding this comment.
#159700 is a recent example of a Warn + report_in_deps lint. It also shows up in a lot of dependency trees so maybe that was not a good call and it should have been Warn-only for a while like normal FCWs...
This comment has been minimized.
This comment has been minimized.
2ff9fb2 to
5cb13f7
Compare
|
It would be good to do a crater check run with the lint level set to deny to see if there are any commonly-used types that don't currently implement |
|
|
This comment has been minimized.
This comment has been minimized.
5cb13f7 to
4a0af16
Compare
|
cc @rust-lang/miri |
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
Add FCW for invalid C variadic arguments
This comment has been minimized.
This comment has been minimized.
|
@rust-lang/lang Nominating for team discussion. :) See the PR description for a summary. We ask for your feedback on which of these three options you would prefer (or whether you'd prefer something entirely different):
My personal preference is option 3. That standard library code looks entirely reasonable, there is no reason to change it. We define |
This comment has been minimized.
This comment has been minimized.
|
💔 Test for ec4987a failed: CI. Failed jobs:
|
|
I'm against making this a FCW; I think it should just be a normal warn-by-default lint (perhaps deny for certain cases). There are perfectly legitimate reasons to pass non- |
@Jules-Bertholet Is there a legitimate reason to then proceed to pass a value of that type as a variadic argument? |
|
If some weird C API you are linking against requires it, then that's what you have to do. (Sorry for typo, see edited message) |
|
@Jules-Bertholet I believe that passing a repr(C) struct (or a struct defined in C) as a variadic argument is UB both in C and in rust. Or if it's not UB when you pass it, it will be UB when you try to read it. |
In C, all types that can be passed as function arguments (except for types like
I think the solution there would be to allow such types to implement |
|
🚧 Experiment ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more |
|
🎉 Experiment
Footnotes
|
I think that may be a new record 😂 |
|
I did a quick The full list()
A
<A as FormatArg>::Raw
<A as PrintfArgument>::CPrintfType
AtFlags
Axis
AxisSource
B
<B as FormatArg>::Raw
<B as PrintfArgument>::CPrintfType
BorrowedFd<'_>
bpf_cmd
BpfCmd
ButtonState
C
Capability
<C as FormatArg>::Raw
<C as PrintfArgument>::CPrintfType
char
*const CStr
&CStr
CString
<D as FormatArg>::Raw
<D as PrintfArgument>::CPrintfType
<E as FormatArg>::Raw
<E as PrintfArgument>::CPrintfType
enums::VipsAccess
enums::VipsFailOn
enums::VipsForeignFlags
enums::VipsHeifCompression
enums::VipsHeifEncoder
enums::VipsIntent
enums::VipsInteresting
enums::VipsInterpretation
enums::VipsKeep
enums::VipsPCS
enums::VipsPngFilter
enums::VipsSize
enums::VipsSubsample
enums::VipsWebpPreset
Errno
extern "C" fn(*const c_void, u32, *mut c_void) -> i32
extern "C" fn(*const c_void, u32) -> u32
extern "C" fn(*mut Box<dyn FnMut(usize, usize, usize, usize)>, f64, f64, f64, f64) -> i32
extern "C" fn(*mut CURL, u32, *mut c_void)
extern "C" fn(*mut CURL, u32, u32, *mut c_void)
extern "C" fn(*mut c_void, i32, *const i8)
extern "C" fn(*mut F)
extern "C" fn(*mut i8, usize, usize, *mut c_void) -> usize
extern "C" fn(*mut u8, usize, usize, *mut Body<'_>) -> usize
extern "C" fn(*mut u8, usize, usize, &mut ResponseBuilder) -> usize
extern "C" fn(*mut u8, usize, usize, *mut ResponseBuilder) -> usize
<F as FormatArg>::Raw
<F as PrintfArgument>::CPrintfType
fcntl::FdFlags
Fd1
FdFlags
for<'a> unsafe fn(&'a Inner)
for<'a> unsafe fn(*const c_void, i32, &'a Inner)
for<'a> unsafe fn(i32, &'a Inner)
for<'a> unsafe fn(u32, *const c_void, &'a Inner)
for<'a> unsafe fn(u32, *mut c_void, &'a Inner)
Format
FsConfig
FullscreenMethod
<G as FormatArg>::Raw
<G as PrintfArgument>::CPrintfType
<H as FormatArg>::Raw
<H as PrintfArgument>::CPrintfType
<I as FormatArg>::Raw
InsnX86
<J as FormatArg>::Raw
<K as FormatArg>::Raw
KdMode
KeymapFormat
KeyState
KvmVmType
<L as FormatArg>::Raw
linux_bindings_x86_64::bpf_cmd
<M as FormatArg>::Raw
MediaDirection
MIR_op_t
mpk::PkeyAccessRights
*mut input_mt_request_layout
&mut [MaybeUninit<u8>]
&mut Self
&mut T
&mut [u8]
&mut [watch_device::{closure#0}::{closure#0}::input_absinfo]
<N as FormatArg>::Raw
NonZero<usize>
<O as FormatArg>::Raw
OfferAnswerParameters
Option<&[u8]>
Option<unsafe extern "C" fn(*mut Curl, i32, i32, *mut c_void, *mut c_void) -> i32>
Option<unsafe extern "C" fn(*mut CurlMulti, i64, *mut c_void) -> i32>
<P as FormatArg>::Raw
Pid
PrintkArg
<Q as FormatArg>::Raw
<R as FormatArg>::Raw
rd_kafka_vtype_t
Resize
Result<CString, NulError>
ruby_special_consts
rustix::process::Pid
<S as FormatArg>::Raw
scmp_arg_cmp
SCMP_ARG_CMP
scmp::scmp_arg_cmp
&[Segment<'_, '_>]
Self
semun
Signal
&[spi_ioc_transfer<'_, '_>]
&std::ffi::CStr
&str
String
Subpixel
sys::linux::kvm::KvmCap
sys::linux::kvm::KvmVmType
sys::linux::vfio::VfioIommu
T
T0Jni
T10Jni
T11Jni
T1Jni
T2Jni
T3Jni
T4Jni
T5Jni
T6Jni
T7Jni
T8Jni
T9Jni
<T as FormatArg>::Raw
<T as traits::CReprHolder>::Output
&[TransferSegment<'_, '_>]
Transform
Transient
TTF_HorizontalAlignment
TunFeature
types::DefaultKeyring
&[u8]
[u8; 16]
<U as FormatArg>::Raw
unicorn_const::ContextMode
unicorn_engine_sys::Arm64Insn
unicorn_engine_sys::ContextMode
unicorn_engine_sys::X86Insn
unsafe extern "C" fn(*mut c_void)
unsafe extern "C" fn(*mut i8, usize, usize, *mut c_void) -> usize
VaList<'_>
VALUE
<V as FormatArg>::Raw
VipsCombineMode
VipsKernel
VipsSize
vp8e_token_partitions
vp8e_tuning
Vp8NoiseSensitivity
Vp8ScreenContentMode
Vp9AQMode
Vp9ColorRange
Vp9NoiseSensitivity
Vp9SvcInterLayerPred
Vp9TuneContent
<W as FormatArg>::Raw
wl_output::Mode
wl_shell_surface::Resize
wl_shell_surface::Transient
x86_const::InsnSysX86
x86::InsnSysX86
x86::InsnX86
<X as FormatArg>::Raw
<Y as FormatArg>::Raw
<Z as FormatArg>::Raw |
|
I'm listing what types triggered the lint in crates with a total 100 or more dependents (total count from all versions of the crate combined, but I'm only taking the types from the latest version that was flagged).
|
|
How do we proceed now? |
Of course you want a
Passing function pointers, Maybe we can start with linting on fat pointers? I imagine that can have a good suggestion too with something like "did you mean Possibly this can be extended to anything that So, I guess what I'm proposing is to tighten the lint gradually? |
|
The |
|
We discussed this in the lang meeting today. Those present weren't sure an FCW made sense while there are types that should implement this but do not (defined by the standard library), see below. My belief is that we shouldn't recommend The suggestion in #162478 (comment) to make thin references implement A question was raised about extern types, which are currently unstable: These types do not implement Function pointers are another class of types that do not implement the trait but should; see #153646. Finally, in the meeting we discussed the question of whether ecosystem crates should be able to implement
Only the standard library can make these kinds of guarantees today. It's conceivable that we could open things up in the future, but for now I think we should continue to say that users can't define types that are VarArgSafe. |
|
Expressing this as a bound is actually quite trivial: impl<T: PointeeSized + core::marker::Pointee<Metadata = ()>> VaArgSafe for *mut TIt might conflict with a blanket impl for |
|
The biggest problem with disallowing external libraries from implementing As proposed in #44930 (comment), we could allow libraries to implement this trait, but only with a check by the compiler to ensure that the impl is actually legitimate. |
if the trait is unstable, you can make it a |
Musing: I wonder if references should be not |
|
The trait is stable... well it will be very soon. |
|
That is being discussed on zulip, because there's other reasons we might want to destabilize it: https://rust-lang.zulipchat.com/#narrow/channel/219381-t-libs/topic/Implementing.20.60VaArgSafe.60.20on.20references.20is.20a.20breaking.20change/with/624988218 |
|
As per #t-libs > Implementing `VaArgSafe` on references is a breaking change, we're very likely to destabilize |
View all comments
Fixes #61275 by adding an FCW for invalid C-variadic arguments:
invalid_c_variadic_arguments.Tracking issue for the FCW: #162483
For the purposes of this FCW, a valid C-variadic argument must either implement
VaArgSafe, or be a thin reference.cc @RalfJung
Background
C-variadic functions are functions defined either in Rust or externally via FFI that can accept any number of arguments. However, due to C ABI weirdness, only certain types can be passed as C-variadic arguments.
Previously, we had a check that would cause us to attempt to emit a hard error on commonly mistakenly used C-variadic argument types. In particular, this affected types that are not "directly supported" as a variadic argument, but would be automatically coerced to a supported type in C/C++. For instance, when passing a
short, a C/C++ compiler will automatically promote this tointand so on the ABI level, anintgets passed. We do not do such coercions in Rust, so passing ani16can lead to fatal bugs due to the wrong ABI being used.In #61275, it was found that this hard error didn't prevent such footguns from occurring when the C-variadic function was called with generic arguments. It was then also later found that this error had a bug that caused it to depend on the details of the type inference algorithm.
The current behavior of this hard error is as follows:
f32,i8,i16,u8,u16, orbool, and the type doesn't implementVaArgSafein the current target, we emit an error.Stringdoesn't trigger this error.)In #155697, we stabilized the ability to define C-variadic functions in Rust. With it, we also stabilized the
VaArgSafetrait. This trait is implemented for types that are supported as variadic arguments. Thus, we now have the ability, in stable Rust, to describe the type requirements for being supported as a C-variadic argument.Therefore, this PR adds an FCW that would warn against C-variadic arguments that are not
VaArgSafe. In generic contexts, users can add aT: VaArgSafebound to satisfy this lint. This FCW runs after type inference is done, but before monomorphization.There's a caveat though: There's likely much code in the wild that passes a reference as a C-variadic argument. However, references do not implement
VaArgSafeyet. Thus, we don't yet lint when a thin reference is passed as a C-variadic argument.In the future, I expect that it would be possible to turn this into a hard error by, in the type inference/checking algorithm, adding a trait obligation that requires C-variadic arguments to implement
VaArgSafe. (This is similar to what happens when one calls afn<T: VaArgSafe>(T).) This can technically break code that wasn't previously linted, due to lifetime-dependent where bounds, and maybe due to the effect that the trait bound has on subsequent type inference. I expect this to be extremely unlikely though.