Fix potential panics in expression engine (#31)

* Fix potential panics in min/max and division by zero

* Update GitHub Actions and fix potential panics

* Fix panics in min/max/div and update GitHub Actions

---------

Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>

Author: google-labs-jules[bot]

.github/workflows/Build.yml

@@ -22,7 +22,7 @@ jobs:
           - aarch64-unknown-linux-gnu
       fail-fast: false
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - uses: dtolnay/rust-toolchain@master
         with:
           toolchain: ${{ matrix.version }}
@@ -34,7 +34,7 @@ jobs:
       - name: ignored test
         run: cargo test -- --ignored || true
         if: matrix.version == 'nightly'
-      - uses: actions/upload-artifact@v3
+      - uses: actions/upload-artifact@v4
         if: failure()
         with:
           name: linux ${{ matrix.version }}
@@ -55,15 +55,15 @@ jobs:
           - x86_64-pc-windows-msvc
       fail-fast: false
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - uses: dtolnay/rust-toolchain@master
         with:
           toolchain: ${{ matrix.version }}
       - name: cache
         uses: Swatinem/rust-cache@v2
       - name: test
         run: cargo test
-      - uses: actions/upload-artifact@v3
+      - uses: actions/upload-artifact@v4
         if: failure()
         with:
           name: windows ${{ matrix.version }}
@@ -81,15 +81,15 @@ jobs:
           - x86_64-apple-darwin	
       fail-fast: false
     steps:
-      - uses: actions/checkout@v3
+      - uses: actions/checkout@v4
       - uses: dtolnay/rust-toolchain@master
         with:
           toolchain: ${{ matrix.version }}
       - name: cache
         uses: Swatinem/rust-cache@v2
       - name: test
         run: cargo test
-      - uses: actions/upload-artifact@v3
+      - uses: actions/upload-artifact@v4
         #if: failure()
         with:
           name: mac ${{ matrix.version }}

.github/workflows/coverage.yml

@@ -10,14 +10,14 @@ jobs:
       options:                  --security-opt seccomp=unconfined
     steps:
       - name:                   Checkout repository
-        uses:                   actions/checkout@v3
+        uses:                   actions/checkout@v4
 
       - name:                   Generate code coverage
         run: |
           cargo tarpaulin --verbose --all-features --workspace --timeout 120 --out Xml
 
       - name:                   Upload to codecov.io
-        uses:                   codecov/codecov-action@v2
+        uses:                   codecov/codecov-action@v4
         with:
           token:                ${{secrets.CODECOV_TOKEN}} # not required for public repos
           fail_ci_if_error:     true

.github/workflows/rust-clippy.yml

@@ -28,15 +28,13 @@ jobs:
       actions: read # only required for a private repository by github/codeql-action/upload-sarif to get the Action run status
     steps:
       - name: Checkout code
-        uses: actions/checkout@v2
+        uses: actions/checkout@v4
 
       - name: Install Rust toolchain
-        uses: actions-rs/toolchain@16499b5e05bf2e26879000db0c1d13f7e13fa3af #@v1
+        uses: dtolnay/rust-toolchain@master
         with:
-          profile: minimal
           toolchain: stable
           components: clippy
-          override: true
 
       - name: Install required cargo
         run: cargo install clippy-sarif sarif-fmt
@@ -49,7 +47,7 @@ jobs:
         continue-on-error: true
 
       - name: Upload analysis results to GitHub
-        uses: github/codeql-action/upload-sarif@v2
+        uses: github/codeql-action/upload-sarif@v3
         with:
           sarif_file: rust-clippy-results.sarif
           wait-for-processing: true

.github/workflows/rust.yml

@@ -15,7 +15,7 @@ jobs:
     runs-on: ubuntu-latest
 
     steps:
-    - uses: actions/checkout@v3
+    - uses: actions/checkout@v4
     - name: Build
       run: cargo build --verbose
     - name: Run tests

src/error.rs

@@ -30,6 +30,8 @@ pub enum Error {
     InvalidInteger,
     InvalidFloat,
     ExpectBinOpToken,
+    ParamEmpty(String),
+    DivByZero,
 }
 
 #[cfg(not(tarpaulin_include))]
@@ -67,6 +69,8 @@ impl fmt::Display for Error {
             InvalidInteger => write!(f, "invalid integer"),
             InvalidFloat => write!(f, "invalid float"),
             ExpectBinOpToken => write!(f, "expect bin op token"),
+            ParamEmpty(name) => write!(f, "param empty: {}", name),
+            DivByZero => write!(f, "division by zero"),
         }
     }
 }

