zeroize: Improved attribute parser (fixes #237)

The previous attribute parser used a hack where it would compare to the
s-exp representation of zeroize attributes to determine of they are
`drop` or `no_drop`. This was a bit of a hack that broke with a recent
nightly, which removed the spaces surrounding the ident.

This commit rewrites the attribute parser to properly walk the attribute
`syn::Meta` nodes in the AST until it arrives at a `syn::Ident`
containing either `drop` or `no_drop`. All other AST nodes will result
in a panic.

This relaxes the previous restriction that any `#[derive(Zeroize)]` MUST
be annotated with either `drop` or `no_drop`. This was put in place
because `zeroize` v0.9 switched from default drop to requiring an
explicit attribute. However v0.8 was the only version with implict drop,
and has been yanked for several weeks. The change is fully backwards
compatible and should have no effect on any existing v0.9 users.

Author: tony-iqlusion

zeroize/README.md

@@ -13,7 +13,7 @@ This crate provides a safe<sup>†</sup>, portable access to cross-platform
 intrinsics for securely zeroing memory which are specifically documented as
 guaranteeing they won't be "optimized away".
 
-The [`Zeroize` trait] is the crate's primary (and only) API.
+The [`Zeroize` trait] is the crate's primary API.
 
 [Documentation]
 

zeroize/src/lib.rs

@@ -52,17 +52,13 @@
 //!
 //! ## 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.
 //!
-//! Additionally it supports the following attributes (you *MUST* pick one):
+//! Additionally it supports the following attributes:
 //!
-//! - `#[zeroize(no_drop)]`: derive only `Zeroize` without adding a `Drop` impl
 //! - `#[zeroize(drop)]`: call `zeroize()` when this item is dropped
+//! - `#[zeroize(no_drop)]`: legacy attribute which will be removed in `zeroize` 1.0
 //!
 //! Example which derives `Drop`:
 //!
@@ -82,7 +78,6 @@
 //!
 //! // This struct will *NOT* be zeroized on drop
 //! #[derive(Copy, Clone, Zeroize)]
-//! #[zeroize(no_drop)]
 //! struct MyStruct([u8; 32]);
 //! ```
 //!

zeroize_derive/src/lib.rs

@@ -8,55 +8,89 @@ extern crate proc_macro;
 
 use proc_macro2::TokenStream;
 use quote::quote;
+use syn::{Attribute, Ident, Meta, NestedMeta};
 use synstructure::{decl_derive, BindStyle};
 
 /// Name of zeroize-related attributes
 const ZEROIZE_ATTR: &str = "zeroize";
 
 /// Custom derive for `Zeroize`
 fn derive_zeroize(s: synstructure::Structure) -> TokenStream {
-    let attributes = ZeroizeDeriveAttrs::parse(&s);
+    let attributes = DeriveAttrs::parse(&s);
 
     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"),
+        Some(false) | None => derive_zeroize_without_drop(s),
     }
 }
 decl_derive!([Zeroize, attributes(zeroize)] => derive_zeroize);
 
 /// Custom derive attributes for `Zeroize`
-struct ZeroizeDeriveAttrs {
+struct DeriveAttrs {
     /// Derive a `Drop` impl which calls zeroize on this type
     drop: Option<bool>,
 }
 
-impl Default for ZeroizeDeriveAttrs {
+impl Default for DeriveAttrs {
     fn default() -> Self {
         Self { drop: None }
     }
 }
 
-impl ZeroizeDeriveAttrs {
+impl DeriveAttrs {
     /// Parse attributes from the incoming AST
     fn parse(s: &synstructure::Structure) -> Self {
         let mut result = Self::default();
 
         for v in s.variants().iter() {
             for attr in v.ast().attrs.iter() {
-                if attr.path.is_ident(ZEROIZE_ATTR) {
-                    // TODO(tarcieri): hax, but probably good enough for now
-                    match attr.tts.to_string().as_ref() {
-                        "( drop )" => result.drop = Some(true),
-                        "( no_drop )" => result.drop = Some(false),
-                        other => panic!("unknown zeroize attribute: {}", other),
-                    }
-                }
+                result.parse_attr(attr);
             }
         }
 
         result
     }
+
+    /// Parse attribute and handle `#[zeroize(...)]` attributes
+    fn parse_attr(&mut self, attr: &Attribute) {
+        let meta = attr
+            .parse_meta()
+            .unwrap_or_else(|e| panic!("error parsing attribute: {} ({})", attr.tts, e));
+
+        if let Meta::List(list) = meta {
+            if list.ident != ZEROIZE_ATTR {
+                return;
+            }
+
+            for nested_meta in &list.nested {
+                if let NestedMeta::Meta(Meta::Word(ident)) = nested_meta {
+                    self.parse_attr_ident(ident);
+                } else {
+                    panic!("malformed #[zeroize] attribute: {:?}", nested_meta);
+                }
+            }
+        }
+    }
+
+    /// Parse a `#[zeroize(...)]` attribute containing a single ident (e.g. `drop`)
+    fn parse_attr_ident(&mut self, ident: &Ident) {
+        if ident == "drop" {
+            self.set_drop_flag(true);
+        } else if ident == "no_drop" {
+            self.set_drop_flag(false);
+        } else {
+            panic!("unknown #[zeroize] attribute type: {}", ident);
+        }
+    }
+
+    /// Set the value of the `drop` flag
+    fn set_drop_flag(&mut self, value: bool) {
+        if self.drop.is_some() {
+            panic!("duplicate #[zeroize] drop/no_drop flags");
+        } else {
+            self.drop = Some(value);
+        }
+    }
 }
 
 /// Custom derive for `Zeroize` (without `Drop`)