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

Use antivirus_use_jit instead of clamd_use_jit #89

Closed
wants to merge 2 commits into from

Conversation

Klaas-
Copy link

@Klaas- Klaas- commented Aug 3, 2018

clamd_use_jit was renamed to antivirus_use_jit https://bugzilla.redhat.com/show_bug.cgi?id=1295473

@Klaas-
Copy link
Author

Klaas- commented Aug 3, 2018

I think I found how to compile that bytecode :) the diff seems a little big for my changes, from git history it looks like there are more changes that were made to source that did not make it into the real bytecode

I am unsure why the diff is that big :) But it seems like the code source was update more often than the compiled bytecode so maybe this includes more changes than just my own.
@micahsnyder
Copy link
Contributor

Thanks for the submission Klaas! That's frustrating that they renamed it to antivirus_use_jit, but I also understand why.

I also tried compiling the header with your change into bytecode to compare and the output I got was mostly the same, but slightly different than what's in your pull request. I'll review it again a little later when I find some more time and see if I can understand why.

@Klaas-
Copy link
Author

Klaas- commented Aug 9, 2018

If I recompile it the first line seems to change, maybe it includes some information like a compile date?

@micahsnyder
Copy link
Contributor

Hmm seems likely.

@micahsnyder
Copy link
Contributor

I'm told that the name antivirus_use_jit is only a redhat/fedora thing, and that the official selinux reference policy has clamd_use_jit. This change would apparently upset all of the other distro users.

@Klaas-
Copy link
Author

Klaas- commented Aug 23, 2018

hehe, okay I did not know that. Should I maybe make a reference to both?

@micahsnyder
Copy link
Contributor

I'm not familiar enough with selinux to say if duplicating that line to set both will work. If it does, that's probably a good way to go.

@micahsnyder
Copy link
Contributor

I will note, that if we do make this change, we'll have to update the bytecode.cvd database to propagate this. The builtin bytecode is overridden by one distributed in bytecode.cvd (which should be identical).

@micahsnyder
Copy link
Contributor

Things have been hectic here on development and we just passed our deadline for completing code for inclusion into 0.101. We're now focused on testing changes that we've made and merging those dev/0.101 prior to an upcoming 0.101.0-beta. As such, this change won't make it into 0.101.

Don't despair about it not being hardcoded into 0.101, as it is more important to get such a change into the bytecode.cvd database where it overrides the built-in version.

Regardless, we still can't accept the change without evaluating if using both clamd_use_jit and antivirus_use_jit will accomplish the desired effect so as to keep all distro's happy.

@micahsnyder
Copy link
Contributor

Adding link for reference, so this ticket isn't forgotten when we eventually figure out what to do with this: https://bugzilla.clamav.net/show_bug.cgi?id=12372

@micahsnyder micahsnyder closed this May 6, 2021
@micahsnyder micahsnyder deleted the branch Cisco-Talos:dev/0.101 May 6, 2021 02:33
@micahsnyder
Copy link
Contributor

I didn't mean to close this -- I deleted the old feature development branch because that version was merged, is done. It automatically closed this rather than re-targeting for a different branch.

It's still a good idea to try to support both antivirus_use_jit and clamd_use_jit. Just no time to work on it on my end and this PR swapped one for the other instead of trying to support both. I suppose it's good that we close it. This should really be a bug report until someone can put together a fix.

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.

None yet

2 participants