zeroize_derive: Require zeroize(drop) or zeroize(no_drop)

The previous version of `zeroize` (v0.8) automatically derived a `Drop`
impl unless `zeroize(no_drop)` was passed explicitly.

This felt like the right thing to do at first, however it turns out
there are a lot of cases in practice where it's undesirable to derive
`Drop` by default, e.g. on any `Copy` type. This was encountered
in-practice retrofitting `zeroize` into the `curve25519-dalek` crate.

This changes the custom derive impl to require an explicit decision in
the form of an attribute as to whether a `Drop` impl should be derived
or not.

Going forward (i.e. in `zeroize` 1.0) deriving `Drop` will need to be
explicitly declared as `zeroize(drop)`, and the need to explicitly
specify `zeroize(no_drop)` can be removed, meaning no `Drop` impl will
be derived by default. However, since previous users of `zeroize` v0.8
will be expecting `Drop` by default, this change makes it an explicit
decision, so as to avoid people expecting to get a `Drop` impl by
default not actually receiving one.

Once crates.io stats show `zeroize` v0.8 is no longer in use (or in
`zeroize` 1.0, whichever comes first), the need to specify explicitly
that no `Drop` handler should be derived via `zeroize(no_drop)` can be
removed, however until then this change does the
safely-explicit-but-annoying thing by default so as to correct for
previous API mistakes.

Author: tony-iqlusion

zeroize/src/lib.rs

@@ -52,42 +52,38 @@
 //!
 //! ## Custom Derive Support
 //!
+//! **NOTICE**: Previous versions of `zeroize` automatically derived
+//! `Drop`. This has been *REMOVED* and you now *MUST* explicitly specify
+//! either `zeroize(drop)` or `zeroize(no_drop)` (see below).
+//!
 //! This crate has custom derive support for the `Zeroize` trait, which
-//! automatically calls `zeroize()` on all members of a struct or tuple struct,
-//! and adds a `Drop` impl which calls `zeroize()` when the item is dropped:
+//! automatically calls `zeroize()` on all members of a struct or tuple struct.
 //!
-//! ```
-//! use zeroize::Zeroize;
+//! Additionally it supports the following attributes (you *MUST* pick one):
 //!
-//! // This struct will be zeroized on drop
-//! #[derive(Zeroize)]
-//! struct MyStruct([u8; 64]);
-//! ```
+//! - `#[zeroize(no_drop)]`: derive only `Zeroize` without adding a `Drop` impl
+//! - `#[zeroize(drop)]`: call `zeroize()` when this item is dropped
 //!
-//! If, for some reason, you only want `Zeroize` to be derived but *don't*
-//! want an automatic `Drop` impl, you can add the `zeroize(no_drop)`
-//! attribute:
+//! Example which derives `Drop`:
 //!
 //! ```
 //! use zeroize::Zeroize;
 //!
-//! // This struct will *NOT* be zeroized on drop
+//! // This struct will be zeroized on drop
 //! #[derive(Zeroize)]
-//! #[zeroize(no_drop)]
-//! struct MyStruct([u8; 64]);
+//! #[zeroize(drop)]
+//! struct MyStruct([u8; 32]);
 //! ```
 //!
-//! If you prefer explicitness, you can add the `#[zeroize(drop)]`
-//! attribute to signal intent to zeroize values on `Drop`. However note this
-//! syntax is not necessary as the `Drop` handler is added by default:
+//! Example which does not derive `Drop` (useful for e.g. `Copy` types)
 //!
 //! ```
 //! use zeroize::Zeroize;
 //!
-//! // This struct will be zeroized on drop
-//! #[derive(Zeroize)]
-//! #[zeroize(drop)]
-//! struct MyStruct([u8; 64]);
+//! // This struct will *NOT* be zeroized on drop
+//! #[derive(Copy, Clone, Zeroize)]
+//! #[zeroize(no_drop)]
+//! struct MyStruct([u8; 32]);
 //! ```
 //!
 //! ## `Zeroizing<Z>`: wrapper for zeroizing arbitrary values on drop

zeroize/tests/zeroize_derive.rs

