libclamav: Use OpenSSL' BN instead tomfastmath. #840
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
ping |
val-ms
requested changes
Mar 8, 2023
There was a problem hiding this comment.
After building, I had some trouble with running freshclam. It seems to get stuck when verifying the databases. On the command line it looks liek this where it hangs:
❯ ./install/bin/freshclam
ClamAV update process started at Tue Mar 7 17:58:41 2023
daily database available for download (remote version: 26834)
Time: 0.2s, ETA: 0.0s [========================>] 57.99MiB/57.99MiB
Testing database: '/home/micah/workspace/clamav-micah/build/install/share/clamav/tmp.c190d1c762/clamav-54297508c3c0d83118e51220f7f23312.tmp-daily.cvd' ...
and in a debugger, I found it was stuck here:
I haven't put too much time into trying to figure out why it hangs.
|
On 2023-03-08 16:42:16 [-0800], Micah Snyder wrote:
Sleep well. Let me know when you're done pushing up changes to the PR
and I'll re-test, re-review. If you have any idea what's causing
freshclam to hang for this build, or if it doesn't hang for you -
please let me know.
I pushed the updated branch and I believe I addressed all of you
comments.
I did use freshclam on top my previous main branch and the current one
(as of today) and I did not encounter any hangs. It took a while but it
passed:
| Fri Mar 10 07:56:08 2023 -> updatedb: Running g_cb_download_complete callback...
| Fri Mar 10 07:56:08 2023 -> download_complete_callback: Download complete for database : /home/bigeasy/clamav/clamav/build/inst/lib/tmp.1d20dec9d6/clamav-c689dbdf9709fdaeafa9d8d847e30df3.tmp-daily.cld
| Fri Mar 10 07:56:08 2023 -> download_complete_callback: fc_context->bTestDatabases : 1
| Fri Mar 10 07:56:08 2023 -> download_complete_callback: fc_context->bBytecodeEnabled : 1
| Fri Mar 10 07:56:08 2023 -> Testing database: '/home/bigeasy/clamav/clamav/build/inst/lib/tmp.1d20dec9d6/clamav-c689dbdf9709fdaeafa9d8d847e30df3.tmp-daily.cld' ...
| Fri Mar 10 07:56:08 2023 -> Loading signatures from /home/bigeasy/clamav/clamav/build/inst/lib/tmp.1d20dec9d6/clamav-c689dbdf9709fdaeafa9d8d847e30df3.tmp-daily.cld
| Fri Mar 10 07:56:16 2023 -> Properly loaded 2025243 signatures from /home/bigeasy/clamav/clamav/build/inst/lib/tmp.1d20dec9d6/clamav-c689dbdf9709fdaeafa9d8d847e30df3.tmp-daily.cld
| Fri Mar 10 07:56:17 2023 -> Database test passed.
| Fri Mar 10 07:56:17 2023 -> daily.cld updated (version: 26836, sigs: 2025243, f-level: 90, builder: raynman)
| Fri Mar 10 07:56:17 2023 -> fc_update_database: daily.cld updated.
I did have the extra debug turned on but I doubt this makes any
difference.
Sebastian
|
val-ms
requested changes
Mar 21, 2023
There was a problem hiding this comment.
Thank you so much for the update. All of the changes you made look good minus one request regarding the .clang-format file.
In testing, I found 2 things:
- our internal test pipelines didn't realize the centos 7 x64/x86 builds failed and was instead failing the tests because clamav was not installed (I will fix) 😅
- the centos 7 x64/x86 builds failed because
BN_bn2binpadis not available for the older openssl version. If it's possible to useBN_bn2bininstead it would be ideal to keep supporting openssl 1.0.2 / centos 7 for a little while longer.
|
Sorry looks like I also just introduced a merge conflict with a different PR merge. |
Use OpenSSL's big number/ multiprecision integer arithmetics functionality to replace tomfastmath. This is a first shot at doing just this. Further improvement could be use more RSA-signature verification from OpenSSL in crtmgr_rsa_verify() and less self parsing. _padding_check_PKCS1_type_1() has been borrowed from OpenSSL to make further replacments easier. Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
Now that the tomfastmath library is no longer used, remove it from the tree. Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
|
On 2023-03-23 09:20:45 [-0700], Micah Snyder wrote:
Sorry looks like I also just introduced a merge conflict with a
different PR merge.
Could you double check? I just pushed on top of current main branch and
should have address your review comments.
Sebastian
|
val-ms
approved these changes
Mar 29, 2023
There was a problem hiding this comment.
I re-read everything and re-tested and re-ran through Jenkins. Everything looks really good to me. Thank you very much @sebastianas for all of the effort you put into this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use OpenSSL's big number/ multiprecision integer arithmetics
functionality to replace tomfastmath.
This is a first shot at doing just this. Further improvement could be
use more RSA-signature verification from OpenSSL in crtmgr_rsa_verify()
and less self parsing.
_padding_check_PKCS1_type_1() has been borrowed from OpenSSL to make
further replacements easier.
This would make the tomfastmath pull obsolete ;)