Repository: rust-cli/config-rs (language: rust)

Issue:
panic on negative out-of-bounds array path expressions
The following program panics
```rust
// panics(release) with message: 'index out of bounds: the len is 0 but the index is 18446744073709551615'
// panics(debug) with message: 'attempt to subtract with overflow'
fn main() {
    let _cfg = config::Config::builder()
        // uncomment to prevent panic
        // .set_override("a[0]", "0") .unwrap()
        .set_override("a[-1]", "0")
        .unwrap()
        .build()
        .unwrap();
}
```

If the out-of-bounds index is positive, then the array is resized implicitly and initialized with new 'nil' values. The resizing logic is flawed when negative indices are involved.

If you change the example to use an index of `-2`, then the (release mode) panic that you get becomes
> thread 'main' panicked at alloc/src/raw_vec.rs:24:5:
capacity overflow

I'm not sure how one should expect the array to be resized when a negative index is out of bounds (or if it should be accepted at all), but am sure that it shouldn't result in a panic.



Discussion / hints:

Fix patch:
diff --git a/src/error.rs b/src/error.rs
index 13e13b82..2d738b94 100644
--- 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
index 3c0b4213..e98e92a3 100644
--- 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;
                     }


Test patch:
diff --git a/tests/testsuite/errors.rs b/tests/testsuite/errors.rs
index 983a3c61..74edd41a 100644
--- 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
index 4a02b5c0..c5d29de0 100644
--- a/tests/testsuite/set.rs
+++ b/tests/testsuite/set.rs
@@ -65,27 +65,44 @@ 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,
@@ -93,14 +110,18 @@ fn test_set_arr_path() {
         .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]