Merge pull request #620 from epage/negative

fix: Dont crash on bad negative bounds
rust-cli · Dec 19, 2024 · e79a41a · e79a41a
2 parents eec8d6d + da18a36
commit e79a41a
Show file tree

Hide file tree

Showing 4 changed files with 104 additions and 28 deletions.
diff --git a/src/error.rs b/src/error.rs
@@ -174,7 +174,7 @@ impl ConfigError {
 }
 
 /// Alias for a `Result` with the error type set to `ConfigError`.
-pub(crate) type Result<T> = result::Result<T, ConfigError>;
+pub(crate) type Result<T, E = ConfigError> = result::Result<T, E>;
 
 // Forward Debug to Display for readable panic! messages
 impl fmt::Debug for ConfigError {

diff --git a/src/path/mod.rs b/src/path/mod.rs
@@ -40,11 +40,14 @@ impl std::fmt::Display for ParseError {
 
 impl std::error::Error for ParseError {}
 
-fn sindex_to_uindex(index: isize, len: usize) -> usize {
+/// Convert a relative index into an absolute index
+fn abs_index(index: isize, len: usize) -> Result<usize, usize> {
     if index >= 0 {
-        index as usize
+        Ok(index as usize)
+    } else if let Some(index) = len.checked_sub(index.unsigned_abs()) {
+        Ok(index)
     } else {
-        len - index.unsigned_abs()
+        Err((len as isize + index).unsigned_abs())
     }
 }
 
@@ -80,13 +83,8 @@ impl Expression {
             Self::Subscript(expr, index) => match expr.get(root) {
                 Some(value) => match value.kind {
                     ValueKind::Array(ref array) => {
-                        let index = sindex_to_uindex(index, array.len());
-
-                        if index >= array.len() {
-                            None
-                        } else {
-                            Some(&array[index])
-                        }
+                        let index = abs_index(index, array.len()).ok()?;
+                        array.get(index)
                     }
 
                     _ => None,
@@ -141,7 +139,7 @@ impl Expression {
 
                     match value.kind {
                         ValueKind::Array(ref mut array) => {
-                            let index = sindex_to_uindex(index, array.len());
+                            let index = abs_index(index, array.len()).ok()?;
 
                             if index >= array.len() {
                                 array.resize(index + 1, Value::new(None, ValueKind::Nil));
@@ -216,10 +214,21 @@ impl Expression {
                     }
 
                     if let ValueKind::Array(ref mut array) = parent.kind {
-                        let uindex = sindex_to_uindex(index, array.len());
-                        if uindex >= array.len() {
-                            array.resize(uindex + 1, Value::new(None, ValueKind::Nil));
-                        }
+                        let uindex = match abs_index(index, array.len()) {
+                            Ok(uindex) => {
+                                if uindex >= array.len() {
+                                    array.resize(uindex + 1, Value::new(None, ValueKind::Nil));
+                                }
+                                uindex
+                            }
+                            Err(insertion) => {
+                                array.splice(
+                                    0..0,
+                                    (0..insertion).map(|_| Value::new(None, ValueKind::Nil)),
+                                );
+                                0
+                            }
+                        };
 
                         array[uindex] = value;
                     }

diff --git a/tests/testsuite/errors.rs b/tests/testsuite/errors.rs
@@ -3,6 +3,52 @@ use snapbox::{assert_data_eq, str};
 
 use config::{Config, ConfigError, File, FileFormat, Map, Value};
 
+#[test]
+#[cfg(feature = "json")]
+fn test_error_path_index_bounds() {
+    let c = Config::builder()
+        .add_source(File::from_str(
+            r#"
+{
+  "arr": [1]
+}
+"#,
+            FileFormat::Json,
+        ))
+        .build()
+        .unwrap();
+
+    let res = c.get::<usize>("arr[2]");
+    assert!(res.is_err());
+    assert_data_eq!(
+        res.unwrap_err().to_string(),
+        str![[r#"configuration property "arr[2]" not found"#]]
+    );
+}
+
+#[test]
+#[cfg(feature = "json")]
+fn test_error_path_index_negative_bounds() {
+    let c = Config::builder()
+        .add_source(File::from_str(
+            r#"
+{
+  "arr": []
+}
+"#,
+            FileFormat::Json,
+        ))
+        .build()
+        .unwrap();
+
+    let res = c.get::<usize>("arr[-1]");
+    assert!(res.is_err());
+    assert_data_eq!(
+        res.unwrap_err().to_string(),
+        str![[r#"configuration property "arr[-1]" not found"#]]
+    );
+}
+
 #[test]
 #[cfg(feature = "json")]
 fn test_error_parse() {

diff --git a/tests/testsuite/set.rs b/tests/testsuite/set.rs
@@ -65,42 +65,63 @@ fn test_set_scalar_path() {
 #[cfg(feature = "json")]
 fn test_set_arr_path() {
     let config = Config::builder()
-        .set_override("items[0].name", "Ivan")
+        .set_override("present[0].name", "Ivan")
         .unwrap()
-        .set_override("data[0].things[1].name", "foo")
+        .set_override("absent[0].things[1].name", "foo")
         .unwrap()
-        .set_override("data[0].things[1].value", 42)
+        .set_override("absent[0].things[1].value", 42)
         .unwrap()
-        .set_override("data[1]", 0)
+        .set_override("absent[1]", 0)
         .unwrap()
-        .set_override("items[2]", "George")
+        .set_override("present[2]", "George")
+        .unwrap()
+        .set_override("reverse[-1]", "Bob")
+        .unwrap()
+        .set_override("reverse[-2]", "Alice")
+        .unwrap()
+        .set_override("empty[-1]", "Bob")
+        .unwrap()
+        .set_override("empty[-2]", "Alice")
         .unwrap()
         .add_source(File::from_str(
             r#"
 {
-  "items": [
+  "present": [
     {
       "name": "1"
     },
     {
       "name": "2"
     }
-  ]
+  ],
+  "reverse": [
+    {
+      "name": "l1"
+    },
+    {
+      "name": "l2"
+    }
+  ],
+  "empty": []
 }
 "#,
             FileFormat::Json,
         ))
         .build()
         .unwrap();
 
-    assert_eq!(config.get("items[0].name").ok(), Some("Ivan".to_owned()));
+    assert_eq!(config.get("present[0].name").ok(), Some("Ivan".to_owned()));
     assert_eq!(
-        config.get("data[0].things[1].name").ok(),
+        config.get("absent[0].things[1].name").ok(),
         Some("foo".to_owned())
     );
-    assert_eq!(config.get("data[0].things[1].value").ok(), Some(42));
-    assert_eq!(config.get("data[1]").ok(), Some(0));
-    assert_eq!(config.get("items[2]").ok(), Some("George".to_owned()));
+    assert_eq!(config.get("absent[0].things[1].value").ok(), Some(42));
+    assert_eq!(config.get("absent[1]").ok(), Some(0));
+    assert_eq!(config.get("present[2]").ok(), Some("George".to_owned()));
+    assert_eq!(config.get("reverse[1]").ok(), Some("Bob".to_owned()));
+    assert_eq!(config.get("reverse[0]").ok(), Some("Alice".to_owned()));
+    assert_eq!(config.get("empty[1]").ok(), Some("Bob".to_owned()));
+    assert_eq!(config.get("empty[0]").ok(), Some("Alice".to_owned()));
 }
 
 #[test]