Skip to content
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

zeroize: breaking change for bounds #878

Closed
ETKNeil opened this issue Mar 27, 2023 · 14 comments · Fixed by #879 or #882
Closed

zeroize: breaking change for bounds #878

ETKNeil opened this issue Mar 27, 2023 · 14 comments · Fixed by #879 or #882

Comments

@ETKNeil
Copy link

ETKNeil commented Mar 27, 2023

Consider the following code

use zeroize::{Zeroize, ZeroizeOnDrop};

#[derive(zeroize::ZeroizeOnDrop, zeroize::Zeroize)]
pub struct Test<A: Marker> {
    #[zeroize(skip)]
    field: Option<A>,
}

pub trait Secret: ZeroizeOnDrop + Zeroize{}

impl<A:Marker> Secret for Test<A>{

}

pub trait Marker {}

This code used to compile with:

[dependencies]
zeroize = { version = "1", features = ["derive"] }
zeroize_derive = { version = "=1.3.3" }

But now by updating the derive

[dependencies]
zeroize = { version = "1", features = ["derive"] }
zeroize_derive = { version = "=1.4.0" }

I get

error[E0277]: the trait bound `Test<A>: Zeroize` is not satisfied
  --> src/lib.rs:11:16
   |
11 | impl<A:Marker> Secret for Test<A>{
   |                ^^^^^^ the trait `Zeroize` is not implemented for `Test<A>`
   |
note: required by a bound in `Secret`
  --> src/lib.rs:9:35
   |
9  | pub trait Secret: ZeroizeOnDrop + Zeroize{}
   |                                   ^^^^^^^ required by this bound in `Secret`
help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
   |
11 | impl<A:Marker> Secret for Test<A> where Test<A>: Zeroize{
   |                                   ++++++++++++++++++++++

For more information about this error, try `rustc --explain E0277`.
error: could not compile `test_zeroize` due to previous error

Which is due to #858

I bisected and downgrading derive to 81536ee works perfectly with my example, upgrading to syn v2 broke something

@ETKNeil
Copy link
Author

ETKNeil commented Mar 27, 2023

If I would have to guess the change of logic in attr_skip/filter_skip is responsible, in particular in generate_fields I see a lot of logic change.

@tarcieri
Copy link
Member

cc @maurer

maurer added a commit to maurer/RustCrypto-utils that referenced this issue Mar 27, 2023
@maurer
Copy link
Contributor

maurer commented Mar 27, 2023

My understanding of the behavior of the previous code was that if a type variable was present in the generics, a : Zeroize bound got added for it by synstructure unless it specified its own explicit bounds (since the .add_bounds(AddBounds::None) was behind a conditional).

However, it must be suppressed elsewhere, since I just tested the original version and it doesn't inject the bound.

I've sent up a PR that never injects the bound but it might be worth considering making the bound get injected at some point if the type parameter is used in an unskipped field, since that will just fail to compile without a manual bounds annotation at the moment.

I've also added an explicit testcase for this issue to avoid making this error again.

@tarcieri
Copy link
Member

@maurer awesome, thank you!

tarcieri pushed a commit that referenced this issue Mar 27, 2023
@tarcieri
Copy link
Member

@ETKNeil can you confirm this was fixed by zeroize_derive v1.4.1?

@ETKNeil
Copy link
Author

ETKNeil commented Mar 28, 2023

@ETKNeil can you confirm this was fixed by zeroize_derive v1.4.1?

I confirm that 1.4.1 did fix it, thank you very much for reacting so quickly

@camshaft
Copy link
Contributor

I am still seeing issues with v1.4.1 for the following code:

#[derive(Zeroize)]
pub struct Key<T>(T);
error[E0599]: the method `zeroize` exists for mutable reference `&mut T`, but its trait bounds were not satisfied
  --> src/lib.rs:53:10
   |
53 | #[derive(Zeroize)]
   |          ^^^^^^^^^^^^^^^^ method cannot be called on `&mut T` due to unsatisfied trait bounds
   |
   = note: the following trait bounds were not satisfied:
           `T: DefaultIsZeroes`
           which is required by `T: zeroize::Zeroize`
           `&mut T: DefaultIsZeroes`
           which is required by `&mut T: zeroize::Zeroize`
   = note: this error originates in the derive macro `zeroize::Zeroize` (in Nightly builds, run with -Z macro-backtrace for more info)

This code is able to compile with zeroize_derive set to v1.3.3.

@tarcieri
Copy link
Member

cc @maurer

maurer added a commit to maurer/RustCrypto-utils that referenced this issue Mar 30, 2023
The where clauses I was previously injecting *were* load bearing, but
they needed to be filtered for skipped fields.

Fixes RustCrypto#878
@maurer
Copy link
Contributor

maurer commented Mar 30, 2023

Sorry about that, the where clauses I pulled out for the previous user shouldn't have been pulled out all the way. I've added another test for this side of things and am automatically generating Zeroize bounds for used fields now.

tarcieri pushed a commit that referenced this issue Mar 31, 2023
The where clauses I was previously injecting *were* load bearing, but
they needed to be filtered for skipped fields.

Fixes #878
@tarcieri tarcieri changed the title [Zeroize] Breaking change for bounds zeroize: breaking change for bounds Mar 31, 2023
@tarcieri
Copy link
Member

Released changes from #882 as zeroize_derive v1.4.2

@conradoplg
Copy link

conradoplg commented Apr 13, 2023

I ran into another breakage related to this. However, it seems that it was mistakenly not broken before?

Here is an example:

use zeroize::Zeroize;

pub trait Field: Copy + Clone {
    type Scalar: Copy + Clone;
}

#[derive(Zeroize)]
struct MyStruct<C: Field>(<C as Field>::Scalar);

fn main() {
    println!("Hello, world!");
}

This compiles with zeroize_derive 1.3.3, but breaks in 1.4.2 with

error[E0599]: the method `zeroize` exists for mutable reference `&mut <C as Field>::Scalar`, but its trait bounds were not satisfied
 --> src/main.rs:8:10
  |
8 | #[derive(Zeroize)]
  |          ^^^^^^^ method cannot be called on `&mut <C as Field>::Scalar` due to unsatisfied trait bounds
  |
  = note: the following trait bounds were not satisfied:
          `<C as Field>::Scalar: DefaultIsZeroes`
          which is required by `<C as Field>::Scalar: Zeroize`
          `&mut <C as Field>::Scalar: DefaultIsZeroes`
          which is required by `&mut <C as Field>::Scalar: Zeroize`
  = note: this error originates in the derive macro `Zeroize` (in Nightly builds, run with -Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0599`.

It seems like it shouldn't really work before since I don't see how it could derive Zeroize for Scalars. But I'm really not an expert on this so I'm reporting it in case it's actually a bug.

@tarcieri
Copy link
Member

@conradoplg it should work if a where <C as Field>::Scalar: Zeroize bound is added to the impl

@conradoplg
Copy link

@conradoplg it should work if a where <C as Field>::Scalar: Zeroize bound is added to the impl

Indeed that works, I'm just curious about how it compiled without the bound before

@tarcieri
Copy link
Member

My guess would be it used to infer the bound /cc @maurer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants