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

Clam 2256 add alz support #1183

Merged
merged 3 commits into from
Apr 15, 2024

Conversation

ragusaa
Copy link
Contributor

@ragusaa ragusaa commented Feb 23, 2024

No description provided.

@ragusaa ragusaa force-pushed the CLAM-2256-AddALZSupport branch 3 times, most recently from beef30b to 387cbc4 Compare February 27, 2024 16:08
@ragusaa ragusaa changed the title Clam 2256 add alz support (NOT READY TO MERGE) Clam 2256 add alz support Feb 27, 2024
@micahsnyder
Copy link
Contributor

PR needs to be rebased with the upstream main branch to bump the FLEVEL to 210 so ALZ file type detection works and tests pass, and to resolve merge conflicts.

Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

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

Over all I'm super impressed with this being your first large amount of Rust code. Pretty awesome.

libclamav_rust/src/scanners.rs Outdated Show resolved Hide resolved
libclamav_rust/src/lib.rs Outdated Show resolved Hide resolved
clam-format Show resolved Hide resolved
clamd/server-th.c Outdated Show resolved Hide resolved
clamscan/clamscan.c Outdated Show resolved Hide resolved
libclamav_rust/src/alz.rs Outdated Show resolved Hide resolved
libclamav_rust/src/alz.rs Outdated Show resolved Hide resolved
libclamav_rust/src/alz.rs Outdated Show resolved Hide resolved
libclamav_rust/src/alz.rs Outdated Show resolved Hide resolved
libclamav_rust/src/alz.rs Outdated Show resolved Hide resolved
@ragusaa
Copy link
Contributor Author

ragusaa commented Mar 15, 2024

Over all I'm super impressed with this being your first large amount of Rust code. Pretty awesome.

Thank you, I am starting to really like rust.

@ragusaa ragusaa force-pushed the CLAM-2256-AddALZSupport branch 2 times, most recently from fe90c1f to c5bc180 Compare March 19, 2024 17:53
Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

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

A few minor things this time. My main concern is I don't think we should use info! at all. We've gotten (understandable) complaints about the "early end" warnings in the PDF parser. Users get confused by warnings when scanning malformed (but non-malicious) files.

In the future, we would want to record such events as weak indicators that could be used by signatures. That is of course pending implementing the weak indicator feature. Anyways... I ramble.

Final thing -- there is a linker error when building on Windows. It seems that the bzip-sys crate is compiling bzip2-1.0.8 into our libclamav_rust static library. That of course causes a linker error when we link libclamav (+libclamav_rust) with bz2.dll.
I'm not sure how to solve it. I created this issue to seek help: alexcrichton/bzip2-rs#102

libclamav/others.h Outdated Show resolved Hide resolved
libclamav/scanners.c Show resolved Hide resolved
libclamav_rust/src/alz.rs Outdated Show resolved Hide resolved
libclamav_rust/src/alz.rs Outdated Show resolved Hide resolved
libclamav_rust/src/alz.rs Outdated Show resolved Hide resolved
libclamav_rust/src/alz.rs Outdated Show resolved Hide resolved
libclamav_rust/src/alz.rs Outdated Show resolved Hide resolved
libclamav_rust/src/alz.rs Outdated Show resolved Hide resolved
@ragusaa
Copy link
Contributor Author

ragusaa commented Mar 25, 2024

A few minor things this time. My main concern is I don't think we should use info! at all. We've gotten (understandable) complaints about the "early end" warnings in the PDF parser. Users get confused by warnings when scanning malformed (but non-malicious) files.

In the future, we would want to record such events as weak indicators that could be used by signatures. That is of course pending implementing the weak indicator feature. Anyways... I ramble.

Final thing -- there is a linker error when building on Windows. It seems that the bzip-sys crate is compiling bzip2-1.0.8 into our libclamav_rust static library. That of course causes a linker error when we link libclamav (+libclamav_rust) with bz2.dll. I'm not sure how to solve it. I created this issue to seek help: alexcrichton/bzip2-rs#102

I agree on the info!.

So we'll wait to merge until we resolve the link issue?

@ragusaa
Copy link
Contributor Author

ragusaa commented Mar 25, 2024

Re-ran testing with your changes, and everything still looks good.

@micahsnyder
Copy link
Contributor

I just rebased it, fixed merged conflicts, and squashed commits down, and re-ran clam-format once more.

Copy link
Contributor

@micahsnyder micahsnyder left a comment

Choose a reason for hiding this comment

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

Jenkins test pipeline looks good to me as well.

@micahsnyder micahsnyder merged commit b6ebfbd into Cisco-Talos:main Apr 15, 2024
23 of 24 checks passed
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