Auto merge of #7748 - ehuss:fix-config-env-var-prefix, r=alexcrichton

Fix config env vars that are prefix of another with underscore.

This fixes the CARGO_BUILD_TARGET_DIR and CARGO_PROFILE_DEV_DEBUG_ASSERTIONS environment variables.  Hopefully the big comment explains everything, but to review:

`deserialize_option` does not know the type of the value it is about to deserialize.  The type could be a struct, in which case it needs to check if `CARGO_FOO_BAR_*` environment variables are set. However, when deserializing something like `build.target`, this prefix check will incorrectly match `CARGO_BUILD_TARGET_DIR` which happens to be a prefix of `CARGO_BUILD_TARGET_*`.  It attempts to call `visit_some` which fails if `CARGO_BUILD_TARGET` is not set.

The solution here is to detect scenarios where one field is a prefix of another, and skip the environment prefix check (by setting `Deserializer::env_prefix_ok` appropriately).  This means we cannot have `Option<SomeStruct>` be a prefix of another field with an underscore.  I think that's fine, and we should try to probably avoid these kinds a prefixes anyways.

Closes #7253.

src/cargo/util/config/de.rs

@@ -4,15 +4,22 @@ use crate::util::config::value;
 use crate::util::config::{Config, ConfigError, ConfigKey};
 use crate::util::config::{ConfigValue as CV, Definition, Value};
 use serde::{de, de::IntoDeserializer};
-use std::collections::HashSet;
 use std::vec;
 
 /// Serde deserializer used to convert config values to a target type using
 /// `Config::get`.
 #[derive(Clone)]