@@ -5,6 +5,7 @@ mod custom_derive_tests {
     use zeroize::Zeroize;
 
     #[derive(Zeroize)]
+    #[zeroize(drop)]
     struct ZeroizableTupleStruct([u8; 3]);
 
     #[test]
@@ -15,6 +16,7 @@ mod custom_derive_tests {
     }
 
     #[derive(Zeroize)]
+    #[zeroize(drop)]
     struct ZeroizableStruct {
         string: String,
         vec: Vec<u8>,

zeroize_derive/src/lib.rs

@@ -14,54 +14,26 @@ use synstructure::{decl_derive, BindStyle};
 const ZEROIZE_ATTR: &str = "zeroize";
 
 /// Custom derive for `Zeroize`
-fn zeroize_derive(mut s: synstructure::Structure) -> TokenStream {
-    s.bind_with(|_| BindStyle::RefMut);
-
+fn derive_zeroize(s: synstructure::Structure) -> TokenStream {
     let attributes = ZeroizeDeriveAttrs::parse(&s);
 
-    let zeroizers = s.each(|bi| quote! { #bi.zeroize(); });
-
-    let zeroize_impl = s.bound_impl(
-        quote!(zeroize::Zeroize),
-        quote! {
-            fn zeroize(&mut self) {
-                match self {
-                    #zeroizers
-                }
-            }
-        },
-    );
-
-    if attributes.no_drop {
-        return zeroize_impl;
-    }
-
-    let drop_impl = s.gen_impl(quote! {
-        gen impl Drop for @Self {
-            fn drop(&mut self) {
-                self.zeroize();
-            }
-        }
-    });
-
-    quote! {
-        #zeroize_impl
-
-        #[doc(hidden)]
-        #drop_impl
+    match attributes.drop {
+        Some(true) => derive_zeroize_with_drop(s),
+        Some(false) => derive_zeroize_without_drop(s),
+        None => panic!("must specify either zeroize(drop) or zeroize(no_drop) attribute"),
     }
 }
-decl_derive!([Zeroize, attributes(zeroize)] => zeroize_derive);
+decl_derive!([Zeroize, attributes(zeroize)] => derive_zeroize);
 
 /// Custom derive attributes for `Zeroize`
 struct ZeroizeDeriveAttrs {
-    /// Disable the on-by-default `Drop` derive
-    no_drop: bool,
+    /// Derive a `Drop` impl which calls zeroize on this type
+    drop: Option<bool>,
 }
 
 impl Default for ZeroizeDeriveAttrs {
     fn default() -> Self {
-        Self { no_drop: false }
+        Self { drop: None }
     }
 }
 
@@ -75,8 +47,8 @@ impl ZeroizeDeriveAttrs {
                 if attr.path.is_ident(ZEROIZE_ATTR) {
                     // TODO(tarcieri): hax, but probably good enough for now
                     match attr.tts.to_string().as_ref() {
-                        "( drop )" => (), // enabled by default
-                        "( no_drop )" => result.no_drop = true,
+                        "( drop )" => result.drop = Some(true),
+                        "( no_drop )" => result.drop = Some(false),
                         other => panic!("unknown zeroize attribute: {}", other),
                     }
                 }
@@ -87,15 +59,89 @@ impl ZeroizeDeriveAttrs {
     }
 }
 
+/// Custom derive for `Zeroize` (without `Drop`)
+fn derive_zeroize_without_drop(mut s: synstructure::Structure) -> TokenStream {
+    s.bind_with(|_| BindStyle::RefMut);
+
+    let zeroizers = s.each(|bi| quote! { #bi.zeroize(); });
+
+    s.bound_impl(
+        quote!(zeroize::Zeroize),
+        quote! {
+            fn zeroize(&mut self) {
+                match self {
+                    #zeroizers
+                }
+            }
+        },
+    )
+}
+
+/// Custom derive for `Zeroize` and `Drop`
+fn derive_zeroize_with_drop(s: synstructure::Structure) -> TokenStream {
+    let drop_impl = s.gen_impl(quote! {
+        gen impl Drop for @Self {
+            fn drop(&mut self) {
+                self.zeroize();
+            }
+        }
+    });
+
+    let zeroize_impl = derive_zeroize_without_drop(s);
+
+    quote! {
+        #zeroize_impl
+
+        #[doc(hidden)]
+        #drop_impl
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
     use synstructure::test_derive;
 
     #[test]
-    fn zeroize() {
+    fn zeroize_without_drop() {
+        test_derive! {
+            derive_zeroize_without_drop {
+                struct Z {
+                    a: String,
+                    b: Vec<u8>,
+                    c: [u8; 3],
+                }
+            }
+            expands to {
+                #[allow(non_upper_case_globals)]
+                #[doc(hidden)]
+                const _DERIVE_zeroize_Zeroize_FOR_Z: () = {
+                    extern crate zeroize;
+                    impl zeroize::Zeroize for Z {
+                        fn zeroize(&mut self) {
+                            match self {
+                                Z {
+                                    a: ref mut __binding_0,
+                                    b: ref mut __binding_1,
+                                    c: ref mut __binding_2,
+                                } => {
+                                    { __binding_0.zeroize(); }
+                                    { __binding_1.zeroize(); }
+                                    { __binding_2.zeroize(); }
+                                }
+                            }
+                        }
+                    }
+                };
+            }
+            no_build // tests the code compiles are in the `zeroize` crate
+        }
+    }
+
+    #[test]
+    fn zeroize_with_drop() {
         test_derive! {
-            zeroize_derive {
+            derive_zeroize_with_drop {
                 struct Z {
                     a: String,
                     b: Vec<u8>,