Merge pull request #27 from James-LG/james/stack

fix: Ensure stack does not overflow on Windows

.github/workflows/rust.yml

@@ -1,6 +1,7 @@
 name: Rust
 
 on:
+  workflow_dispatch:
   push:
     branches: [ "master" ]
   pull_request:
@@ -11,12 +12,21 @@ env:
 
 jobs:
   build:
-
     runs-on: ubuntu-latest
-
     steps:
     - uses: actions/checkout@v3
     - name: Build
       run: cargo build --verbose
     - name: Run tests
       run: cargo test --verbose
+
+  stack_overflow_tests:
+    runs-on: windows-latest
+    steps:
+    - uses: actions/checkout@v3
+    - name: Install Rust Nightly
+      run: rustup toolchain install nightly
+    - name: Build
+      run: cargo build
+    - name: Run tests
+      run: cargo test --test run_stack_overflow_tests -- --include-ignored --test-threads=1

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "skyscraper"
-version = "0.6.0"
+version = "0.6.1-beta.0"
 authors = ["James La Novara-Gsell <[email protected]>"]
 edition = "2021"
 description = "XPath for HTML web scraping"
@@ -28,6 +28,7 @@ criterion = "0.5.1"
 mockall = "0.12.0"
 indoc = "2"
 proptest = "1.3.1"
+regex = "1.10.3"
 
 [[bench]]
 name = "benchmarks"

src/xpath/grammar/expressions/sequence_expressions/constructing_sequences.rs

