The branch, master has been updated
via 0ae4f368c6c smbd: reuse close_free_pending_aio() in
close_directory()
via f94cd10a211 smbd: call tevent_req_nterror() for directories when
cleaning up pending aio
via acb0b017613 smbd: move pending aio cleanup to a helper function
via 95cfcda13fe vfs_default: Protect vfs_getxattrat_done() from
accessing a freed req pointer
via 0e894f3e482 vfs_default: pass in state as the callback data to the
subreq
from 54c21a99e6c winexe: add configure option to control whether to
build it (default: auto)
https://git.samba.org/?p=samba.git;a=shortlog;h=master
- Log -----------------------------------------------------------------
commit 0ae4f368c6c8d2c8c7aa34069007a984055df0da
Author: Ralph Boehme <s...@samba.org>
Date: Mon Mar 9 11:18:23 2020 +0100
smbd: reuse close_free_pending_aio() in close_directory()
A directory fsp can have outstanding aio requests as well.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301
Signed-off-by: Ralph Boehme <s...@samba.org>
Reviewed-by: Jeremy Allison <j...@samba.org>
Autobuild-User(master): Jeremy Allison <j...@samba.org>
Autobuild-Date(master): Mon Mar 9 19:34:27 UTC 2020 on sn-devel-184
commit f94cd10a211e2eae966ba4bd26921556bbe513fc
Author: Ralph Boehme <s...@samba.org>
Date: Mon Mar 9 11:31:00 2020 +0100
smbd: call tevent_req_nterror() for directories when cleaning up pending aio
smbd_smb2_query_directory_recv() calls tevent_req_is_nterror() which
requires a
NTSTATUS error code.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301
Signed-off-by: Ralph Boehme <s...@samba.org>
Reviewed-by: Jeremy Allison <j...@samba.org>
commit acb0b01761330864a23932f643f7ad4e3d374634
Author: Ralph Boehme <s...@samba.org>
Date: Mon Mar 9 11:16:37 2020 +0100
smbd: move pending aio cleanup to a helper function
We'll be reusing this from close_directory() in the next commit.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301
Signed-off-by: Ralph Boehme <s...@samba.org>
Reviewed-by: Jeremy Allison <j...@samba.org>
commit 95cfcda13fe9a70b9955a7c44173d619eacb34c1
Author: Ralph Boehme <s...@samba.org>
Date: Mon Mar 9 11:54:37 2020 +0100
vfs_default: Protect vfs_getxattrat_done() from accessing a freed req
pointer
If the fsp is forced closed by a SHUTDOWN_CLOSE whilst the request is in
flight (share forced closed by smbcontrol), then we set state->req = NULL
in the
state destructor.
The existing state destructor prevents the state memory from being freed, so
when the thread completes and calls vfs_getxattrat_done(), just throw away
the result
if state->req == NULL.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301
Signed-off-by: Ralph Boehme <s...@samba.org>
Reviewed-by: Jeremy Allison <j...@samba.org>
commit 0e894f3e48285415f72cf7a68e26f1802fe8045d
Author: Ralph Boehme <s...@samba.org>
Date: Mon Mar 9 11:54:28 2020 +0100
vfs_default: pass in state as the callback data to the subreq
Find the req we're finishing off by looking inside the state. In a shutdown
close the caller calls talloc_free(req), so we can't access it directly as
callback data.
The next commit will NULL out the state->req pointer when a caller calls
talloc_free(req), and the request is still in flight.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301
Signed-off-by: Ralph Boehme <s...@samba.org>
Reviewed-by: Jeremy Allison <j...@samba.org>
-----------------------------------------------------------------------
Summary of changes:
source3/modules/vfs_default.c | 28 ++++++--
source3/smbd/close.c | 152 ++++++++++++++++++++----------------------
2 files changed, 94 insertions(+), 86 deletions(-)
Changeset truncated at 500 lines:
diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c
index fac7fa30ab7..cf24e66e048 100644
--- a/source3/modules/vfs_default.c
+++ b/source3/modules/vfs_default.c
@@ -3289,6 +3289,15 @@ struct vfswrap_getxattrat_state {
static int vfswrap_getxattrat_state_destructor(
struct vfswrap_getxattrat_state *state)
{
+ /*
+ * This destructor only gets called if the request is still
+ * in flight, which is why we deny it by returning -1. We
+ * also set the req pointer to NULL so the _done function
+ * can detect the caller doesn't want the result anymore.
+ *
+ * Forcing the fsp closed by a SHUTDOWN_CLOSE can cause this.
+ */
+ state->req = NULL;
return -1;
}
@@ -3408,7 +3417,7 @@ static struct tevent_req *vfswrap_getxattrat_send(
if (tevent_req_nomem(subreq, req)) {
return tevent_req_post(req, ev);
}
- tevent_req_set_callback(subreq, vfswrap_getxattrat_done, req);
+ tevent_req_set_callback(subreq, vfswrap_getxattrat_done, state);
talloc_set_destructor(state, vfswrap_getxattrat_state_destructor);
@@ -3503,10 +3512,9 @@ end_profile:
static void vfswrap_getxattrat_done(struct tevent_req *subreq)
{
- struct tevent_req *req = tevent_req_callback_data(
- subreq, struct tevent_req);
- struct vfswrap_getxattrat_state *state = tevent_req_data(
- req, struct vfswrap_getxattrat_state);
+ struct vfswrap_getxattrat_state *state = tevent_req_callback_data(
+ subreq, struct vfswrap_getxattrat_state);
+ struct tevent_req *req = state->req;
int ret;
bool ok;
@@ -3520,6 +3528,16 @@ static void vfswrap_getxattrat_done(struct tevent_req
*subreq)
TALLOC_FREE(subreq);
SMBPROFILE_BYTES_ASYNC_END(state->profile_bytes);
talloc_set_destructor(state, NULL);
+ if (req == NULL) {
+ /*
+ * We were shutdown closed in flight. No one wants the result,
+ * and state has been reparented to the NULL context, so just
+ * free it so we don't leak memory.
+ */
+ DBG_NOTICE("getxattr request abandoned in flight\n");
+ TALLOC_FREE(state);
+ return;
+ }
if (ret != 0) {
if (ret != EAGAIN) {
tevent_req_error(req, ret);
diff --git a/source3/smbd/close.c b/source3/smbd/close.c
index c7be0c8d447..73f9c75762d 100644
--- a/source3/smbd/close.c
+++ b/source3/smbd/close.c
@@ -31,6 +31,7 @@
#include "auth.h"
#include "messages.h"
#include "../librpc/gen_ndr/open_files.h"
+#include "lib/util/tevent_ntstatus.h"
/****************************************************************************
Run a file if it is a magic script.
@@ -635,6 +636,74 @@ static NTSTATUS ntstatus_keeperror(NTSTATUS s1, NTSTATUS
s2)
return s2;
}
+static void close_free_pending_aio(struct files_struct *fsp,
+ enum file_close_type close_type)
+{
+ unsigned num_requests = fsp->num_aio_requests;
+
+ if (num_requests == 0) {
+ return;
+ }
+
+ if (close_type != SHUTDOWN_CLOSE) {
+ /*
+ * reply_close and the smb2 close must have
+ * taken care of this. No other callers of
+ * close_file should ever have created async
+ * I/O.
+ *
+ * We need to panic here because if we close()
+ * the fd while we have outstanding async I/O
+ * requests, in the worst case we could end up
+ * writing to the wrong file.
+ */
+ DBG_ERR("fsp->num_aio_requests=%u\n", num_requests);
+ smb_panic("can not close with outstanding aio requests");
+ return;
+ }
+
+ /*
+ * For shutdown close, just drop the async requests
+ * including a potential close request pending for
+ * this fsp. Drop the close request first, the
+ * destructor for the aio_requests would execute it.
+ */
+ TALLOC_FREE(fsp->deferred_close);
+
+ while (fsp->num_aio_requests != 0) {
+ /*
+ * Previously we just called talloc_free()
+ * on the outstanding request, but this
+ * caused crashes (before the async callback
+ * functions were fixed not to reference req
+ * directly) and also leaves the SMB2 request
+ * outstanding on the processing queue.
+ *
+ * Using tevent_req_[nt]error() instead
+ * causes the outstanding SMB1/2/3 request to
+ * return with NT_STATUS_INVALID_HANDLE
+ * and removes it from the processing queue.
+ *
+ * The callback function called from this
+ * calls talloc_free(req). The destructor will remove
+ * itself from the fsp and the aio_requests array.
+ */
+ if (fsp->is_directory) {
+ tevent_req_nterror(fsp->aio_requests[0],
+ NT_STATUS_INVALID_HANDLE);
+ } else {
+ tevent_req_error(fsp->aio_requests[0], EBADF);
+ }
+
+ /* Paranoia to ensure we don't spin. */
+ num_requests--;
+ if (fsp->num_aio_requests != num_requests) {
+ smb_panic("cannot cancel outstanding aio "
+ "requests");
+ }
+ }
+}
+
/****************************************************************************
Close a file.
@@ -651,63 +720,7 @@ static NTSTATUS close_normal_file(struct smb_request *req,
files_struct *fsp,
connection_struct *conn = fsp->conn;
bool is_durable = false;
- if (fsp->num_aio_requests != 0) {
- unsigned num_requests = fsp->num_aio_requests;
-
- if (close_type != SHUTDOWN_CLOSE) {
- /*
- * reply_close and the smb2 close must have
- * taken care of this. No other callers of
- * close_file should ever have created async
- * I/O.
- *
- * We need to panic here because if we close()
- * the fd while we have outstanding async I/O
- * requests, in the worst case we could end up
- * writing to the wrong file.
- */
- DEBUG(0, ("fsp->num_aio_requests=%u\n",
- fsp->num_aio_requests));
- smb_panic("can not close with outstanding aio "
- "requests");
- }
-
- /*
- * For shutdown close, just drop the async requests
- * including a potential close request pending for
- * this fsp. Drop the close request first, the
- * destructor for the aio_requests would execute it.
- */
- TALLOC_FREE(fsp->deferred_close);
-
- while (fsp->num_aio_requests != 0) {
- /*
- * Previously we just called talloc_free()
- * on the outstanding request, but this
- * caused crashes (before the async callback
- * functions were fixed not to reference req
- * directly) and also leaves the SMB2 request
- * outstanding on the processing queue.
- *
- * Using tevent_req_error() instead
- * causes the outstanding SMB1/2/3 request to
- * return with NT_STATUS_INVALID_HANDLE
- * and removes it from the processing queue.
- *
- * The callback function called from this
- * calls talloc_free(req). The destructor will remove
- * itself from the fsp and the aio_requests array.
- */
- tevent_req_error(fsp->aio_requests[0], EBADF);
-
- /* Paranoia to ensure we don't spin. */
- num_requests--;
- if (fsp->num_aio_requests != num_requests) {
- smb_panic("cannot cancel outstanding aio "
- "requests");
- }
- }
- }
+ close_free_pending_aio(fsp, close_type);
while (talloc_array_length(fsp->blocked_smb1_lock_reqs) != 0) {
smbd_smb1_brl_finish_by_req(
@@ -1152,30 +1165,7 @@ static NTSTATUS close_directory(struct smb_request *req,
files_struct *fsp,
notify_status = NT_STATUS_OK;
}
- if (fsp->num_aio_requests != 0) {
- if (close_type != SHUTDOWN_CLOSE) {
- /*
- * We panic here because if we close() the fd while we
- * have outstanding async I/O requests, an async IO
- * request might use the fd. For directories the fd is
- * read-only, so this is not as bad as with files, but
- * still, better safe then sorry.
- */
- DBG_ERR("fsp->num_aio_requests=%u\n",
- fsp->num_aio_requests);
- smb_panic("close with outstanding aio requests");
- return NT_STATUS_INTERNAL_ERROR;
- }
-
- while (fsp->num_aio_requests != 0) {
- /*
- * The destructor of the req will remove itself from the
- * fsp. Don't use TALLOC_FREE here, this will overwrite
- * what the destructor just wrote into aio_requests[0].
- */
- talloc_free(fsp->aio_requests[0]);
- }
- }
+ close_free_pending_aio(fsp, close_type);
/*
* NT can set delete_on_close of the last open
--
Samba Shared Repository
