From f088353d37b433af7b979a17871233cccddf7aca Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Mon, 9 Mar 2009 18:04:38 -0400 Subject: Fix potential segfaults using freed memory. In some code paths ltdb_context was still referenced even after we were returned an error by one of the callbacks. Because the interface assumes that once an error is returned the ldb_request may be freed, and because the ltdb_context was allocated as a child of the request, this might cause access to freed memory. Allocate the ltdb_context on ldb, and keep track of what's going on with the request by adding a spy children on it. This way even if the request is freed before the ltdb_callback is called, we will safely free the ctx and just quietly return. --- source4/lib/ldb/ldb_tdb/ldb_index.c | 2 +- source4/lib/ldb/ldb_tdb/ldb_search.c | 6 ++--- source4/lib/ldb/ldb_tdb/ldb_tdb.c | 49 ++++++++++++++++++++++++++++++++---- source4/lib/ldb/ldb_tdb/ldb_tdb.h | 7 +++++- 4 files changed, 54 insertions(+), 10 deletions(-) (limited to 'source4') diff --git a/source4/lib/ldb/ldb_tdb/ldb_index.c b/source4/lib/ldb/ldb_tdb/ldb_index.c index ad27c9a9a9..c99c2936d8 100644 --- a/source4/lib/ldb/ldb_tdb/ldb_index.c +++ b/source4/lib/ldb/ldb_tdb/ldb_index.c @@ -1055,7 +1055,7 @@ static int ltdb_index_filter(const struct dn_list *dn_list, ret = ldb_module_send_entry(ac->req, msg, NULL); if (ret != LDB_SUCCESS) { - ac->callback_failed = true; + ac->request_terminated = true; return ret; } } diff --git a/source4/lib/ldb/ldb_tdb/ldb_search.c b/source4/lib/ldb/ldb_tdb/ldb_search.c index 0f595267fc..d395c28f28 100644 --- a/source4/lib/ldb/ldb_tdb/ldb_search.c +++ b/source4/lib/ldb/ldb_tdb/ldb_search.c @@ -424,10 +424,10 @@ static int search_func(struct tdb_context *tdb, TDB_DATA key, TDB_DATA data, voi ret = ldb_module_send_entry(ac->req, msg, NULL); if (ret != LDB_SUCCESS) { - ac->callback_failed = true; + ac->request_terminated = true; /* the callback failed, abort the operation */ return -1; - } + } return 0; } @@ -544,7 +544,7 @@ int ltdb_search(struct ltdb_context *ctx) /* Check if we got just a normal error. * In that case proceed to a full search unless we got a * callback error */ - if ( ! ctx->callback_failed && ret != LDB_SUCCESS) { + if ( ! ctx->request_terminated && ret != LDB_SUCCESS) { /* Not indexed, so we need to do a full scan */ ret = ltdb_search_full(ctx); if (ret != LDB_SUCCESS) { diff --git a/source4/lib/ldb/ldb_tdb/ldb_tdb.c b/source4/lib/ldb/ldb_tdb/ldb_tdb.c index 24ec06ea32..d38cb828bb 100644 --- a/source4/lib/ldb/ldb_tdb/ldb_tdb.c +++ b/source4/lib/ldb/ldb_tdb/ldb_tdb.c @@ -1019,7 +1019,15 @@ static void ltdb_timeout(struct tevent_context *ev, struct ltdb_context *ctx; ctx = talloc_get_type(private_data, struct ltdb_context); - ltdb_request_done(ctx, LDB_ERR_TIME_LIMIT_EXCEEDED); + if (!ctx->request_terminated) { + /* neutralize the spy */ + ctx->spy->ctx = NULL; + + /* request is done now */ + ltdb_request_done(ctx, LDB_ERR_TIME_LIMIT_EXCEEDED); + } + + talloc_free(ctx); } static void ltdb_request_extended_done(struct ltdb_context *ctx, @@ -1078,6 +1086,11 @@ static void ltdb_callback(struct tevent_context *ev, ctx = talloc_get_type(private_data, struct ltdb_context); + if (!ctx->request_terminated) { + /* neutralize the spy */ + ctx->spy->ctx = NULL; + } else goto done; + switch (ctx->req->operation) { case LDB_SEARCH: ret = ltdb_search(ctx); @@ -1102,11 +1115,24 @@ static void ltdb_callback(struct tevent_context *ev, ret = LDB_ERR_UNWILLING_TO_PERFORM; } - if (!ctx->callback_failed) { - /* Once we are done, we do not need timeout events */ - talloc_free(ctx->timeout_event); + if (!ctx->request_terminated) { + /* request is done now */ ltdb_request_done(ctx, ret); } + +done: + talloc_free(ctx); +} + +static int ltdb_request_destructor(void *ptr) +{ + struct ltdb_req_spy *spy = talloc_get_type(ptr, struct ltdb_req_spy); + + if (spy->ctx != NULL) { + spy->ctx->request_terminated = true; + } + + return 0; } static int ltdb_handle_request(struct ldb_module *module, @@ -1131,7 +1157,7 @@ static int ltdb_handle_request(struct ldb_module *module, ev = ldb_get_event_context(ldb); - ac = talloc_zero(req, struct ltdb_context); + ac = talloc_zero(ldb, struct ltdb_context); if (ac == NULL) { ldb_set_errstring(ldb, "Out of Memory"); return LDB_ERR_OPERATIONS_ERROR; @@ -1144,15 +1170,28 @@ static int ltdb_handle_request(struct ldb_module *module, tv.tv_usec = 0; te = tevent_add_timer(ev, ac, tv, ltdb_callback, ac); if (NULL == te) { + talloc_free(ac); return LDB_ERR_OPERATIONS_ERROR; } tv.tv_sec = req->starttime + req->timeout; ac->timeout_event = tevent_add_timer(ev, ac, tv, ltdb_timeout, ac); if (NULL == ac->timeout_event) { + talloc_free(ac); return LDB_ERR_OPERATIONS_ERROR; } + /* set a spy so that we do not try to use the request context + * if it is freed before ltdb_callback fires */ + ac->spy = talloc(req, struct ltdb_req_spy); + if (NULL == ac->spy) { + talloc_free(ac); + return LDB_ERR_OPERATIONS_ERROR; + } + ac->spy->ctx = ac; + + talloc_set_destructor((TALLOC_CTX *)ac->spy, ltdb_request_destructor); + return LDB_SUCCESS; } diff --git a/source4/lib/ldb/ldb_tdb/ldb_tdb.h b/source4/lib/ldb/ldb_tdb/ldb_tdb.h index 0a06cdb1b0..5a1c8fee2d 100644 --- a/source4/lib/ldb/ldb_tdb/ldb_tdb.h +++ b/source4/lib/ldb/ldb_tdb/ldb_tdb.h @@ -36,11 +36,16 @@ struct ltdb_private { the async local context holds also internal search state during a full db search */ +struct ltdb_req_spy { + struct ltdb_context *ctx; +}; + struct ltdb_context { struct ldb_module *module; struct ldb_request *req; - bool callback_failed; + bool request_terminated; + struct ltdb_req_spy *spy; /* search stuff */ const struct ldb_parse_tree *tree; -- cgit From a67bc41d6bc25974e6aa6d864d5265d07c890413 Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Tue, 10 Mar 2009 15:12:48 +1100 Subject: added a test that triggers a bug in the Samba3 notify code The Samba3 internal notify code doesn't work correctly when there is more than one tree connect to the same smbd process. This change to the RAW-NOTIFY test triggers the bug. --- source4/torture/raw/notify.c | 169 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 169 insertions(+) (limited to 'source4') diff --git a/source4/torture/raw/notify.c b/source4/torture/raw/notify.c index 3ffc58dbe6..c92170cf61 100644 --- a/source4/torture/raw/notify.c +++ b/source4/torture/raw/notify.c @@ -1429,6 +1429,174 @@ done: return ret; } + +/* + create a secondary tree connect - used to test for a bug in Samba3 messaging + with change notify +*/ +static struct smbcli_tree *secondary_tcon(struct smbcli_state *cli, + struct torture_context *tctx) +{ + NTSTATUS status; + const char *share, *host; + struct smbcli_tree *tree; + union smb_tcon tcon; + + share = torture_setting_string(tctx, "share", NULL); + host = torture_setting_string(tctx, "host", NULL); + + printf("create a second tree context on the same session\n"); + tree = smbcli_tree_init(cli->session, tctx, false); + + tcon.generic.level = RAW_TCON_TCONX; + tcon.tconx.in.flags = 0; + tcon.tconx.in.password = data_blob(NULL, 0); + tcon.tconx.in.path = talloc_asprintf(tctx, "\\\\%s\\%s", host, share); + tcon.tconx.in.device = "A:"; + status = smb_raw_tcon(tree, tctx, &tcon); + if (!NT_STATUS_IS_OK(status)) { + talloc_free(tree); + printf("Failed to create secondary tree\n"); + return NULL; + } + + tree->tid = tcon.tconx.out.tid; + printf("tid1=%d tid2=%d\n", cli->tree->tid, tree->tid); + + return tree; +} + + +/* + very simple change notify test +*/ +static bool test_notify_tcon(struct smbcli_state *cli, struct torture_context *torture) +{ + bool ret = true; + NTSTATUS status; + union smb_notify notify; + union smb_open io; + int fnum, fnum2; + struct smbcli_request *req; + extern int torture_numops; + struct smbcli_tree *tree = NULL; + + printf("TESTING SIMPLE CHANGE NOTIFY\n"); + + /* + get a handle on the directory + */ + io.generic.level = RAW_OPEN_NTCREATEX; + io.ntcreatex.in.root_fid = 0; + io.ntcreatex.in.flags = 0; + io.ntcreatex.in.access_mask = SEC_FILE_ALL; + io.ntcreatex.in.create_options = NTCREATEX_OPTIONS_DIRECTORY; + io.ntcreatex.in.file_attr = FILE_ATTRIBUTE_NORMAL; + io.ntcreatex.in.share_access = NTCREATEX_SHARE_ACCESS_READ | NTCREATEX_SHARE_ACCESS_WRITE; + io.ntcreatex.in.alloc_size = 0; + io.ntcreatex.in.open_disposition = NTCREATEX_DISP_OPEN; + io.ntcreatex.in.impersonation = NTCREATEX_IMPERSONATION_ANONYMOUS; + io.ntcreatex.in.security_flags = 0; + io.ntcreatex.in.fname = BASEDIR; + + status = smb_raw_open(cli->tree, torture, &io); + CHECK_STATUS(status, NT_STATUS_OK); + fnum = io.ntcreatex.out.file.fnum; + + status = smb_raw_open(cli->tree, torture, &io); + CHECK_STATUS(status, NT_STATUS_OK); + fnum2 = io.ntcreatex.out.file.fnum; + + /* ask for a change notify, + on file or directory name changes */ + notify.nttrans.level = RAW_NOTIFY_NTTRANS; + notify.nttrans.in.buffer_size = 1000; + notify.nttrans.in.completion_filter = FILE_NOTIFY_CHANGE_NAME; + notify.nttrans.in.file.fnum = fnum; + notify.nttrans.in.recursive = true; + + printf("testing notify mkdir\n"); + req = smb_raw_changenotify_send(cli->tree, ¬ify); + smbcli_mkdir(cli->tree, BASEDIR "\\subdir-name"); + + status = smb_raw_changenotify_recv(req, torture, ¬ify); + CHECK_STATUS(status, NT_STATUS_OK); + + CHECK_VAL(notify.nttrans.out.num_changes, 1); + CHECK_VAL(notify.nttrans.out.changes[0].action, NOTIFY_ACTION_ADDED); + CHECK_WSTR(notify.nttrans.out.changes[0].name, "subdir-name", STR_UNICODE); + + printf("testing notify rmdir\n"); + req = smb_raw_changenotify_send(cli->tree, ¬ify); + smbcli_rmdir(cli->tree, BASEDIR "\\subdir-name"); + + status = smb_raw_changenotify_recv(req, torture, ¬ify); + CHECK_STATUS(status, NT_STATUS_OK); + CHECK_VAL(notify.nttrans.out.num_changes, 1); + CHECK_VAL(notify.nttrans.out.changes[0].action, NOTIFY_ACTION_REMOVED); + CHECK_WSTR(notify.nttrans.out.changes[0].name, "subdir-name", STR_UNICODE); + + printf("SIMPLE CHANGE NOTIFY OK\n"); + + printf("TESTING WITH SECONDARY TCON\n"); + tree = secondary_tcon(cli, torture); + + printf("testing notify mkdir\n"); + req = smb_raw_changenotify_send(cli->tree, ¬ify); + smbcli_mkdir(cli->tree, BASEDIR "\\subdir-name"); + + status = smb_raw_changenotify_recv(req, torture, ¬ify); + CHECK_STATUS(status, NT_STATUS_OK); + + CHECK_VAL(notify.nttrans.out.num_changes, 1); + CHECK_VAL(notify.nttrans.out.changes[0].action, NOTIFY_ACTION_ADDED); + CHECK_WSTR(notify.nttrans.out.changes[0].name, "subdir-name", STR_UNICODE); + + printf("testing notify rmdir\n"); + req = smb_raw_changenotify_send(cli->tree, ¬ify); + smbcli_rmdir(cli->tree, BASEDIR "\\subdir-name"); + + status = smb_raw_changenotify_recv(req, torture, ¬ify); + CHECK_STATUS(status, NT_STATUS_OK); + CHECK_VAL(notify.nttrans.out.num_changes, 1); + CHECK_VAL(notify.nttrans.out.changes[0].action, NOTIFY_ACTION_REMOVED); + CHECK_WSTR(notify.nttrans.out.changes[0].name, "subdir-name", STR_UNICODE); + + printf("CHANGE NOTIFY WITH TCON OK\n"); + + printf("Disconnecting secondary tree\n"); + status = smb_tree_disconnect(tree); + CHECK_STATUS(status, NT_STATUS_OK); + talloc_free(tree); + + printf("testing notify mkdir\n"); + req = smb_raw_changenotify_send(cli->tree, ¬ify); + smbcli_mkdir(cli->tree, BASEDIR "\\subdir-name"); + + status = smb_raw_changenotify_recv(req, torture, ¬ify); + CHECK_STATUS(status, NT_STATUS_OK); + + CHECK_VAL(notify.nttrans.out.num_changes, 1); + CHECK_VAL(notify.nttrans.out.changes[0].action, NOTIFY_ACTION_ADDED); + CHECK_WSTR(notify.nttrans.out.changes[0].name, "subdir-name", STR_UNICODE); + + printf("testing notify rmdir\n"); + req = smb_raw_changenotify_send(cli->tree, ¬ify); + smbcli_rmdir(cli->tree, BASEDIR "\\subdir-name"); + + status = smb_raw_changenotify_recv(req, torture, ¬ify); + CHECK_STATUS(status, NT_STATUS_OK); + CHECK_VAL(notify.nttrans.out.num_changes, 1); + CHECK_VAL(notify.nttrans.out.changes[0].action, NOTIFY_ACTION_REMOVED); + CHECK_WSTR(notify.nttrans.out.changes[0].name, "subdir-name", STR_UNICODE); + + printf("CHANGE NOTIFY WITH TDIS OK\n"); +done: + smb_raw_exit(cli->session); + return ret; +} + + /* basic testing of change notify */ @@ -1442,6 +1610,7 @@ bool torture_raw_notify(struct torture_context *torture, return false; } + ret &= test_notify_tcon(cli, torture); ret &= test_notify_dir(cli, cli2, torture); ret &= test_notify_mask(cli, torture); ret &= test_notify_recursive(cli, torture); -- cgit From 97cccb22194380ce84bf6188df90e2438f1176db Mon Sep 17 00:00:00 2001 From: Simo Sorce Date: Tue, 10 Mar 2009 10:05:52 -0400 Subject: Fix extended operation return path. Extended operations return was not going thorugh the same patch as others leaving the ctx around. Plus we were neutralizing the spy too early so that it didn't set the request_terminated flag as it should have. This should hopefully fix the failures in the build farm. --- source4/lib/ldb/ldb_tdb/ldb_tdb.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) (limited to 'source4') diff --git a/source4/lib/ldb/ldb_tdb/ldb_tdb.c b/source4/lib/ldb/ldb_tdb/ldb_tdb.c index d38cb828bb..9df62be936 100644 --- a/source4/lib/ldb/ldb_tdb/ldb_tdb.c +++ b/source4/lib/ldb/ldb_tdb/ldb_tdb.c @@ -1020,13 +1020,14 @@ static void ltdb_timeout(struct tevent_context *ev, ctx = talloc_get_type(private_data, struct ltdb_context); if (!ctx->request_terminated) { - /* neutralize the spy */ - ctx->spy->ctx = NULL; - /* request is done now */ ltdb_request_done(ctx, LDB_ERR_TIME_LIMIT_EXCEEDED); } + if (!ctx->request_terminated) { + /* neutralize the spy */ + ctx->spy->ctx = NULL; + } talloc_free(ctx); } @@ -1086,10 +1087,9 @@ static void ltdb_callback(struct tevent_context *ev, ctx = talloc_get_type(private_data, struct ltdb_context); - if (!ctx->request_terminated) { - /* neutralize the spy */ - ctx->spy->ctx = NULL; - } else goto done; + if (ctx->request_terminated) { + goto done; + } switch (ctx->req->operation) { case LDB_SEARCH: @@ -1109,7 +1109,7 @@ static void ltdb_callback(struct tevent_context *ev, break; case LDB_EXTENDED: ltdb_handle_extended(ctx); - return; + goto done; default: /* no other op supported */ ret = LDB_ERR_UNWILLING_TO_PERFORM; @@ -1121,6 +1121,10 @@ static void ltdb_callback(struct tevent_context *ev, } done: + if (!ctx->request_terminated) { + /* neutralize the spy */ + ctx->spy->ctx = NULL; + } talloc_free(ctx); } -- cgit