[PATCH] s3: vfs: time_audit: fix handling of token_blob in smb_time_audit_offload_read_recv()
Christof Schmitt
cs at samba.org
Fri Aug 10 21:35:41 UTC 2018
On Fri, Aug 10, 2018 at 02:21:01PM -0700, Christof Schmitt wrote:
> On Fri, Aug 10, 2018 at 09:50:36AM +0200, Ralph Wuerthner wrote:
> > 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;
> > }
>
> Fare enough, we can go with your version to avoid the talloc call.
>
> > 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?
>
> The initial goal was to simply load the modules somewhere to trigger the
> smb_vfs_assert_all_fns check. We can probably just load them everywhere.
> I am running the attached patches through a gitlab pipeline now to see
> if anything else shows up:
> https://gitlab.com/samba-team/devel/samba/pipelines/27670843
Now with attached patches.
Christof
-------------- next part --------------
From 045ae93ef7d1ec98136d169054779f10d70b0e86 Mon Sep 17 00:00:00 2001
From: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
Date: Wed, 8 Aug 2018 17:42:18 +0200
Subject: [PATCH 1/2] s3: vfs: time_audit: fix handling of token_blob in
smb_time_audit_offload_read_recv()
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13568
Signed-off-by: Ralph Wuerthner <ralph.wuerthner at de.ibm.com>
Reviewed-by: Christof Schmitt <cs at samba.org>
---
source3/modules/vfs_time_audit.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/source3/modules/vfs_time_audit.c b/source3/modules/vfs_time_audit.c
index 64e1f5d..aefea33 100644
--- a/source3/modules/vfs_time_audit.c
+++ b/source3/modules/vfs_time_audit.c
@@ -1988,13 +1988,12 @@ static NTSTATUS smb_time_audit_offload_read_recv(
struct tevent_req *req,
struct vfs_handle_struct *handle,
TALLOC_CTX *mem_ctx,
- DATA_BLOB *_token_blob)
+ DATA_BLOB *token_blob)
{
struct time_audit_offload_read_state *state = tevent_req_data(
req, struct time_audit_offload_read_state);
struct timespec ts_recv;
double timediff;
- DATA_BLOB token_blob;
NTSTATUS status;
clock_gettime_mono(&ts_recv);
@@ -2008,13 +2007,8 @@ static NTSTATUS smb_time_audit_offload_read_recv(
return status;
}
- token_blob = data_blob_talloc(mem_ctx,
- state->token_blob.data,
- state->token_blob.length);
- if (token_blob.data == NULL) {
- tevent_req_received(req);
- return NT_STATUS_NO_MEMORY;
- }
+ token_blob->length = state->token_blob.length;
+ token_blob->data = talloc_move(mem_ctx, &state->token_blob.data);
tevent_req_received(req);
return NT_STATUS_OK;
--
1.8.3.1
From df7b9df2dad6f7b08a41e6a85cf03e6e7c31d95e Mon Sep 17 00:00:00 2001
From: Christof Schmitt <cs at samba.org>
Date: Fri, 10 Aug 2018 10:38:28 -0700
Subject: [PATCH 2/2] selftest: Load time_audit and full_audit modules for all
tests
Previously the only test was to load these modules to trigger the
smb_vfs_assert_all_fns check. As these modules just pass through the
calls, they can be loaded for all tests to ensure that the codepaths are
exercised. This would have found the problem in
smb_time_audit_offload_read_recv.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=13568
Signed-off-by: Christof Schmitt <cs at samba.org>
---
selftest/target/Samba3.pm | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/selftest/target/Samba3.pm b/selftest/target/Samba3.pm
index 447c1e8..d90c8f7 100755
--- a/selftest/target/Samba3.pm
+++ b/selftest/target/Samba3.pm
@@ -762,14 +762,10 @@ sub setup_simpleserver
my $simpleserver_options = "
lanman auth = yes
ntlm auth = yes
- vfs objects = xattr_tdb streams_depot time_audit full_audit
+ vfs objects = xattr_tdb streams_depot
change notify = no
smb encrypt = off
- full_audit:syslog = no
- full_audit:success = none
- full_audit:failure = none
-
[vfs_aio_pthread]
path = $prefix_abs/share
read only = no
@@ -1766,7 +1762,11 @@ sub provision($$$$$$$$$)
dos filemode = yes
strict rename = yes
strict sync = yes
- vfs objects = acl_xattr fake_acls xattr_tdb streams_depot
+ vfs objects = acl_xattr fake_acls xattr_tdb streams_depot time_audit full_audit
+
+ full_audit:syslog = no
+ full_audit:success = none
+ full_audit:failure = none
printing = vlp
print command = $bindir_abs/vlp tdbfile=$lockdir/vlp.tdb print %p %s
--
1.8.3.1
More information about the samba-technical
mailing list