src/function.rs

@@ -30,7 +30,11 @@ impl InnerFunctionManager {
                         min = Some(num);
                     }
                 }
-                Ok(Value::Number(min.unwrap()))
+                if let Some(min) = min {
+                    Ok(Value::Number(min))
+                } else {
+                    Err(Error::ParamEmpty("min".to_string()))
+                }
             }),
         );
 
@@ -44,7 +48,11 @@ impl InnerFunctionManager {
                         max = Some(num);
                     }
                 }
-                Ok(Value::Number(max.unwrap()))
+                if let Some(max) = max {
+                    Ok(Value::Number(max))
+                } else {
+                    Err(Error::ParamEmpty("max".to_string()))
+                }
             }),
         );
 

src/lib.rs

@@ -257,4 +257,56 @@ mod tests {
         assert!(ans.is_ok());
         assert_eq!(ans.unwrap(), Value::from(89));
     }
+
+    #[test]
+    fn test_min_no_panic() {
+        use crate::error::Error;
+        let input = "min()";
+        let ctx = create_context!();
+        let ans = execute(input, ctx);
+        assert!(ans.is_err());
+        match ans.unwrap_err() {
+            Error::ParamEmpty(name) => assert_eq!(name, "min"),
+            _ => panic!("Expected Error::ParamEmpty"),
+        }
+    }
+
+    #[test]
+    fn test_max_no_panic() {
+        use crate::error::Error;
+        let input = "max()";
+        let ctx = create_context!();
+        let ans = execute(input, ctx);
+        assert!(ans.is_err());
+        match ans.unwrap_err() {
+            Error::ParamEmpty(name) => assert_eq!(name, "max"),
+            _ => panic!("Expected Error::ParamEmpty"),
+        }
+    }
+
+    #[test]
+    fn test_div_zero_no_panic() {
+        use crate::error::Error;
+        let input = "5 / 0";
+        let ctx = create_context!();
+        let ans = execute(input, ctx);
+        assert!(ans.is_err());
+        match ans.unwrap_err() {
+            Error::DivByZero => (),
+            _ => panic!("Expected Error::DivByZero"),
+        }
+    }
+
+    #[test]
+    fn test_mod_zero_no_panic() {
+        use crate::error::Error;
+        let input = "5 % 0";
+        let ctx = create_context!();
+        let ans = execute(input, ctx);
+        assert!(ans.is_err());
+        match ans.unwrap_err() {
+            Error::DivByZero => (),
+            _ => panic!("Expected Error::DivByZero"),
+        }
+    }
 }

src/operator.rs

@@ -69,8 +69,18 @@ impl InfixOpManager {
                         "+=" => a += b,
                         "-=" => a -= b,
                         "*=" => a *= b,
-                        "/=" => a /= b,
-                        "%=" => a %= b,
+                        "/=" => {
+                            if b.is_zero() {
+                                return Err(Error::DivByZero);
+                            }
+                            a /= b
+                        }
+                        "%=" => {
+                            if b.is_zero() {
+                                return Err(Error::DivByZero);
+                            }
+                            a %= b
+                        }
                         _ => (),
                     }
                     Ok(Value::Number(a))
@@ -189,8 +199,18 @@ impl InfixOpManager {
                         "+" => a += b,
                         "-" => a -= b,
                         "*" => a *= b,
-                        "/" => a /= b,
-                        "%" => a %= b,
+                        "/" => {
+                            if b.is_zero() {
+                                return Err(Error::DivByZero);
+                            }
+                            a /= b
+                        }
+                        "%" => {
+                            if b.is_zero() {
+                                return Err(Error::DivByZero);
+                            }
+                            a %= b
+                        }
                         _ => (),
                     }
                     Ok(Value::from(a))