Make Default trait derive module specific#470
Conversation
There was a problem hiding this comment.
Code Review
This pull request correctly makes derive_default a module-specific option, which resolves an issue with the cupti module. This is a good improvement. However, enabling default derivations across the board introduces a soundness issue for the _IO_FILE type, which is pulled in as a dependency for some modules. I've left a critical comment with a suggested fix to prevent bindgen from generating an incorrect Default implementation for this type.
| non_exhaustive: false, | ||
| }) | ||
| .derive_default(false) | ||
| .derive_default(self.derive_default) |
There was a problem hiding this comment.
While enabling derive_default is a great improvement for ergonomics, it introduces a critical soundness issue. For the cublaslt and cusolver modules, bindgen pulls in the _IO_FILE type (also known as FILE in C) and generates a Default implementation for it.
This generated implementation zero-initializes the struct, which is unsound because a zeroed FILE struct is not guaranteed to be valid. Using a default-initialized _IO_FILE could lead to undefined behavior.
To fix this, you can instruct bindgen to not generate a Default implementation for _IO_FILE using .no_default(). I recommend making this configurable in ModuleConfig by adding a no_default_types field.
You could update ModuleConfig like this:
struct ModuleConfig {
// ...
/// Whether to derive Default implementations for types.
derive_default: bool,
/// Types to not derive `Default` for.
no_default_types: Vec<&'static str>,
}Then, in create_modules(), you can specify no_default_types: vec!["_IO_FILE"] for cublaslt and cusolver, and an empty vec![] for the other modules where derive_default is true.
Finally, apply this configuration in ModuleConfig::builder():
// ... in ModuleConfig::builder()
let mut builder = builder.derive_default(self.derive_default);
for ty in &self.no_default_types {
builder = builder.no_default(*ty);
}
builder
.derive_eq(true)
// ...This will resolve the soundness issue while keeping the benefits of derive_default for the other types.
|
Hi, ty for the fast pr - resolving this with #471 instead |
Make the use of bindgen's
derive_defaultmodule-specific. Keep deriving it for anything except the CUPTI module. Fixes: #469