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

Magic module #12

Closed
wants to merge 8 commits into from
Closed

Magic module #12

wants to merge 8 commits into from

Conversation

Blacksyke
Copy link

Port of the magic module. Note that building this feature requires libmagic in your Rust library path.

I've implemented both magic.type() and magic.mime_type(), no rule changes should be needed. After chatting with @wxsBSD, we decided it was best to add a minimal protobuf specification rather than hack the build scripts to account for modules that only have exported functions and return no data, like this one.

Right now, I'm mostly asking for feedback. A notable omission from this implementation is the cache design present in the C version. I imagine adding this would still be a net performance gain, but I want to make sure this design seems reasonable first.

cargo test --package yara-x --lib --features magic-module -- modules::tests::magic::e2e_test --exact --nocapture

   Compiling yara-x v0.1.0 (yara-x/yara-x)
    Finished test [unoptimized + debuginfo] target(s) in 1.76s
     Running unittests src/lib.rs (target/debug/deps/yara_x-56a7c9914ffab9dd)

running 1 test
test modules::tests::magic::e2e_test ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 24 filtered out; finished in 0.14s

@google-cla
Copy link

google-cla bot commented Mar 6, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@plusvic
Copy link
Member

plusvic commented Mar 17, 2023

This looks good to me. The cache system is important but I'm not sure how to implement, maybe we need to provide a mechanism so that modules can store arbitrary data in the ScanContext and retrieve it later. Also, run rustfmt with the rustfmt.toml file in the root directory.

@Blacksyke Blacksyke marked this pull request as draft April 7, 2023 03:31
@Blacksyke
Copy link
Author

RFD

Having been stymied in my previous testing file layout by module scope problems, I have resolved to move this module into a directory with tests alongside, as we do with the scanner component. This is probably slightly better overall, as larger modules spanning multiple files will want their own directory anyway. What do we think? I still hate having to specify the fact that that there are tests in the module file itself, but I can't find anything more elegant that actually works right now.

If we're OK with requiring tests for modules, we could probably enforce this when writing modules.rs, i.e. having the build script break if it doesn't find <module>_tests.

The caching system remains #todo at this time.

cargo test --package yara-x --lib --features magic-module -- modules::magic::tests --nocapture

running 3 tests
test modules::magic::tests::get_filetype ... ok
test modules::magic::tests::get_mimetype ... ok
test modules::magic::tests::e2e_test ... ok

test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 32 filtered out; finished in 0.12s

@plusvic
Copy link
Member

plusvic commented Apr 11, 2023

I think that having the module's test within the module's itself is ok, as this is a well-known Rust pattern. For small modules having a single file is also ok, while larger ones can have its own directory, like in this case.

@plusvic plusvic marked this pull request as ready for review March 12, 2024 16:43
plusvic added a commit that referenced this pull request Mar 12, 2024
Mostly a copy of #12. After too many changes since the PR, it's easier to commit the code again than resolving the merge conflicts.
plusvic added a commit that referenced this pull request Mar 13, 2024
Mostly a copy of #12. After too many changes since the PR, it's easier to commit the code again than resolving the merge conflicts.
@plusvic
Copy link
Member

plusvic commented Mar 13, 2024

Already merged in #91.

@plusvic plusvic closed this Mar 13, 2024
TommYDeeee pushed a commit to TommYDeeee/yara-x that referenced this pull request Sep 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants