Skip to content

Don't set parent's "don't cache" to match child #886

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

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

candrews
Copy link
Contributor

@candrews candrews commented Apr 3, 2023

Don't set the parent layer's "don't cache" flag to match the child.

emax_reached() already does the same thing so doing it again is unnecessary.

See db013a2 to see when the code that this commit removes was added.

Don't set the parent layer's "don't cache" flag to match the child.

`emax_reached()` already does the same thing so doing it again is unnecessary.

Signed-off-by: Craig Andrews <candrews@integralblue.com>
@val-ms
Copy link
Contributor

val-ms commented May 19, 2023

Ah I see you feel the same as I felt when writing the comment:

TODO: This may not be needed since emax_reached() should've already done that for us.

Yeah I think it should be safe to merge your PR. It's a very minor optimization and shouldn't change the behavior at all. Right?

@candrews
Copy link
Contributor Author

It's a very minor optimization

It increases cacheability which can be a major optimization :-)

and shouldn't change the behavior at all. Right?

Yes, correct.

@val-ms
Copy link
Contributor

val-ms commented May 19, 2023

It increases cacheability which can be a major optimization :-)

It shouldn't and it is critical that we do not increase cacheability here.

We do basically the same process but in one fell swoop whenever we exceed the limits. Right here we set that flag for all parent layers:
https://github.com/Cisco-Talos/clamav/blob/clamav-1.1.0/libclamav/scanners.c#L3957-L3965

The reason is that if any portion of the file could not not be scanned because of these limits, it does not mean it is clean. Scanning it later (perhaps at a lower recursion depth or in a smaller file) may result in actually scanning that portion and finding malware.

@candrews
Copy link
Contributor Author

You are, of course, correct, and I was mistaken.

I got this PR confused with another effort - I apologize for the mistake.

This PR is, as you said, a very minor optimization and shouldn't change the behavior at all, and I believe it is safe to merge.

@val-ms val-ms self-requested a review May 23, 2023 21:23
Copy link
Contributor

@val-ms val-ms left a comment

Choose a reason for hiding this comment

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

I did a little bit more testing and confirmed I'm very comfortable merging this PR.

@val-ms val-ms merged commit cec59d7 into Cisco-Talos:main Jul 31, 2023
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