Ping, this just killed our mail system again:

May 13 17:34:09 mx1 freshclam[9344]: Testing database: '/var/lib/clamav/tmp.7c01d/clamav-9e81402cf229e7c978780e6270053756.tmp-malwarepatrol-ext.db' ...
May 13 17:34:09 mx1 freshclam[9356]: Loading signatures from /var/lib/clamav/tmp.7c01d/clamav-9e81402cf229e7c978780e6270053756.tmp-malwarepatrol-ext.db
May 13 17:34:09 mx1 freshclam[9356]: [LibClamAV] Malformed pattern line 18698
May 13 17:34:09 mx1 freshclam[9356]: [LibClamAV] Problem parsing database at line 18698
May 13 17:34:09 mx1 freshclam[9356]: [LibClamAV] Can't load /var/lib/clamav/tmp.7c01d/clamav-9e81402cf229e7c978780e6270053756.tmp-malwarepatrol-ext.db: Malformed database
May 13 17:34:09 mx1 freshclam[9356]: Failed to load new database: Malformed database
May 13 17:34:09 mx1 freshclam[9344]: Database test passed.
May 13 17:34:09 mx1 freshclam[9344]: malwarepatrol-ext.db updated (version: custom database, sigs: 152304)

And again:

Jun 12 10:59:07 mx1 freshclam[22987]: Database update completed successfully.
Jun 12 10:59:07 mx1 freshclam[22987]: Clamd successfully notified about the update.
Jun 12 10:59:07 mx1 freshclam[22987]: --------------------------------------
Jun 12 10:59:09 mx1 clamd[22976]: Reading databases from /var/lib/clamav
Jun 12 10:59:19 mx1 clamd[22976]: reload db failed: Malformed database
Jun 12 10:59:20 mx1 clamd[22976]: Terminating because of a fatal error.
Jun 12 10:59:20 mx1 clamd[22976]: Pid file removed.
Jun 12 10:59:20 mx1 clamd[22976]: --- Stopped at Fri Jun 12 10:59:20 2020
Jun 12 10:59:20 mx1 clamd[22976]: Socket file removed.

Sorry about the delay responding. 

The issue does seem self explanatory.  I see it says it failed to load the new database, bu then Database test passed. It sounds like a bug in error handling.

Will take quick look.

I don't see any obvious logic issues in the code.

I was unable to reproduce the issue where it fails to load but then "passes":

Time:    0.0s, ETA:    0.0s [========================>]         8B/8B
Testing database: '/Users/micasnyd/workspace/clamav-micasnyd-4/install/share/clamav/tmp.896ca9569a/clamav-6796b88e676053889edd44d2f8866046.tmp-test.db' ...
WARNING: Stderr output from database load : WARNING: [LibClamAV] Malformed pattern line 1 [...] ERROR: Failed to load new database: Malformed database
WARNING: Database load exited with "Test failed" (8)
ERROR: Database test FAILED.
Unexpected error when attempting to update from custom database URL: http://127.0.0.1:8000/test.db
WARNING: fc_download_url_databases: fc_download_url_database failed: Test failed (8)
ERROR: Database update process failed: Test failed (8)
ERROR: Update failed.


Perhaps there is some OS specific issue relating to how the exit code for the db test subprocess is intepreted? This code is a little convoluted: https://github.com/Cisco-Talos/clamav-devel/blob/dev/0.103/freshclam/freshclam.c#L325

Can you remind me, which OS are you working with? 

Regards,
Micah

(In reply to Micah Snyder from comment #4)
> 
> Perhaps there is some OS specific issue relating to how the exit code for
> the db test subprocess is intepreted? This code is a little convoluted:
> https://github.com/Cisco-Talos/clamav-devel/blob/dev/0.103/freshclam/
> freshclam.c#L325
> 
> Can you remind me, which OS are you working with? 
> 

Gentoo. In the distribution package, we're not doing anything *too* unusual. You can see the build process here:

https://gitweb.gentoo.org/repo/gentoo.git/tree/app-antivirus/clamav/clamav-0.102.3-r1.ebuild

It's mostly hacking the default config with some Gentoo-specific locations. On my particular mail server, I'm building with clamdtop, iconv, and bzip2 support -- and nothing else.

The only config changes that I've made that I think might be relevant are to log both clamd/freshclam to syslog, and to enable verbose logging for freshclam. (The code seems concerned with stderr, and your output contains "WARNING:" in the LibClamAV error messages while mine doesn't.)

