Bug 14606 - vfs_virusfilter stops working after some time after startup
Summary: vfs_virusfilter stops working after some time after startup
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Karolin Seeger
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-01-02 04:54 UTC by Arne Kreddig
Modified: 2021-02-01 15:40 UTC (History)
3 users (show)

See Also:


Attachments
Samba Log File (1.51 KB, text/plain)
2021-01-02 04:54 UTC, Arne Kreddig
no flags Details
git-am fix for 4.13.next. (5.72 KB, patch)
2021-01-07 23:00 UTC, Jeremy Allison
vl: review+
slow: review+
Details
git-am fix for 4.12.next. (5.70 KB, patch)
2021-01-07 23:00 UTC, Jeremy Allison
vl: review+
slow: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Arne Kreddig 2021-01-02 04:54:11 UTC
Created attachment 16372 [details]
Samba Log File

Overview:
If samba is up for a while and vfs_virusfilter is used, the virusfilter will stop working after some time.

Steps to Reproduce:
I do not have a definitive 100% working method here. However, on both my productive system (home server) and my laptop, it happened persistently enough to debug a bit into it. Steps that I took:
  1) Start smbd
  2) Open a few Excel files on Windows 10 (on the samba share of course)
  3) Write and close and reopen the Excel-Files in a random fashion
  4) After some minutes (sometimes just seconds) the bug should occur

Actual result:
vfs_virusfilter stops working. According to the log file, it cannot connect to the ClamAV socket anymore. Judging from the log file (see attachment), the memory where the "virusfilter:socket path" configuration is stored gets overwritten by some seemingly random data (yes, I have set it correctly in the configuration file and directly after startup of smbd everything works and smbd communicates properly with the ClamAV socket).

Expected result:
vfs_virusfilter should keep working without overwriting its internal configuration memory.

Build:
Version 4.11.6-Ubuntu

Additional Information:
From my first investigation, it seems that the memory which holds the configuration gets freed and thus gets later allocated somewhere else and thus gets overwritten with some seemingly random data. I made a first attempt in fixing the bug here: https://gitlab.com/fr89k/samba/-/commit/6e0b8468195fac8f5791ca9e8eb25af0321b89ae
I think the configuration char pointers should be copied into a newly allocated memory during initialization of vfs_virusfilter. At least this is the way other vfs modules do it, as far as I have seen. However, due to lack of understanding of the samba software architecture, memory ownership, and memory allocation concepts, I am not sure if I identified the root cause correctly and if my fix is actually the correct way to fix the problem (or if I just introduced a new memory leak).
Comment 1 Samba QA Contact 2021-01-07 19:26:03 UTC
This bug was referenced in samba master:

2f21d1b0ac8526508161de73290f67858b2fe668
Comment 2 Jeremy Allison 2021-01-07 23:00:14 UTC
Created attachment 16381 [details]
git-am fix for 4.13.next.
Comment 3 Jeremy Allison 2021-01-07 23:00:56 UTC
Created attachment 16382 [details]
git-am fix for 4.12.next.
Comment 4 Karolin Seeger 2021-01-08 07:56:17 UTC
Puhsed to autobuild-v4-{13,12}-test.
Comment 5 Samba QA Contact 2021-01-13 13:45:21 UTC
This bug was referenced in samba v4-12-test:

6adf36190693d3183437fdc07bd77fc8b708bace
Comment 6 Samba QA Contact 2021-01-13 14:55:10 UTC
This bug was referenced in samba v4-13-test:

9ab30ab1c80bfc31d28830b14924e4bcba72d9a2
Comment 7 Samba QA Contact 2021-01-14 08:34:59 UTC
This bug was referenced in samba v4-12-stable (Release samba-4.12.11):

6adf36190693d3183437fdc07bd77fc8b708bace
Comment 8 Karolin Seeger 2021-01-14 08:45:29 UTC
Closing out bug report.

Thanks!
Comment 9 Samba QA Contact 2021-01-29 13:56:10 UTC
This bug was referenced in samba v4-13-stable (Release samba-4.13.4):

9ab30ab1c80bfc31d28830b14924e4bcba72d9a2
Comment 10 Matthias Leopold 2021-02-01 09:58:57 UTC
Is there a reason why suddenly a couple of configuration options, which should have default values according to the manual, have to be set explicitly? This concerns

        virusfilter:infected file command
        virusfilter:scan error command
        virusfilter:quarantine directory
        virusfilter:quarantine prefix
        virusfilter:quarantine suffix
        virusfilter:rename prefix
        virusfilter:rename suffix

Also the error message ("virusfilter-vfs: out of memory!") when not doing so is very confusing. Without reading the source I would never have found out.
Comment 11 Arne Kreddig 2021-02-01 10:36:26 UTC
Are you sure that it does not work for these:

        virusfilter:quarantine directory
        virusfilter:quarantine prefix
        virusfilter:quarantine suffix
        virusfilter:rename prefix
        virusfilter:rename suffix

I just checked the code and for the "infected file command" and "scan error command", there is indeed something wrong. For the others, however, it should work. According to the code, the values are taking from the configuration. If the configuration was not set, it takes the default values. Then, they get copied to a separate memory and if this copy operation fails, you get the "out of memory" error.
And for the command options, the default is set to NULL which would cause this weird error message.

I am not extremely familiar with the samba dev workflow, but I think this should get a new bug ticket and not be an amendment to this bug? In this case, it would be good if you opened a new ticket.

In the meanwhile, I'll try to reproduce it.
Comment 12 Arne Kreddig 2021-02-01 15:40:09 UTC
I just checked and I can only reproduce the bug for these two:

        virusfilter:infected file command
        virusfilter:scan error command

Can you please check that it really also doesn't work for the others?
I prefer not hunting some ghosts that don't exist.