[PATCH] s3: vfs: time_audit: fix handling of token_blob in smb_time_audit_offload_read_recv()

[Christof Schmitt]

On Thu, Aug 09, 2018 at 12:46:32PM -0700, Christof Schmitt via samba-technical wrote:
> On Thu, Aug 09, 2018 at 04:15:23PM +0200, Ralph Wuerthner via samba-technical wrote:
> > Hi list,
> > 
> > please see attached patch to fix a defect within vfs_time_audit.
> > When using vfs_time_audit smbtorture test case
> > smb2.ioctl.req_resume_key is failing with the following error
> > message:
> > 
> > Using seed 1533822459
> > time: 2018-08-09 15:47:39.669901
> > test: req_resume_key
> > time: 2018-08-09 15:47:39.670594
> > time: 2018-08-09 15:47:39.712858
> > failure: req_resume_key [
> > ../source4/torture/smb2/ioctl.c:104: status was
> > NT_STATUS_INTERNAL_ERROR, expected NT_STATUS_OK:
> > FSCTL_SRV_REQUEST_RESUME_KEY
> > ]
> > 
> > A similar failure can be seen by using vfs_fruit and vfs_time_audit
> > when running vfs.fruit.copyfile:
> > 
> > Using seed 1533822525
> > time: 2018-08-09 15:48:45.461933
> > test: copyfile
> > time: 2018-08-09 15:48:45.464498
> > WARNING!: ../source4/torture/vfs/fruit.c:2482: status was
> > NT_STATUS_NO_MEMORY, expected NT_STATUS_OK:
> > FSCTL_SRV_REQUEST_RESUME_KEY
> > time: 2018-08-09 15:48:45.548604
> > failure: copyfile [
> > ../source4/torture/vfs/fruit.c:2637: setup copy chunk error
> > ]
> 
> Good catch! I think the intention of the code was to first use a local
> variable and then assign the result to the pointer. That pattern is
> common in the Samba code.
> 
> > Dump question: wouldn't it make sense to run all self tests with
> > vfs_time_audit enabled as this module is by definition a no-op?
> 
> Good question. At least in master, the "simpleserver" environment
> already includes time_audit. The gap is that smb2.ioctl is not running
> against the simpleserver.
> 
> Attached are two patches, one is a proposed shorter fix, the other one is
> for running smb2.ioctl also against simpleserver. If you only apply the
> second patch, you can easily see the failing test:
> 
> ./configure --picky-developer --enable-selftest
> make -j
> make test TESTS=smb2.ioctl
> 
> If you agree with the fix, then we should open a bugzilla and also
> backport the fix to 4.8 (i have not check 4.7 yet).

Just confirmed that 4.7 has the same problem. We should backport this
to 4.7, 4.8 and 4.9rc.

> Christof

> From a93ccc62f3eb34a2e66fbdd4700701440b76f834 Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Thu, 9 Aug 2018 12:39:34 -0700
> Subject: [PATCH 1/2] s3: vfs: time_audit: fix handling of token_blob in
>  smb_time_audit_offload_read_recv()
> 
> Found by Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
> 
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
>  source3/modules/vfs_time_audit.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/source3/modules/vfs_time_audit.c b/source3/modules/vfs_time_audit.c
> index 64e1f5d..5754a07 100644
> --- a/source3/modules/vfs_time_audit.c
> +++ b/source3/modules/vfs_time_audit.c
> @@ -2016,6 +2016,8 @@ static NTSTATUS smb_time_audit_offload_read_recv(
>  		return NT_STATUS_NO_MEMORY;
>  	}
>  
> +	*_token_blob = token_blob;
> +
>  	tevent_req_received(req);
>  	return NT_STATUS_OK;
>  }
> -- 
> 1.8.3.1
> 
> 
> From 86f0afdc6d99e16371cd0a1bb71fa85764a110ef Mon Sep 17 00:00:00 2001
> From: Christof Schmitt <cs at samba.org>
> Date: Thu, 9 Aug 2018 12:40:11 -0700
> Subject: [PATCH 2/2] selftest: Also run smb2.ioctl against simpleserver
> 
> Signed-off-by: Christof Schmitt <cs at samba.org>
> ---
>  source3/selftest/tests.py | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
> index d3c5487..d552f7b 100755
> --- a/source3/selftest/tests.py
> +++ b/source3/selftest/tests.py
> @@ -516,6 +516,7 @@ for t in tests:
>          plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/fs_specific -U$USERNAME%$PASSWORD', 'fs_specific')
>          plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
>          plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD')
> +        plansmbtorture4testsuite(t, "simpleserver", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
>      elif t == "smb2.lock":
>          plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/aio -U$USERNAME%$PASSWORD', 'aio')
>          plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD')
> -- 
> 1.8.3.1
>