It does seem concerned with stderr output, but only for the purposes of displaying the warnings and a warning that there were warnings. :D

I reproduced the issue using the --daemonize (-d) options and also observed in testing that even when it said it "failed" it still saved the downloaded test database.

Example failure with the `freshclam -d -F`

Time: 0.0s, ETA: 0.0s [=============================>] 8B/8B
Testing database: '/Users/micasnyd/workspace/clamav-micasnyd-4/install/share/clamav/tmp.7c26b/clamav-9f1c3b6d988d24c08def028e453ce6b3.tmp-test.db' ...
WARNING: Stderr output from database load : WARNING: [LibClamAV] Malformed pattern line 1 [...] ERROR: Failed to load new database: Malformed database
WARNING: Database successfully loaded, but there is stderr output
Database test passed.
test.db updated (version: custom database, sigs: 1)


Will try to find time to solve this bug now that I can reproduce it reliably.  However, we are scrambling to finish a couple high priority items before we run out of time in our schedule and have to publish a 0.103 release candidate and the 0.102.4 patch release.

Ah, good. I assumed this would be easy to reproduce or I would have made sure to provide enough information in the first place.

I may be biased, but I think the ability to remotely crash someone else's security product is worthy of being included in the next patch release =)

*** Bug 12628 has been marked as a duplicate of this bug. ***

Created attachment 7711 [details]
remove sigchld handler after freshclam daemonize

Ok so it seems that when we daemonize freshclam, we set a handler for SIGCHLD so we don't become a zombie when the parent process exits -- but that makes it so future forks (like the one used for testing databases) aren't child processes and thus we can't get the return status.  The solution I'm testing is to remove the handler for SIGCHLD immediately after freshclam daemonizes.  This seems to work ok, patch is attached.

Ah, so the problem is the global SIGCHLD handler getting in the way of the waitpid() in download_complete_callback()?

It looks like it should work... but now I have more questions. Mainly, is the global SIGCHLD handler needed at all, after the change to use daemonize_parent_wait()? None of the other daemons seem to need it.

If it's still necessary, you might be able to install a more-specific global handler by e.g. making daemonize_wait_parent() return the fork's PID and then using waitpid() only for that PID in the global handler. But that all depends on *why* it's still necessary, if it is.

Maybe related: also note that g_sigchildWait is constant "1" now.

(In reply to Michael Orlitzky from comment #12)
> Maybe related: also note that g_sigchildWait is constant "1" now.

Yikes ok yes there's more work to be done here.  It does look like the g_sigchildWait thing is neglected/broken since the freshclam overhaul.  I'll review the old code to understand how it works.

You're probably right about SIGCHLD not being required, unless the sigchildWait-thing is important. Will focus on it today and get back to you.

Created attachment 7713 [details]
disable wait in global handler before test

Good catch with regards to g_sigchildWait. Setting that to `0` before forking for the database test was the correct solution to bring it back to how things worked before I introduced this bug when overhauling freshclam to use libcurl a couple years ago. 

Attached is an updated patch with the new fix.

LGTM. I've pushed it out on Gentoo, and updated our MX server. Will know for sure the next time the malwarepatrol database goes berserk.

I just merged the fix for this.  See:
0.104: https://github.com/Cisco-Talos/clamav-devel/commit/ade9352d9a168f3560f28da806b48e010ff009b7
0.103.1: https://github.com/Cisco-Talos/clamav-devel/commit/10825613a7dcf25022f0fe75c8c9cd693affdeb2

Thanks!