[PATCH] Samba-VirusFilter: fix crash in virusfilter_vfs_close that was missed by me in review/cleanup

[Trever L. Adams]

On 01/24/2018 01:41 PM, Jeremy Allison wrote:
> On Wed, Jan 24, 2018 at 11:35:16AM -0700, Trever L. Adams via samba-technical wrote:
>> In the cleanup process a variable was set to be NULL which shouldn't
>> have been done. For some reason I missed this in review/cleanup.
>>
>> This patch fixes that problem.
> LGTM. RB+. Sorry for missing that in the review.
>
> Ralph, can I get a second Team review ?
>
I think I know how my testing failed to catch this. Not all of my shares
have scan on close turned on. I didn't do write tests to all of the
shares. If a file is not modified scan on close doesn't scan. I may have
failed to write to the shares that do have it turned on.

As was suggested elsewhere, it might be good to have some tests to make
sure this module stays functioning properly.

Suggested share configuration would be rename or quarantine such as:

        virusfilter:scanner = clamav
        virusfilter:scan streams = yes
        virusfilter:cache time limit = 3600
        virusfilter:scan on open = yes
        virusfilter:scan on close = yes
#       virusfilter:infected file action = quarantine
       virusfilter:infected file action = rename
       virusfilter:rename prefix =
       virusfilter:rename suffix = .INFECTED
       virusfilter:quarantine prefix =
       virusfilter:quarantine suffix = .INFECTED
       virusfilter:quarantine keep tree = true
       virusfilter:quarantine keep name = true

I remove the prefix in my configurations, here it wouldn't matter.

I think we need four test cases:

1) A file exists in the share (created outside of Samba or with the
virusfilter off). It contains a string which a fake clamav daemon (I
have one written, but we may want to modify it as I have EICAR test
virus split into two in it and whatever creates the files would have to
play similar games.) recognizes as a virus. Everything else is reported
as ok. Success would be if when the file is then access it gets
renamed/quarantined according to the configuration.

2) A file is created or rewritten to contain the same test string. It is
then closed. If the file is renamed/quarantined then it is a success.

3) A file exists in the share (again created outside of Samba or with
the filter off). It does NOT contain the string. Success is the file is
opened successfully and is read successfully. No rename/quarantine.

4) A file is created, or rewritten not having and never having had the
test string. Success is the file is closed successfully. No
rename/quarantine.

The fake clamav daemon (written in python only about 150 lines, I
believe). I have can go by a regex of the file string (makes 2 and 4 a
little more difficult to test as the file name will always be the same),
or it can check for a test string. I think checking for the test string
is the correct way to go.

As I am not familiar with Samba's self tests, I would appreciate if
someone could work with me to implement these tests and make sure they
work and that my fake daemon is correct. I would also appreciate input
on the correct thing to do (have EICAR test virus split up and combine
it in programs to do the testing, or come up with a new test string or
use the file name method).

Thank you.
Trever

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 886 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180124/585065b5/signature.sig>