Implementing SMB_VFS_FCNTL in Samba

[Jeremy Allison]

Ping ! Can I get a second Team reviewer for Anoop's work ?

My VFS_RMDIR removal is waiting on this :-).

Thanks,

Jeremy.

On Fri, Oct 04, 2019 at 10:55:28AM -0700, Jeremy Allison via samba-technical wrote:
> On Fri, Oct 04, 2019 at 03:17:01PM +0530, Anoop C S wrote:
> > On Fri, 2019-10-04 at 10:38 +0200, Ralph Boehme via samba-technical
> > wrote:
> > > On 10/3/19 3:20 PM, Anoop C S via samba-technical wrote:
> > > > On Thu, 2019-10-03 at 18:43 +0530, Anoop C S via samba-technical
> > > > wrote:
> > > > > OK. This should be it. I hope attached patch covers the missing
> > > > > part
> > > > > where recent fcntl() commands are detected during configure. A
> > > > > pipeline
> > > > > has been completed successfully for the attached patches.
> > > > > 
> > > > > https://gitlab.com/samba-team/devel/samba/pipelines/86263033
> > > > > 
> > > > > Reviews are appreciated.
> > > > 
> > > > Please ignore the previous version which had a typo in checking
> > > > HAVE_XX_XX inside vfs_default. Attaching the patches after
> > > > correction.
> > > 
> > > nice addition, thanks!
> > > 
> > > One nitpick and one general question.
> > > 
> > > Please don't do function calls in if expressions:
> > > 
> > > if ((val = SMB_VFS_FCNTL(fsp, F_GETFL, 0)) == -1) {
> > >     return -1;
> > > }
> > > 
> > > Instead:
> > > 
> > > val = SMB_VFS_FCNTL(fsp, F_GETFL, 0);
> > > if (val == -1) {
> > >     return -1;
> > > }
> > 
> > Right. I was also skeptical in putting it that way(frankly speaking it
> > is a copy of current set_blocking() function). :-)
> > 
> > Please see the new patch set attached.
> 
> Thanks !
> 
> > > Then, I wonder why you make a copy of va_args in the time_audit and
> > > full_audit VFS modules before calling the NEXT function. Can't we
> > > just pass the va_list on to the NEXT function?
> > 
> > Two reasons:
> > 
> > * Both SMB_VFS_FCNTL and SMB_VFS_NEXT_FCNTL invoke same
> > smb_vfs_call_fcntl() function which does a va_start() on the received
> > variable arguments. Thus if va_list is passed as variable argument to
> > SMB_VFS_NEXT_FCNTL we again end up calling va_start() on it and a
> > subsequent va_arg() would not give us the required(/original) argument.
> > This lead me to do argument extraction before passing it to
> > SMB_VFS_NEXT_FCNTL. Now here comes the next problem..
> > 
> > * This is the prominent one and I mentioned in one of my previous
> > responses in this thread. Basically the program(in our case smbd)
> > crashes when va_arg() is done directly on the received va_list without
> > duplication. Following blog tries to explain this in some detail:
> > 
> > https://julio.meroh.net/2011/09/using-vacopy-to-safely-pass-ap.html
> > 
> > It may be platform dependent but va_copy() seems to be the right way.
> 
> +1 on that. The va_XXX() calls are complex enough and implemented
> differently on enough systems such that *always* calling the expected
> macros is the only safe way to be portable on such things.
> 
> This new patchset - +1 and RB+ me. Ralph, are you also good with it ?
> 
> If so I'll push.
> 
> Thanks once again Anoop.
> 
> Jeremy.
>