From 3a903adaf8f0cca25d557972bc7689a7898a9ccc Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 27 Feb 2020 16:30:51 -0800 Subject: [PATCH 01/19] s3: VFS: vfs_default: Add tevent_req pointer to state struct in vfswrap_pread_state. We will need this to detect when this request is outstanding but has been destroyed in a SHUTDOWN_CLOSE on this file. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/modules/vfs_default.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c index a30f3ba1d31..4bb4adf5f7e 100644 --- a/source3/modules/vfs_default.c +++ b/source3/modules/vfs_default.c @@ -780,6 +780,7 @@ static ssize_t vfswrap_pwrite(vfs_handle_struct *handle, files_struct *fsp, cons } struct vfswrap_pread_state { + struct tevent_req *req; ssize_t ret; int fd; void *buf; @@ -809,6 +810,7 @@ static struct tevent_req *vfswrap_pread_send(struct vfs_handle_struct *handle, return NULL; } + state->req = req; state->ret = -1; state->fd = fsp->fh->fd; state->buf = data; -- 2.25.0.265.gbab2e86ba0-goog From 5fd132bc072e32f3441f2adff915493af7a86c98 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 27 Feb 2020 16:34:51 -0800 Subject: [PATCH 02/19] s3: VFS: vfs_default. Pass in struct vfswrap_pread_state as the callback data to the subreq. Find the req we're finishing off by looking inside vfswrap_pread_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 vfswrap_pread_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: Jeremy Allison --- source3/modules/vfs_default.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c index 4bb4adf5f7e..b8c36180b7c 100644 --- a/source3/modules/vfs_default.c +++ b/source3/modules/vfs_default.c @@ -827,7 +827,7 @@ static struct tevent_req *vfswrap_pread_send(struct vfs_handle_struct *handle, if (tevent_req_nomem(subreq, req)) { return tevent_req_post(req, ev); } - tevent_req_set_callback(subreq, vfs_pread_done, req); + tevent_req_set_callback(subreq, vfs_pread_done, state); talloc_set_destructor(state, vfs_pread_state_destructor); @@ -868,10 +868,9 @@ static int vfs_pread_state_destructor(struct vfswrap_pread_state *state) static void vfs_pread_done(struct tevent_req *subreq) { - struct tevent_req *req = tevent_req_callback_data( - subreq, struct tevent_req); - struct vfswrap_pread_state *state = tevent_req_data( - req, struct vfswrap_pread_state); + struct vfswrap_pread_state *state = tevent_req_callback_data( + subreq, struct vfswrap_pread_state); + struct tevent_req *req = state->req; int ret; ret = pthreadpool_tevent_job_recv(subreq); -- 2.25.0.265.gbab2e86ba0-goog From 792bafe43b80313030eb02af3d4f702508cb3203 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 27 Feb 2020 16:40:46 -0800 Subject: [PATCH 03/19] s3: VFS: vfs_default. Protect vfs_pread_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_pread_done(), just throw away the result if state->req == NULL. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/modules/vfs_default.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c index b8c36180b7c..21bc9c7adf7 100644 --- a/source3/modules/vfs_default.c +++ b/source3/modules/vfs_default.c @@ -863,6 +863,15 @@ static void vfs_pread_do(void *private_data) static int vfs_pread_state_destructor(struct vfswrap_pread_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; } @@ -877,6 +886,17 @@ static void vfs_pread_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("pread request abandoned in flight\n"); + TALLOC_FREE(state); + return; + } if (ret != 0) { if (ret != EAGAIN) { tevent_req_error(req, ret); -- 2.25.0.265.gbab2e86ba0-goog From 4d79e71ae0289df4ce3f786ed0216c896df3d781 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 27 Feb 2020 16:44:39 -0800 Subject: [PATCH 04/19] s3: VFS: vfs_default: Add tevent_req pointer to state struct in vfswrap_pwrite_state. We will need this to detect when this request is outstanding but has been destroyed in a SHUTDOWN_CLOSE on this file. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/modules/vfs_default.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c index 21bc9c7adf7..cbc8335cd12 100644 --- a/source3/modules/vfs_default.c +++ b/source3/modules/vfs_default.c @@ -929,6 +929,7 @@ static ssize_t vfswrap_pread_recv(struct tevent_req *req, } struct vfswrap_pwrite_state { + struct tevent_req *req; ssize_t ret; int fd; const void *buf; @@ -958,6 +959,7 @@ static struct tevent_req *vfswrap_pwrite_send(struct vfs_handle_struct *handle, return NULL; } + state->req = req; state->ret = -1; state->fd = fsp->fh->fd; state->buf = data; -- 2.25.0.265.gbab2e86ba0-goog From a1d4770cb91678e0e09c25602d827e6cf506da07 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 27 Feb 2020 16:49:38 -0800 Subject: [PATCH 05/19] s3: VFS: vfs_default. Pass in struct vfswrap_pwrite_state as the callback data to the subreq. Find the req we're finishing off by looking inside vfswrap_pwrite_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 vfswrap_pwrite_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: Jeremy Allison --- source3/modules/vfs_default.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c index cbc8335cd12..641764e41f1 100644 --- a/source3/modules/vfs_default.c +++ b/source3/modules/vfs_default.c @@ -976,7 +976,7 @@ static struct tevent_req *vfswrap_pwrite_send(struct vfs_handle_struct *handle, if (tevent_req_nomem(subreq, req)) { return tevent_req_post(req, ev); } - tevent_req_set_callback(subreq, vfs_pwrite_done, req); + tevent_req_set_callback(subreq, vfs_pwrite_done, state); talloc_set_destructor(state, vfs_pwrite_state_destructor); @@ -1017,10 +1017,9 @@ static int vfs_pwrite_state_destructor(struct vfswrap_pwrite_state *state) static void vfs_pwrite_done(struct tevent_req *subreq) { - struct tevent_req *req = tevent_req_callback_data( - subreq, struct tevent_req); - struct vfswrap_pwrite_state *state = tevent_req_data( - req, struct vfswrap_pwrite_state); + struct vfswrap_pwrite_state *state = tevent_req_callback_data( + subreq, struct vfswrap_pwrite_state); + struct tevent_req *req = state->req; int ret; ret = pthreadpool_tevent_job_recv(subreq); -- 2.25.0.265.gbab2e86ba0-goog From a72da31e76e848153c5ddd80309a8407e38cee11 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 27 Feb 2020 16:51:35 -0800 Subject: [PATCH 06/19] s3: VFS: vfs_default. Protect vfs_pwrite_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_pwrite_done(), just throw away the result if state->req == NULL. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/modules/vfs_default.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c index 641764e41f1..3425ee31dcb 100644 --- a/source3/modules/vfs_default.c +++ b/source3/modules/vfs_default.c @@ -1012,6 +1012,15 @@ static void vfs_pwrite_do(void *private_data) static int vfs_pwrite_state_destructor(struct vfswrap_pwrite_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; } @@ -1026,6 +1035,17 @@ static void vfs_pwrite_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("pwrite request abandoned in flight\n"); + TALLOC_FREE(state); + return; + } if (ret != 0) { if (ret != EAGAIN) { tevent_req_error(req, ret); -- 2.25.0.265.gbab2e86ba0-goog From 4c522ffe56a6a43761f2acd3b093eaf045d1c998 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 27 Feb 2020 16:53:10 -0800 Subject: [PATCH 07/19] s3: VFS: vfs_default: Add tevent_req pointer to state struct in vfswrap_fsync_state. We will need this to detect when this request is outstanding but has been destroyed in a SHUTDOWN_CLOSE on this file. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/modules/vfs_default.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c index 3425ee31dcb..28b8c04dee4 100644 --- a/source3/modules/vfs_default.c +++ b/source3/modules/vfs_default.c @@ -1078,6 +1078,7 @@ static ssize_t vfswrap_pwrite_recv(struct tevent_req *req, } struct vfswrap_fsync_state { + struct tevent_req *req; ssize_t ret; int fd; @@ -1102,6 +1103,7 @@ static struct tevent_req *vfswrap_fsync_send(struct vfs_handle_struct *handle, return NULL; } + state->req = req; state->ret = -1; state->fd = fsp->fh->fd; -- 2.25.0.265.gbab2e86ba0-goog From 4c86b16d315daff914f9a36a8c52314e03cbd9f8 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 27 Feb 2020 16:54:47 -0800 Subject: [PATCH 08/19] s3: VFS: vfs_default. Pass in struct vfswrap_fsync_state as the callback data to the subreq. Find the req we're finishing off by looking inside vfswrap_fsync_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 vfswrap_fsync_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: Jeremy Allison --- source3/modules/vfs_default.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c index 28b8c04dee4..f9d958a003d 100644 --- a/source3/modules/vfs_default.c +++ b/source3/modules/vfs_default.c @@ -1116,7 +1116,7 @@ static struct tevent_req *vfswrap_fsync_send(struct vfs_handle_struct *handle, if (tevent_req_nomem(subreq, req)) { return tevent_req_post(req, ev); } - tevent_req_set_callback(subreq, vfs_fsync_done, req); + tevent_req_set_callback(subreq, vfs_fsync_done, state); talloc_set_destructor(state, vfs_fsync_state_destructor); @@ -1156,10 +1156,9 @@ static int vfs_fsync_state_destructor(struct vfswrap_fsync_state *state) static void vfs_fsync_done(struct tevent_req *subreq) { - struct tevent_req *req = tevent_req_callback_data( - subreq, struct tevent_req); - struct vfswrap_fsync_state *state = tevent_req_data( - req, struct vfswrap_fsync_state); + struct vfswrap_fsync_state *state = tevent_req_callback_data( + subreq, struct vfswrap_fsync_state); + struct tevent_req *req = state->req; int ret; ret = pthreadpool_tevent_job_recv(subreq); -- 2.25.0.265.gbab2e86ba0-goog From fc4963a15f0a9d607d9d1f6e1d00d4cf91d04167 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 27 Feb 2020 16:56:41 -0800 Subject: [PATCH 09/19] s3: VFS: vfs_default. Protect vfs_fsync_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_fsync_done(), just throw away the result if state->req == NULL. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/modules/vfs_default.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/source3/modules/vfs_default.c b/source3/modules/vfs_default.c index f9d958a003d..fac7fa30ab7 100644 --- a/source3/modules/vfs_default.c +++ b/source3/modules/vfs_default.c @@ -1151,6 +1151,15 @@ static void vfs_fsync_do(void *private_data) static int vfs_fsync_state_destructor(struct vfswrap_fsync_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; } @@ -1165,6 +1174,17 @@ static void vfs_fsync_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("fsync request abandoned in flight\n"); + TALLOC_FREE(state); + return; + } if (ret != 0) { if (ret != EAGAIN) { tevent_req_error(req, ret); -- 2.25.0.265.gbab2e86ba0-goog From b75302b414bffe0aa7127af58444c62f22e8f780 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 28 Feb 2020 15:33:35 -0800 Subject: [PATCH 10/19] s3: VFS: vfs_glusterfs: Add tevent_req pointer to state struct in vfs_gluster_pread_state. We will need this to detect when this request is outstanding but has been destroyed in a SHUTDOWN_CLOSE on this file. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/modules/vfs_glusterfs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source3/modules/vfs_glusterfs.c b/source3/modules/vfs_glusterfs.c index d4b68fba376..6598aadad17 100644 --- a/source3/modules/vfs_glusterfs.c +++ b/source3/modules/vfs_glusterfs.c @@ -713,6 +713,7 @@ static ssize_t vfs_gluster_pread(struct vfs_handle_struct *handle, } struct vfs_gluster_pread_state { + struct tevent_req *req; ssize_t ret; glfs_fd_t *fd; void *buf; @@ -748,6 +749,7 @@ static struct tevent_req *vfs_gluster_pread_send(struct vfs_handle_struct return NULL; } + state->req = req; state->ret = -1; state->fd = glfd; state->buf = data; -- 2.25.0.265.gbab2e86ba0-goog From 5d908bb2b28ff2c39f88548d72523d04fc62332c Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 28 Feb 2020 15:35:46 -0800 Subject: [PATCH 11/19] s3: VFS: vfs_glusterfs. Pass in struct vfs_gluster_pread_state as the callback data to the subreq. Find the req we're finishing off by looking inside vfs_gluster_pread_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 vfs_gluster_pread_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: Jeremy Allison --- source3/modules/vfs_glusterfs.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/source3/modules/vfs_glusterfs.c b/source3/modules/vfs_glusterfs.c index 6598aadad17..84b284152c6 100644 --- a/source3/modules/vfs_glusterfs.c +++ b/source3/modules/vfs_glusterfs.c @@ -766,7 +766,7 @@ static struct tevent_req *vfs_gluster_pread_send(struct vfs_handle_struct if (tevent_req_nomem(subreq, req)) { return tevent_req_post(req, ev); } - tevent_req_set_callback(subreq, vfs_gluster_pread_done, req); + tevent_req_set_callback(subreq, vfs_gluster_pread_done, state); talloc_set_destructor(state, vfs_gluster_pread_state_destructor); @@ -812,10 +812,9 @@ static int vfs_gluster_pread_state_destructor(struct vfs_gluster_pread_state *st static void vfs_gluster_pread_done(struct tevent_req *subreq) { - struct tevent_req *req = tevent_req_callback_data( - subreq, struct tevent_req); - struct vfs_gluster_pread_state *state = tevent_req_data( - req, struct vfs_gluster_pread_state); + struct vfs_gluster_pread_state *state = tevent_req_callback_data( + subreq, struct vfs_gluster_pread_state); + struct tevent_req *req = state->req; int ret; ret = pthreadpool_tevent_job_recv(subreq); -- 2.25.0.265.gbab2e86ba0-goog From 89133b357f1c0900356302d4c01755d85a1151b0 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 28 Feb 2020 15:38:04 -0800 Subject: [PATCH 12/19] s3: VFS: vfs_glusterfs. Protect vfs_gluster_pread_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_gluster_pread_done(), just throw away the result if state->req == NULL. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/modules/vfs_glusterfs.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/source3/modules/vfs_glusterfs.c b/source3/modules/vfs_glusterfs.c index 84b284152c6..7924f123cca 100644 --- a/source3/modules/vfs_glusterfs.c +++ b/source3/modules/vfs_glusterfs.c @@ -807,6 +807,15 @@ static void vfs_gluster_pread_do(void *private_data) static int vfs_gluster_pread_state_destructor(struct vfs_gluster_pread_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; } @@ -821,6 +830,17 @@ static void vfs_gluster_pread_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("gluster pread request abandoned in flight\n"); + TALLOC_FREE(state); + return; + } if (ret != 0) { if (ret != EAGAIN) { tevent_req_error(req, ret); -- 2.25.0.265.gbab2e86ba0-goog From f54e312e7ff389dc0cde9419e480ad79629705a1 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 28 Feb 2020 15:47:52 -0800 Subject: [PATCH 13/19] s3: VFS: vfs_glusterfs: Add tevent_req pointer to state struct in vfs_gluster_pwrite_state. We will need this to detect when this request is outstanding but has been destroyed in a SHUTDOWN_CLOSE on this file. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/modules/vfs_glusterfs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source3/modules/vfs_glusterfs.c b/source3/modules/vfs_glusterfs.c index 7924f123cca..456e0c8a498 100644 --- a/source3/modules/vfs_glusterfs.c +++ b/source3/modules/vfs_glusterfs.c @@ -873,6 +873,7 @@ static ssize_t vfs_gluster_pread_recv(struct tevent_req *req, } struct vfs_gluster_pwrite_state { + struct tevent_req *req; ssize_t ret; glfs_fd_t *fd; const void *buf; @@ -908,6 +909,7 @@ static struct tevent_req *vfs_gluster_pwrite_send(struct vfs_handle_struct return NULL; } + state->req = req; state->ret = -1; state->fd = glfd; state->buf = data; -- 2.25.0.265.gbab2e86ba0-goog From e4b59851161d5c639a565e2a03952bf52a7e5b5b Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 28 Feb 2020 15:53:19 -0800 Subject: [PATCH 14/19] s3: VFS: vfs_glusterfs. Pass in struct vfs_gluster_pwrite_state as the callback data to the subreq. Find the req we're finishing off by looking inside vfs_gluster_pwrite_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 vfs_gluster_pwrite_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: Jeremy Allison --- source3/modules/vfs_glusterfs.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/source3/modules/vfs_glusterfs.c b/source3/modules/vfs_glusterfs.c index 456e0c8a498..52c33725b8d 100644 --- a/source3/modules/vfs_glusterfs.c +++ b/source3/modules/vfs_glusterfs.c @@ -926,7 +926,7 @@ static struct tevent_req *vfs_gluster_pwrite_send(struct vfs_handle_struct if (tevent_req_nomem(subreq, req)) { return tevent_req_post(req, ev); } - tevent_req_set_callback(subreq, vfs_gluster_pwrite_done, req); + tevent_req_set_callback(subreq, vfs_gluster_pwrite_done, state); talloc_set_destructor(state, vfs_gluster_pwrite_state_destructor); @@ -972,10 +972,9 @@ static int vfs_gluster_pwrite_state_destructor(struct vfs_gluster_pwrite_state * static void vfs_gluster_pwrite_done(struct tevent_req *subreq) { - struct tevent_req *req = tevent_req_callback_data( - subreq, struct tevent_req); - struct vfs_gluster_pwrite_state *state = tevent_req_data( - req, struct vfs_gluster_pwrite_state); + struct vfs_gluster_pwrite_state *state = tevent_req_callback_data( + subreq, struct vfs_gluster_pwrite_state); + struct tevent_req *req = state->req; int ret; ret = pthreadpool_tevent_job_recv(subreq); -- 2.25.0.265.gbab2e86ba0-goog From 6bfe04ec2dc4fe2790ad72169c0f61dcc6081763 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 28 Feb 2020 15:55:36 -0800 Subject: [PATCH 15/19] s3: VFS: vfs_glusterfs. Protect vfs_gluster_pwrite_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_gluster_pwrite_done(), just throw away the result if state->req == NULL. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/modules/vfs_glusterfs.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/source3/modules/vfs_glusterfs.c b/source3/modules/vfs_glusterfs.c index 52c33725b8d..4e978f168d6 100644 --- a/source3/modules/vfs_glusterfs.c +++ b/source3/modules/vfs_glusterfs.c @@ -967,6 +967,15 @@ static void vfs_gluster_pwrite_do(void *private_data) static int vfs_gluster_pwrite_state_destructor(struct vfs_gluster_pwrite_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; } @@ -981,6 +990,17 @@ static void vfs_gluster_pwrite_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("gluster pwrite request abandoned in flight\n"); + TALLOC_FREE(state); + return; + } if (ret != 0) { if (ret != EAGAIN) { tevent_req_error(req, ret); -- 2.25.0.265.gbab2e86ba0-goog From 972857e66c767d2fe1485a3ad76165790264a37c Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 28 Feb 2020 15:57:20 -0800 Subject: [PATCH 16/19] s3: VFS: vfs_glusterfs: Add tevent_req pointer to state struct in vfs_gluster_fsync_state. We will need this to detect when this request is outstanding but has been destroyed in a SHUTDOWN_CLOSE on this file. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/modules/vfs_glusterfs.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/source3/modules/vfs_glusterfs.c b/source3/modules/vfs_glusterfs.c index 4e978f168d6..d5d402d72ab 100644 --- a/source3/modules/vfs_glusterfs.c +++ b/source3/modules/vfs_glusterfs.c @@ -1114,6 +1114,7 @@ static int vfs_gluster_renameat(struct vfs_handle_struct *handle, } struct vfs_gluster_fsync_state { + struct tevent_req *req; ssize_t ret; glfs_fd_t *fd; @@ -1144,6 +1145,7 @@ static struct tevent_req *vfs_gluster_fsync_send(struct vfs_handle_struct return NULL; } + state->req = req; state->ret = -1; state->fd = glfd; -- 2.25.0.265.gbab2e86ba0-goog From 47a0a26968b69d09717f0f4b4e07881e33027113 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 28 Feb 2020 15:59:16 -0800 Subject: [PATCH 17/19] s3: VFS: vfs_glusterfs. Pass in struct vfs_gluster_fsync_state as the callback data to the subreq. Find the req we're finishing off by looking inside vfs_gluster_fsync_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 vfs_gluster_fsync_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: Jeremy Allison --- source3/modules/vfs_glusterfs.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/source3/modules/vfs_glusterfs.c b/source3/modules/vfs_glusterfs.c index d5d402d72ab..4706e6f9189 100644 --- a/source3/modules/vfs_glusterfs.c +++ b/source3/modules/vfs_glusterfs.c @@ -1158,7 +1158,7 @@ static struct tevent_req *vfs_gluster_fsync_send(struct vfs_handle_struct if (tevent_req_nomem(subreq, req)) { return tevent_req_post(req, ev); } - tevent_req_set_callback(subreq, vfs_gluster_fsync_done, req); + tevent_req_set_callback(subreq, vfs_gluster_fsync_done, state); talloc_set_destructor(state, vfs_gluster_fsync_state_destructor); @@ -1202,10 +1202,9 @@ static int vfs_gluster_fsync_state_destructor(struct vfs_gluster_fsync_state *st static void vfs_gluster_fsync_done(struct tevent_req *subreq) { - struct tevent_req *req = tevent_req_callback_data( - subreq, struct tevent_req); - struct vfs_gluster_fsync_state *state = tevent_req_data( - req, struct vfs_gluster_fsync_state); + struct vfs_gluster_fsync_state *state = tevent_req_callback_data( + subreq, struct vfs_gluster_fsync_state); + struct tevent_req *req = state->req; int ret; ret = pthreadpool_tevent_job_recv(subreq); -- 2.25.0.265.gbab2e86ba0-goog From ec69274a71c4a4a4271b0225f541fdf0b9dd92fb Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Fri, 28 Feb 2020 16:01:11 -0800 Subject: [PATCH 18/19] s3: VFS: vfs_glusterfs. Protect vfs_gluster_fsync_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_gluster_fsync_done(), just throw away the result if state->req == NULL. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/modules/vfs_glusterfs.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/source3/modules/vfs_glusterfs.c b/source3/modules/vfs_glusterfs.c index 4706e6f9189..b5300282b7b 100644 --- a/source3/modules/vfs_glusterfs.c +++ b/source3/modules/vfs_glusterfs.c @@ -1197,6 +1197,15 @@ static void vfs_gluster_fsync_do(void *private_data) static int vfs_gluster_fsync_state_destructor(struct vfs_gluster_fsync_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; } @@ -1211,6 +1220,17 @@ static void vfs_gluster_fsync_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("gluster fsync request abandoned in flight\n"); + TALLOC_FREE(state); + return; + } if (ret != 0) { if (ret != EAGAIN) { tevent_req_error(req, ret); -- 2.25.0.265.gbab2e86ba0-goog From d538874e69dcd32c6e59cd1819d4b2507667eaf5 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Mon, 2 Mar 2020 13:11:06 -0800 Subject: [PATCH 19/19] s3: smbd: Make sure we correctly reply to outstanding aio requests with an error on SHUTDOWN_CLOSE. SHUTDOWN_CLOSE can be called when smbcontrol close-share is used to terminate active connections. 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. BUG: https://bugzilla.samba.org/show_bug.cgi?id=14301 Signed-off-by: Jeremy Allison --- source3/smbd/close.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/source3/smbd/close.c b/source3/smbd/close.c index f45371e656c..c7be0c8d447 100644 --- a/source3/smbd/close.c +++ b/source3/smbd/close.c @@ -652,6 +652,7 @@ static NTSTATUS close_normal_file(struct smb_request *req, files_struct *fsp, bool is_durable = false; if (fsp->num_aio_requests != 0) { + unsigned num_requests = fsp->num_aio_requests; if (close_type != SHUTDOWN_CLOSE) { /* @@ -681,13 +682,30 @@ static NTSTATUS close_normal_file(struct smb_request *req, files_struct *fsp, 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]. + * 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. */ - talloc_free(fsp->aio_requests[0]); + 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"); + } } } -- 2.25.0.265.gbab2e86ba0-goog