-pub(crate) struct Deserializer<'config> {
-    pub(crate) config: &'config Config,
-    pub(crate) key: ConfigKey,
+pub(super) struct Deserializer<'config> {
+    pub(super) config: &'config Config,
+    /// The current key being deserialized.
+    pub(super) key: ConfigKey,
+    /// Whether or not this key part is allowed to be an inner table. For
+    /// example, `profile.dev.build-override` needs to check if
+    /// CARGO_PROFILE_DEV_BUILD_OVERRIDE_ prefixes exist. But
+    /// CARGO_BUILD_TARGET should not check for prefixes because it would
+    /// collide with CARGO_BUILD_TARGET_DIR. See `ConfigMapAccess` for
+    /// details.
+    pub(super) env_prefix_ok: bool,
 }
 
 macro_rules! deserialize_method {
@@ -109,7 +116,7 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> {
     where
         V: de::Visitor<'de>,
     {
-        if self.config.has_key(&self.key) {
+        if self.config.has_key(&self.key, self.env_prefix_ok) {
             visitor.visit_some(self)
         } else {
             // Treat missing values as `None`.
@@ -179,8 +186,10 @@ impl<'de, 'config> de::Deserializer<'de> for Deserializer<'config> {
 
 struct ConfigMapAccess<'config> {
     de: Deserializer<'config>,
-    set_iter: <HashSet<KeyKind> as IntoIterator>::IntoIter,
-    next: Option<KeyKind>,
+    /// The fields that this map should deserialize.
+    fields: Vec<KeyKind>,
+    /// Current field being deserialized.
+    field_index: usize,
 }
 
 #[derive(Debug, PartialEq, Eq, Hash)]
@@ -191,50 +200,53 @@ enum KeyKind {
 
 impl<'config> ConfigMapAccess<'config> {
     fn new_map(de: Deserializer<'config>) -> Result<ConfigMapAccess<'config>, ConfigError> {
-        let mut set = HashSet::new();
+        let mut fields = Vec::new();
         if let Some(mut v) = de.config.get_table(&de.key)? {
             // `v: Value<HashMap<String, CV>>`
             for (key, _value) in v.val.drain() {
-                set.insert(KeyKind::CaseSensitive(key));
+                fields.push(KeyKind::CaseSensitive(key));
             }
         }
         if de.config.cli_unstable().advanced_env {
             // `CARGO_PROFILE_DEV_PACKAGE_`
-            let env_pattern = format!("{}_", de.key.as_env_key());
+            let env_prefix = format!("{}_", de.key.as_env_key());
             for env_key in de.config.env.keys() {
-                if env_key.starts_with(&env_pattern) {
+                if env_key.starts_with(&env_prefix) {
                     // `CARGO_PROFILE_DEV_PACKAGE_bar_OPT_LEVEL = 3`
-                    let rest = &env_key[env_pattern.len()..];
+                    let rest = &env_key[env_prefix.len()..];
                     // `rest = bar_OPT_LEVEL`
                     let part = rest.splitn(2, '_').next().unwrap();
                     // `part = "bar"`
-                    set.insert(KeyKind::CaseSensitive(part.to_string()));
+                    fields.push(KeyKind::CaseSensitive(part.to_string()));
                 }
             }
         }
         Ok(ConfigMapAccess {
             de,
-            set_iter: set.into_iter(),
-            next: None,
+            fields,
+            field_index: 0,
         })
     }
 
     fn new_struct(
         de: Deserializer<'config>,
         fields: &'static [&'static str],
     ) -> Result<ConfigMapAccess<'config>, ConfigError> {
-        let mut set = HashSet::new();
-        for field in fields {
-            set.insert(KeyKind::Normal(field.to_string()));
-        }
+        let fields: Vec<KeyKind> = fields
+            .iter()
+            .map(|field| KeyKind::Normal(field.to_string()))
+            .collect();
 
         // Assume that if we're deserializing a struct it exhaustively lists all
         // possible fields on this key that we're *supposed* to use, so take
         // this opportunity to warn about any keys that aren't recognized as
         // fields and warn about them.
         if let Some(mut v) = de.config.get_table(&de.key)? {
             for (t_key, value) in v.val.drain() {
-                if set.contains(&KeyKind::Normal(t_key.to_string())) {
+                if fields.iter().any(|k| match k {
+                    KeyKind::Normal(s) => s == &t_key,
+                    KeyKind::CaseSensitive(s) => s == &t_key,
+                }) {
                     continue;
                 }
                 de.config.shell().warn(format!(
@@ -248,8 +260,8 @@ impl<'config> ConfigMapAccess<'config> {
 
         Ok(ConfigMapAccess {
             de,
-            set_iter: set.into_iter(),
-            next: None,
+            fields,
+            field_index: 0,
         })
     }
 }
@@ -261,30 +273,61 @@ impl<'de, 'config> de::MapAccess<'de> for ConfigMapAccess<'config> {
     where
         K: de::DeserializeSeed<'de>,
     {
-        match self.set_iter.next() {
-            Some(key) => {
-                let name = match &key {
-                    KeyKind::Normal(s) | KeyKind::CaseSensitive(s) => s.as_str(),
-                };
-                let result = seed.deserialize(name.into_deserializer()).map(Some);
-                self.next = Some(key);
-                result
-            }
-            None => Ok(None),
+        if self.field_index >= self.fields.len() {
+            return Ok(None);
         }
+        let field = match &self.fields[self.field_index] {
+            KeyKind::Normal(s) | KeyKind::CaseSensitive(s) => s.as_str(),
+        };
+        seed.deserialize(field.into_deserializer()).map(Some)
     }
 
     fn next_value_seed<V>(&mut self, seed: V) -> Result<V::Value, Self::Error>
     where
         V: de::DeserializeSeed<'de>,
     {
-        match self.next.take().expect("next field missing") {
-            KeyKind::Normal(key) => self.de.key.push(&key),
-            KeyKind::CaseSensitive(key) => self.de.key.push_sensitive(&key),
-        }
+        let field = &self.fields[self.field_index];
+        self.field_index += 1;
+        // Set this as the current key in the deserializer.
+        let field = match field {
+            KeyKind::Normal(field) => {
+                self.de.key.push(&field);
+                field
+            }
+            KeyKind::CaseSensitive(field) => {
+                self.de.key.push_sensitive(&field);
+                field
+            }
+        };
+        // Env vars that are a prefix of another with a dash/underscore cannot
+        // be supported by our serde implementation, so check for them here.
+        // Example:
+        //     CARGO_BUILD_TARGET
+        //     CARGO_BUILD_TARGET_DIR
+        // or
+        //     CARGO_PROFILE_DEV_DEBUG
+        //     CARGO_PROFILE_DEV_DEBUG_ASSERTIONS
+        // The `deserialize_option` method does not know the type of the field.
+        // If the type is an Option<struct> (like
+        // `profile.dev.build-override`), then it needs to check for env vars
+        // starting with CARGO_FOO_BAR_. This is a problem for keys like
+        // CARGO_BUILD_TARGET because checking for a prefix would incorrectly
+        // match CARGO_BUILD_TARGET_DIR. `deserialize_option` would have no
+        // choice but to call `visit_some()` which would then fail if
+        // CARGO_BUILD_TARGET isn't set. So we check for these prefixes and
+        // disallow them here.
+        let env_prefix = format!("{}_", field).replace('-', "_");
+        let env_prefix_ok = !self.fields.iter().any(|field| {
+            let field = match field {
+                KeyKind::Normal(s) | KeyKind::CaseSensitive(s) => s.as_str(),
+            };
+            field.replace('-', "_").starts_with(&env_prefix)
+        });
+
         let result = seed.deserialize(Deserializer {
             config: self.de.config,
             key: self.de.key.clone(),
+            env_prefix_ok,
         });
         self.de.key.pop();
         result

src/cargo/util/config/mod.rs

@@ -35,6 +35,12 @@
 //! tables (because Cargo must fetch all of them), so those do not support
 //! environment variables.
 //!
+//! Try to avoid keys that are a prefix of another with a dash/underscore. For
+//! example `build.target` and `build.target-dir`. This is OK if these are not
+//! structs/maps, but if it is a struct or map, then it will not be able to
+//! read the environment variable due to ambiguity. (See `ConfigMapAccess` for
+//! more details.)
+//!
 //! ## Internal API
 //!
 //! Internally config values are stored with the `ConfigValue` type after they
@@ -520,13 +526,16 @@ impl Config {
         }
     }
 
-    fn has_key(&self, key: &ConfigKey) -> bool {
-        if self.env.get(key.as_env_key()).is_some() {
+    fn has_key(&self, key: &ConfigKey, env_prefix_ok: bool) -> bool {
+        if self.env.contains_key(key.as_env_key()) {
             return true;
         }
-        let env_pattern = format!("{}_", key.as_env_key());
-        if self.env.keys().any(|k| k.starts_with(&env_pattern)) {
-            return true;
+        // See ConfigMapAccess for a description of this.
+        if env_prefix_ok {
+            let env_prefix = format!("{}_", key.as_env_key());
+            if self.env.keys().any(|k| k.starts_with(&env_prefix)) {
+                return true;
+            }
         }
         if let Ok(o_cv) = self.get_cv(key) {
             if o_cv.is_some() {
@@ -1101,6 +1110,7 @@ impl Config {
         let d = Deserializer {
             config: self,
             key: ConfigKey::from_str(key),
+            env_prefix_ok: true,
         };
         T::deserialize(d).map_err(|e| e.into())
     }

src/cargo/util/config/value.rs

@@ -51,10 +51,14 @@ pub(crate) const DEFINITION_FIELD: &str = "$__cargo_private_definition";
 pub(crate) const NAME: &str = "$__cargo_private_Value";
 pub(crate) static FIELDS: [&str; 2] = [VALUE_FIELD, DEFINITION_FIELD];
 
+/// Location where a config value is defined.
 #[derive(Clone, Debug, Eq)]
 pub enum Definition {
+    /// Defined in a `.cargo/config`, includes the path to the file.
     Path(PathBuf),
+    /// Defined in an environment variable, includes the environment key.
     Environment(String),
+    /// Passed in on the command line.
     Cli,
 }
 

tests/testsuite/build.rs

@@ -2806,6 +2806,11 @@ fn custom_target_dir_env() {
     assert!(p.root().join("foo/target/debug").join(&exe_name).is_file());
     assert!(p.root().join("target/debug").join(&exe_name).is_file());
 
+    p.cargo("build")
+        .env("CARGO_BUILD_TARGET_DIR", "foo2/target")
+        .run();
+    assert!(p.root().join("foo2/target/debug").join(&exe_name).is_file());
+
     fs::create_dir(p.root().join(".cargo")).unwrap();
     File::create(p.root().join(".cargo/config"))
         .unwrap()

tests/testsuite/config.rs

@@ -424,6 +424,32 @@ lto = false
     );
 }
 
+#[cargo_test]
+fn profile_env_var_prefix() {
+    // Check for a bug with collision on DEBUG vs DEBUG_ASSERTIONS.
+    let config = ConfigBuilder::new()
+        .env("CARGO_PROFILE_DEV_DEBUG_ASSERTIONS", "false")
+        .build();
+    let p: toml::TomlProfile = config.get("profile.dev").unwrap();
+    assert_eq!(p.debug_assertions, Some(false));
+    assert_eq!(p.debug, None);
+
+    let config = ConfigBuilder::new()
+        .env("CARGO_PROFILE_DEV_DEBUG", "1")
+        .build();
+    let p: toml::TomlProfile = config.get("profile.dev").unwrap();
+    assert_eq!(p.debug_assertions, None);
+    assert_eq!(p.debug, Some(toml::U32OrBool::U32(1)));
+
+    let config = ConfigBuilder::new()
+        .env("CARGO_PROFILE_DEV_DEBUG_ASSERTIONS", "false")
+        .env("CARGO_PROFILE_DEV_DEBUG", "1")
+        .build();
+    let p: toml::TomlProfile = config.get("profile.dev").unwrap();
+    assert_eq!(p.debug_assertions, Some(false));
+    assert_eq!(p.debug, Some(toml::U32OrBool::U32(1)));
+}
+
 #[cargo_test]
 fn config_deserialize_any() {
     // Some tests to exercise deserialize_any for deserializers that need to
@@ -1103,3 +1129,53 @@ Caused by:
   expected string but found integer in list",
     );
 }
+
+#[cargo_test]
+fn struct_with_opt_inner_struct() {
+    // Struct with a key that is Option of another struct.
+    // Check that can be defined with environment variable.
+    #[derive(Deserialize)]
+    struct Inner {
+        value: Option<i32>,
+    }
+    #[derive(Deserialize)]
+    struct Foo {
+        inner: Option<Inner>,
+    }
+    let config = ConfigBuilder::new()
+        .env("CARGO_FOO_INNER_VALUE", "12")
+        .build();
+    let f: Foo = config.get("foo").unwrap();
+    assert_eq!(f.inner.unwrap().value.unwrap(), 12);
+}
+
+#[cargo_test]
+fn overlapping_env_config() {
+    // Issue where one key is a prefix of another.
+    #[derive(Deserialize)]
+    #[serde(rename_all = "kebab-case")]
+    struct Ambig {
+        debug: Option<u32>,
+        debug_assertions: Option<bool>,
+    }
+    let config = ConfigBuilder::new()
+        .env("CARGO_AMBIG_DEBUG_ASSERTIONS", "true")
+        .build();
+
+    let s: Ambig = config.get("ambig").unwrap();
+    assert_eq!(s.debug_assertions, Some(true));
+    assert_eq!(s.debug, None);
+
+    let config = ConfigBuilder::new().env("CARGO_AMBIG_DEBUG", "0").build();
+    let s: Ambig = config.get("ambig").unwrap();
+    assert_eq!(s.debug_assertions, None);
+    assert_eq!(s.debug, Some(0));
+
+    let config = ConfigBuilder::new()
+        .env("CARGO_AMBIG_DEBUG", "1")
+        .env("CARGO_AMBIG_DEBUG_ASSERTIONS", "true")
+        .build();
+    let s: Ambig = config.get("ambig").unwrap();
+    assert_eq!(s.debug_assertions, Some(true));
+    assert_eq!(s.debug, Some(1));
+}