@@ -25,17 +25,17 @@ pub fn range_expr(input: &str) -> Res<&str, RangeExpr> {
         (
             next_input,
             RangeExpr {
-                expr: res.0,
-                to_expr: res.1.map(|res| res.1),
+                expr: Box::new(res.0),
+                to_expr: res.1.map(|res| Box::new(res.1)),
             },
         )
     })
 }
 
 #[derive(PartialEq, Debug, Clone)]
 pub struct RangeExpr {
-    pub expr: AdditiveExpr,
-    pub to_expr: Option<AdditiveExpr>,
+    pub expr: Box<AdditiveExpr>,
+    pub to_expr: Option<Box<AdditiveExpr>>,
 }
 
 impl Display for RangeExpr {

tests/run_stack_overflow_tests.rs

@@ -0,0 +1,126 @@
+//! These tests are ignored because they all operate on a single project.
+//! Run these tests with `cargo test --test run_stack_overflow_tests -- --include-ignored --test-threads=1`
+//! to ensure there is only one thread when running them.
+
+use regex::Regex;
+use std::{path::PathBuf, process::Command};
+
+/// Runs the `stack_overflow_tests` project.
+/// See the [README](stack_overflow_tests/README.md) for more details.
+#[test]
+#[ignore]
+fn test_stack_overflow_project() {
+    /* Stage 1 - Clean the stack_overflow_project */
+    let mut stack_overflow_manifest_path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
+    stack_overflow_manifest_path.push("tests/stack_overflow_tests/Cargo.toml");
+
+    let output = Command::new("cargo")
+        .arg("clean")
+        .arg("--manifest-path")
+        .arg(
+            stack_overflow_manifest_path
+                .clone()
+                .into_os_string()
+                .into_string()
+                .unwrap(),
+        )
+        .output()
+        .expect("failed to execute stack overflow tests");
+
+    assert!(
+        output.status.success(),
+        "{}\n{}",
+        String::from_utf8_lossy(&output.stdout),
+        String::from_utf8_lossy(&output.stderr)
+    );
+
+    /* Stage 2 - Run it */
+    let output = Command::new("cargo")
+        .arg("run")
+        .arg("--manifest-path")
+        .arg(
+            stack_overflow_manifest_path
+                .into_os_string()
+                .into_string()
+                .unwrap(),
+        )
+        .output()
+        .expect("failed to execute stack overflow tests");
+
+    assert!(
+        output.status.success(),
+        "{}\n{}",
+        String::from_utf8_lossy(&output.stdout),
+        String::from_utf8_lossy(&output.stderr)
+    );
+}
+
+/// Ensures that no structs are over 1024 bytes since it might cause stack overflows.
+/// This is an arbitrary number subject to change.
+#[test]
+#[ignore]
+fn test_struct_size() {
+    /* Stage 1 - Clean the stack_overflow_project */
+    let mut stack_overflow_manifest_path = PathBuf::from(env!("CARGO_MANIFEST_DIR"));
+    stack_overflow_manifest_path.push("tests/stack_overflow_tests/Cargo.toml");
+
+    let output = Command::new("cargo")
+        .arg("clean")
+        .arg("--manifest-path")
+        .arg(
+            stack_overflow_manifest_path
+                .clone()
+                .into_os_string()
+                .into_string()
+                .unwrap(),
+        )
+        .output()
+        .expect("failed to execute stack overflow tests");
+
+    assert!(
+        output.status.success(),
+        "{}\n{}",
+        String::from_utf8_lossy(&output.stdout),
+        String::from_utf8_lossy(&output.stderr)
+    );
+
+    /* Stage 2 - Compile it with print-type-sizes */
+    let output = Command::new("rustup")
+        .arg("run")
+        .arg("nightly")
+        .arg("cargo")
+        .arg("rustc")
+        .arg("--manifest-path")
+        .arg(
+            stack_overflow_manifest_path
+                .clone()
+                .into_os_string()
+                .into_string()
+                .unwrap(),
+        )
+        .arg("--")
+        .arg("-Zprint-type-sizes")
+        .output()
+        .expect("failed to execute stack overflow tests");
+
+    let stdout = String::from_utf8_lossy(&output.stdout);
+
+    assert!(
+        output.status.success(),
+        "{}\n{}",
+        stdout,
+        String::from_utf8_lossy(&output.stderr)
+    );
+
+    /* Stage 3 - Ensure the structs are not too large */
+    let lines = stdout.lines();
+
+    let re = Regex::new("(?<bytes>[0-9]*) bytes").unwrap();
+    for line in lines {
+        if line.contains("type: ") {
+            let capture = re.captures(line).unwrap();
+            let num_bytes = capture["bytes"].parse::<usize>().unwrap();
+            assert!(num_bytes < 1000, "Struct is too large {}", line);
+        }
+    }
+}

tests/stack_overflow_tests/.gitignore

@@ -0,0 +1 @@
+/target

tests/stack_overflow_tests/Cargo.toml

@@ -0,0 +1,9 @@
+[package]
+name = "skyscraper_stack_overflow_tests"
+version = "0.1.0"
+edition = "2021"
+
+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
+skyscraper = { path = "../.." }

tests/stack_overflow_tests/README.md

@@ -0,0 +1,24 @@
+# stack_overflow_tests
+
+The structs in this library are large enough that the recursive parsing of XPath expressions can cause stack overflows.
+
+This has only ever been reproduced on Windows 10 and only when building a binary in debug mode.
+Notably, this cannot be reproduced using an actual test, thus this mini project was created.
+
+This project is tested by the main project by way of `tests/run_stack_overflow_tests.rs`.
+
+A typical failure of this tests appears as follows:
+
+```rs
+---- does_this_run stdout ----
+thread 'does_this_run' panicked at tests\run_stack_overflow_tests.rs:20:5:
+
+   Compiling skyscraper_stack_overflow_tests v0.1.0 (D:\code\Skyscraper\tests\stack_overflow_tests)
+    Finished dev [unoptimized + debuginfo] target(s) in 0.30s
+     Running `tests\stack_overflow_tests\target\debug\skyscraper_stack_overflow_tests.exe`
+
+thread 'main' has overflowed its stack
+error: process didn't exit successfully: `tests\stack_overflow_tests\target\debug\skyscraper_stack_overflow_tests.exe` (exit code: 0xc00000fd, STATUS_STACK_OVERFLOW)
+
+note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
+```

tests/stack_overflow_tests/src/main.rs

@@ -0,0 +1,10 @@
+fn main() {
+    // arrange
+    let xpath_text = r###"//head//script[not(@src) and not(contains(., "gtag"))]"###;
+
+    // act
+    let xpath = skyscraper::xpath::parse(xpath_text).unwrap();
+
+    // assert
+    assert_eq!(xpath.to_string(), xpath_text);
+}