[PATCH] Samba VirusFilter (version 12)

[Trever L. Adams]

On 01/02/2018 07:59 PM, Jeremy Allison wrote:
> On Tue, Jan 02, 2018 at 06:31:06PM -0700, Trever L. Adams via samba-technical wrote:
>> On 01/02/2018 04:26 PM, Jeremy Allison wrote:
>>> On Tue, Jan 02, 2018 at 10:34:47AM -0700, Trever L. Adams via samba-technical wrote:
>>>> Thank you to all who have been most helpful it getting things cleaned
>>>> up. Hopefully this is ready for merging.
>>> Will still need a few README.Coding changes, but closer !
>>>
>>> Can you explain what this code does ?
>>>
>>>                 q_filepath = talloc_asprintf(talloc_tos(), "%s/%s%s%s", q_dir,
>>>                                              q_prefix, base_name, q_suffix);
>>>
>>>                 TALLOC_FREE(q_dir);
>>>                 TALLOC_FREE(q_prefix);
>>>                 TALLOC_FREE(q_suffix);
>>>
>>>                 become_root();
>>>
>>>                 q_smb_fname = synthetic_smb_fname(mem_ctx, q_filepath,
>>>                                                   smb_fname->stream_name, NULL,
>>>                                                   smb_fname->flags);
>>>                 if (q_smb_fname == NULL) {
>>>>>>>>>>>>>>>>           unlink(q_filepath);
>>>                         unbecome_root();
>>>                         return VIRUSFILTER_ACTION_DO_NOTHING;
>>>                 }
>>>
>>>                 int_ok = virusfilter_vfs_next_move(handle, smb_fname,
>>>                                                    q_smb_fname);
>>>                 if (int_ok == -1)
>>>                 {
>>>                         unbecome_root();
>>>                         DBG_ERR("Rename failed: %s/%s: Rename failed: %s\n",
>>>                                 cwd_fname, fname,
>>>                                 strerror(errno));
>>>                         return VIRUSFILTER_ACTION_DO_NOTHING;
>>>                 }
>>>                 unbecome_root();
>>>
>>> Why are you unlinking q_filepath under root protections here ?
>>>
>>> Once I understand that, I'll send you a README.coding fix
>>> you can merge in and then I'll do a final review.
>>>
>>> Thanks !
>>>
>>> Jeremy.
>>>
>> Jeremy,
>>
>> I am very sorry. This was a mistake from when I needed a safe call when
>> debugging a memory related crash over a year ago. It some how slipped
>> into my commits back then. You called me on it before. As noted
>> elsewhere I went back to a previous version of this (quarantine) code in
>> a recent changeset to simplify things and make the quarantine
>> functionality work like more people might expect it to. Unfortunately,
>> while integrating changes from the then current version into the old
>> one, this bug slipped back in.
>>
>> The unlink is supposed to be TALLOC_FREE. I have changed it locally.
>> Again, my apologies for this slip. Thank you for catching it a second time.
> Hey, no worries Trevor ! This is *fiendishly* complex stuff,
> and I really appreciate you doing the work. I wasn't trying
> to complain, just understand :-).
I appreciate the help. I didn't think you were complaining. Hopefully my
answer gave understanding while correcting the flaw your question made
obvious. I am glad you asked. I found a similar problem in the RENAME
action code path. I have fixed that as well. I figure where you will
soon be providing me with additional fixes, flooding the list with an
interim fix would not be productive.

https://github.com/treveradams/samba/tree/testing

The last 5 commits in that tree are current (always will be until this
is finished as I use that as the way to propagate the changes to the
server(s) I use for testing.

Thank you again.

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/20180104/48b99cee/signature.sig>