[PATCH 2/7] Samba-VirusFilter: common headers and sources. version 3

[Jeremy Allison]

On Tue, Nov 01, 2016 at 03:51:56PM -0600, Trever L. Adams wrote:
> On 11/01/2016 01:21 PM, Jeremy Allison wrote:
> > On Fri, Oct 21, 2016 at 11:58:05AM -0600, Trever L. Adams wrote:
> >
> > OK - more requests inline (sorry).
> >
> > We're getting there (slowly... :-).
> >
> > Jeremy.
> Thank you for your patience.

No, thank *you* :-). I know doing all this is a pain,
but I want to get this code into an easily maintainable
state. We've have modules merged in the past that got
dropped after a few releases after the maintainer got
bored, so I want to avoid that again.

> > virusfilter_url_quote() seems to be used only in the sophos
> > module. I think it should be moved there and out of the common
> > code. Also please fix the spacing/tabs inside there.
> I will take care of this as soon as I know things are working.
> I have already taken care of the tabs/spaces.
> >
> > Something to think about - the above is passed pathname
> > strings. This will only work if the backend is utf8 or ascii
> > compatible - yes ? What if the backend is one of the asian
> > sjis or others ? (Do we even use those anymore) ?
> This is a good question. I am not even sure it works with utf-8, but it
> might. I also have no answer on what is or isn't used. Part of the
> problem will also be that Sophos, or another backend, may use a
> different encoding. I am not sure where to go on this.

Yeah. Me neither. Let's leave it for now and I'll think about
it some more.

> > All of the functions above are direct calls into POSIX.
> > This makes the module explicitly not-stackable. Is this
> > what you intended ?
> No. I am doing what I can to keep this stackable. Do you have any
> suggestions on rewriting this or of a samba internal move that works
> across file systems?

Maybe junk it - just fail the rename on EXDEV ?

> > If we're changing that to be a tstream can we fix this
> > up as well to use a tevent context with callback functions ?
> I am in the process of trying to fix the writev code and this. Write now
> I am struggling to figure out how to get wscript to build with
> samba-socket as a declared public dependency.
> 
> I also realize I need to move my code out of sys_rw_data.[ch] as the
> changes make it appropriate to be there. Would the Samba team prefer
> this is abstracted out for others to use, or shall I merge it back into
> vfs_virusfilter.utils.[ch]?

Use vfs_virusfilter.utils.[ch] for now. If we decide we need
it we can always make it generic and move it out later.

> > Urgggh. I *hate* passing these parameters as environment variables.
> >
> > I guess it's more dangerous to pass them as command line parameters
> > though, due to shell expansions ?
> This is what I believe was behind that design. That and allowing
> flexibility in the scripts that are called with this code.
> >
> > Can we think more about this ?
> Sure. You have given me much to work on.

> >> +
> >> +#define str_eq(s1, s2)		\
> >> +	((strcmp((s1), (s2)) == 0) ? true : false)
> >> +#define strn_eq(s1, s2, n)	\
> >> +	((strncmp((s1), (s2), (n)) == 0) ? true : false)
> > Please don't use macro's like the above. No one else
> > has a clue what they mean (although these are more obvious than
> > most :-). Please use the standard functions - or create functions
> > for this.
> Are you specifically meaning the str... macros or the header definition
> macro? I believe I can also remove the
> VIRUSFILTER_SCAN_RESULTS_CACHE_TALLOC stuff as that was for use during
> the merge process.

The str.. macros. Great if you can remove VIRUSFILTER_SCAN_RESULTS_CACHE_TALLOC
also - the less new code the better :-).

> >> +#define ALLOC_CHECK(ptr, label) do { if ((ptr) == NULL) { DEBUG(0, \
> >> +	("virusfilter-vfs: out of memory!\n")); \
> >> +	errno = ENOMEM; goto label; } } while(0)
> > No macros like the above please - we're trying to get rid of things
> > like this (yes I know it's copied from vfs_recycle - it's ugly there
> > too :-).
> Does this apply to the module name or just the alloc check?

Just the alloc check - sorry I wasn't clear. We don't like
chunks of code in macros if we can avoid it.

> > Should the above be signed ?
> >> +}
> > How does the above handle streams ? Has this been tested
> > with viruses being written into streams ? (This is a very
> > popular way to hide virus code on Windows NTFS filesystems) ?
> I do not know if it has been tested. I have never worked with streams.
> If anyone has a test case of such a file that I can use in testing, I
> will see what I can do to fix things up to handle streams. Of course,
> changing the real virus for the test virus would be appreciated.

Yes, testing with streams would be very useful. If this is stackable
on top of vfs_streams_depot then we can make sure someone squirriling
away a virus in a named stream with a name ending in .EXE gets caught.

> Thank you. I have done the conversion and will do the cleanups on the
> messages as I work on your other change requests.
> 
> I believe I mostly have the tevent/tsocket code done, but until I can
> figure out how to get wscript to not complain, I won't be able to test.

Thanks ! Much appreciated.

Jeremy.