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 2269 & 2271: CDIFF: unlink, close issues. Freshclam: download whole CVD if failure applying CDIFF #893

Merged

Conversation

micahsnyder
Copy link
Contributor

In the event that there is an issue with the CDIFF process, freshclam is treating it as thought no patch was downloaded.

If freshclam fails to apply the patch because of an issue with the patch, or some bug in the CDIFF module, it should retry for the whole CVD.

In the event that there is an issue with the CDIFF process, freshclam is
treating it as thought no patch was downloaded.

If freshclam fails to apply the patch because of an issue with the
patch, or some bug in the CDIFF module, it should retry for the whole
CVD.
@ragusaa
Copy link
Contributor

ragusaa commented Apr 11, 2023

Code looks good to me, however tests 8 and 9 fail when I run ctest.

@micahsnyder
Copy link
Contributor Author

Code looks good to me, however tests 8 and 9 fail when I run ctest.

OH NO! How'd I miss that? Will look into it and get back to you.

@micahsnyder micahsnyder changed the title Freshclam: Download whole CVD if failure applying CDIFF Clam 2269 & 2271: CDIFF: unlink, close issues. Freshclam: download whole CVD if failure applying CDIFF Apr 13, 2023
@micahsnyder
Copy link
Contributor Author

Okay I think it's all fixed and ready for review.

I found a couple extra issues when investigating.

The main other issue is this. It turns out the reason my original fix didn't work is because it did work and in so doing, it exposed a bug. It turns out the tests that apply test-3.cdiff should have failed because the logic to create a new file when appending new lines to a non-existent file was missing. So any CDIFF that added files would fail. Anyways, that's fixed.

I also fixed some bad log messages and improved some other log messages.

I've also merged the fixes from #892 into this PR because a moment I thought at first that the unlink bug was the cause of the CDIFF failures. It makes sense though in the end because I've had to add the CDIFF CLOSE operation fix so may as well do it in the PR that adds the CDIFF UNLINK operation fix.

Copy link
Contributor

@shutton shutton left a comment

Choose a reason for hiding this comment

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

Changes as discussed live.

Also includes:
- A sigtool test to verify that Rust log macros are working.
- Changing the freshclam tests to use --no-dns so they run faster
  when DNS isn't working (e.g. no internet).
Some log statements using the old ^, !, and * logg-prefix where they
were making use a ternary to determine the log level in the log
statement.

Also sigtool and freshclam weren't outputting error log messages using
the Rust log macros e.g. `error!("...")`.
Any cdiff or script using the UNLINK operation will fail to delete the
file claiming "No DB open for action UNLINK".

The UNLINK operation appears to be trying to delete a currently open
database, when in fact it should ensure no database is open before
deleting the local file given by the single "db_name" parameter.
Copy link
Contributor

@shutton shutton left a comment

Choose a reason for hiding this comment

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

Just that one utf8 conversion issue

libclamav_rust/src/cdiff.rs Outdated Show resolved Hide resolved
The CLOSE command is failing to create a file when appending changes if
the file does not already exist. This prevents adding new files to a
database with a CDIFF and caused failures applying the test-3.cdiff file
in the freshclam feature tests.

Also improved the error message to show which command, specifically, is
failing (not just the line number).
@micahsnyder micahsnyder force-pushed the CLAM-2271-cdiff-apply-failure branch 2 times, most recently from 1e579fa to e8103c1 Compare April 17, 2023 16:36
@micahsnyder micahsnyder force-pushed the CLAM-2271-cdiff-apply-failure branch from e8103c1 to 5c9bca0 Compare April 17, 2023 18:46
@micahsnyder
Copy link
Contributor Author

Just fixed some python 3.5 compatibility issues because python 3.6 isn't available on all machines we test on, even though python 3.5 is past enough of life. Sigh. Anyways all looked good now in the test pipeline. Will merge.

@micahsnyder micahsnyder merged commit 00559c2 into Cisco-Talos:main Apr 17, 2023
19 of 23 checks passed
@micahsnyder micahsnyder added the 🍒cherry-pick-candidate A PR that should be backported once approved. label Apr 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍒cherry-pick-candidate A PR that should be backported once approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants