[PATCH] Samba VirusFilter (version 12)

[Jeremy Allison]

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 am sorry that I am missing other coding style issues. I look forward
> to merging your changes.

OK, I'll try and finish them up and send them to you soon.

Jeremy.