Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Arbitrary to Glob #2720

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
19 changes: 19 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -215,3 +215,22 @@ jobs:
env:
RUSTDOCFLAGS: -D warnings
run: cargo doc --no-deps --document-private-items --workspace

fuzz_testing:
name: Run Fuzz Tests
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4

- name: Install packages (Ubuntu)
run: |
sudo apt-get update
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

libFuzzer requires LLVM sanitizer support and a C++ compiler with C++ 11 support. It is required to install the fuzzer and to compile the targets. Please see https://rust-fuzz.github.io/book/cargo-fuzz/setup.html for more information.

sudo apt-get install g++ --yes
- uses: dtolnay/rust-toolchain@nightly
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is nightly needed here? If not, please use @master like the rest of the jobs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are not concerned with running the fuzz tests in CI then we don't need the nightly build. I'll change this to match the others.


- run: cargo fuzz_install
working-directory: fuzz

- run: ./scripts/run-all.sh
working-directory: fuzz
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For CI, I don't think we need to actually run fuzzing. Let's just make sure all of the fuzz targets build.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'll update this to simply run a cargo check in the fuzz directory.

21 changes: 21 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ exclude = [
"/pkg/brew",
"/benchsuite/",
"/scripts/",
"/crates/fuzz",
]
build = "build.rs"
autotests = false
Expand Down
2 changes: 1 addition & 1 deletion ci/ubuntu-install-packages
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,4 @@ if ! command -V sudo; then
fi
sudo apt-get update
sudo apt-get install -y --no-install-recommends \
zsh xz-utils liblz4-tool musl-tools brotli zstd
zsh xz-utils liblz4-tool musl-tools brotli zstd g++
2 changes: 2 additions & 0 deletions crates/globset/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ bench = false

[dependencies]
aho-corasick = "1.1.1"
arbitrary = { version = "1.3.2", optional = true, features = ["derive"] }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like implicitly depending on crate names for features. Could you explicitly add an arbitrary feature? Example: https://github.com/rust-lang/regex/blob/0c0990399270277832fbb5b91a1fa118e6f63dba/regex-syntax/Cargo.toml#L18

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add docs for this feature in the crate docs, e.g., https://docs.rs/regex-syntax/latest/regex_syntax/#crate-features

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you be ok with me adding a fuzz folder with a simple fuzz test?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably fine. But it needs to be tested in CI.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @BurntSushi. I've setup a fuzz directory and added a fuzz_testing action to ci.yml. Let me know if you want to adjust the duration for the fuzz testing as this can be modified. I've also addressed the other comments above.

bstr = { version = "1.6.2", default-features = false, features = ["std"] }
log = { version = "0.4.20", optional = true }
serde = { version = "1.0.188", optional = true }
Expand All @@ -41,6 +42,7 @@ serde_json = "1.0.107"

[features]
default = ["log"]
arbitrary = ["dep:arbitrary"]
# DEPRECATED. It is a no-op. SIMD is done automatically through runtime
# dispatch.
simd-accel = []
Expand Down
4 changes: 4 additions & 0 deletions crates/globset/src/glob.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ impl MatchStrategy {
/// It cannot be used directly to match file paths, but it can be converted
/// to a regular expression string or a matcher.
#[derive(Clone, Debug, Eq)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
pub struct Glob {
glob: String,
re: String,
Expand Down Expand Up @@ -193,6 +194,7 @@ pub struct GlobBuilder<'a> {
}

#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
struct GlobOptions {
/// Whether to match case insensitively.
case_insensitive: bool,
Expand All @@ -219,6 +221,7 @@ impl GlobOptions {
}

#[derive(Clone, Debug, Default, Eq, PartialEq)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
struct Tokens(Vec<Token>);

impl std::ops::Deref for Tokens {
Expand All @@ -235,6 +238,7 @@ impl std::ops::DerefMut for Tokens {
}

#[derive(Clone, Debug, Eq, PartialEq)]
#[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
enum Token {
Literal(char),
Any,
Expand Down
13 changes: 13 additions & 0 deletions crates/globset/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,19 @@ globs as matching.

This example shows how to match a single glob against a single file path.


# Crate features
johnsonw marked this conversation as resolved.
Show resolved Hide resolved

This crate includes optional features that can be enabled if necessary. These features are
johnsonw marked this conversation as resolved.
Show resolved Hide resolved
not required but may be useful depending on the use case.

The following features are available:

* **arbitrary** -
Enabling this feature introduces a public dependency on the
[`arbitrary`](https://crates.io/crates/arbitrary)
crate. Namely, it implements the `Arbitrary` trait from that crate for the
[`Glob`] type. This feature is disabled by default.
```
use globset::Glob;

Expand Down
4 changes: 4 additions & 0 deletions fuzz/.cargo/config.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[alias]
johnsonw marked this conversation as resolved.
Show resolved Hide resolved
fuzz_install = "install cargo-fuzz"
fuzz_list = "fuzz list"
fuzz_run = "fuzz run"
johnsonw marked this conversation as resolved.
Show resolved Hide resolved
4 changes: 4 additions & 0 deletions fuzz/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
target
corpus
artifacts
coverage
188 changes: 188 additions & 0 deletions fuzz/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 25 additions & 0 deletions fuzz/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
[package]
name = "fuzz"
version = "0.0.1"
publish = false
edition = "2021"

[package.metadata]
cargo-fuzz = true

[dependencies]
libfuzzer-sys = "0.4"
globset = { path = "../crates/globset", features = ["arbitrary"] }

# Prevent this from interfering with workspaces
[workspace]
members = ["."]

[profile.release]
debug = 1

[[bin]]
name = "fuzz_glob"
path = "fuzz_targets/fuzz_glob.rs"
test = false
doc = false