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

[Ralph Wuerthner]

On 09.08.2018 21:46, Christof Schmitt wrote:
> 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.

The original intention regarding variable token_blob in 
smb_time_audit_offload_read_recv() is obvious. But from a performance 
perspective this is suboptimal. In my point of view it doesn't make 
sense to allocate a new blob, fill the new blob with the contents of the 
old blob and destroy the old one. Keeping the old blob is much more 
straight forward.
BTW: the corresponding VFS call in vfs_fruit is using the talloc_move() 
pattern too:

static NTSTATUS fruit_offload_read_recv(struct tevent_req *req,
                                         struct vfs_handle_struct *handle,
                                         TALLOC_CTX *mem_ctx,
                                         DATA_BLOB *token)
{
         struct fruit_offload_read_state *state = tevent_req_data(
                 req, struct fruit_offload_read_state);
         NTSTATUS status;

         if (tevent_req_is_nterror(req, &status)) {
                 tevent_req_received(req);
                 return status;
         }

         token->length = state->token.length;
         token->data = talloc_move(mem_ctx, &state->token.data);

         tevent_req_received(req);
         return NT_STATUS_OK;
}

>> 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).
> 
> Christof

You only added smb2.ioctl to the simpleserver environmet. What about 
other smbtorture tests? Do we have a full coverage now for 
vfs_time_audit? Are there any reasons why we shouldn't add 
vfs_time_audit to the other test environments too?

Regards

Ralph