> On Wed, 2011-04-27 at 17:45 +0200, Jakub Hrozek wrote: > > On 04/27/2011 03:30 PM, Stephen Gallagher wrote: > > > On Tue, 2011-04-19 at 14:57 -0400, Stephen Gallagher wrote: > > >> On Tue, 2011-04-12 at 12:33 +0200, Jan Zelený wrote: > > >>> Can someone please take a look at the patch and ack that all issues > > >>> discovered by Simo have been corrected? > > >>> > > >>> Also note that in the original email there is another patch, which > > >>> hasn't been reviewed yet. > > >> > > >> Ack to both patches and pushed to master. > > > > > > Unfortunately, I've now discovered several issues with this patch (too > > > late). > > > > > > First, it causes a crash if SSSD started up in offline mode (e.g. > > > disconnected laptop) and then goes online (e.g. VPN). > > > > > > #0 0x00007fec7adddd28 in sdap_check_online_done (req=0x0) > > > at ../src/providers/ldap/ldap_id.c:729 > > > #1 0x00007fec7ae0f3e1 in sdap_cli_auth_done (subreq=0x0) > > > at ../src/providers/ldap/sdap_async_connection.c:1430 > > > #2 0x00007fec7ae0e207 in sdap_auth_done (subreq=0x1ea9570) > > > at ../src/providers/ldap/sdap_async_connection.c:1007 > > > #3 0x00007fec7ae0c8a9 in simple_bind_done (op=0x1ea9880, > > > reply=0x1ea97f0, error=0, pvt=0x1ea9570) > > > at ../src/providers/ldap/sdap_async_connection.c:540 > > > #4 0x00007fec7adf4aa9 in sdap_process_message (ev=0x1e74470, > > > sh=0x1eb2c70, msg=0x1eb33e0) at ../src/providers/ldap/sdap_async.c:364 > > > #5 0x00007fec7adf4363 in sdap_process_result (ev=0x1e74470, > > > pvt=0x1eb2c70) at ../src/providers/ldap/sdap_async.c:207 > > > #6 0x00007fec7adf3db6 in sdap_ldap_result (ev=0x1e74470, > > > fde=0x1eb3140, flags=1, pvt=0x1eb2c70) at > > > ../src/providers/ldap/sdap_async.c:152 #7 0x000000389ee05d57 in ?? () > > > from /usr/lib64/libtevent.so.0 #8 0x000000389ee035b0 in > > > _tevent_loop_once () > > > from /usr/lib64/libtevent.so.0 > > > #9 0x000000389ee0373b in tevent_common_loop_wait () > > > from /usr/lib64/libtevent.so.0 > > > #10 0x000000000043d41f in server_loop (main_ctx=0x1e755e0) > > > at ../src/util/server.c:526 > > > #11 0x000000000040f8d2 in main (argc=6, argv=0x7fffbb9c67a8) > > > at ../src/providers/data_provider_be.c:1284 > > > > > > > > > The problem is that ctx->srv_opts is NULL here (since we've never made > > > a connection to the server before this). > > > > > > While I was investigating this, I noticed something else as well. This > > > entire IF block is completely useless as written. In the IF block, you > > > set ctx->srv_opts attributes, but then immediately after this block, > > > you replace the contents of ctx->srv_opts with the returned srv_opts > > > from sdap_cli_connect_recv() > > > > > > My guess here is that the assignments in the IF block were supposed to > > > be setting srv_opts instead of ctx->srv_opts. The only part that > > > confuses me is where you're setting ctx->srv_opts->last_usn to > > > srv_opts->last_usn. I thought we had covered before that if the value > > > of srv_opts->last_usn was ever lower than the stored value, we MUST set > > > it to zero. This is because we know that the USN values are not in line > > > with the ones we have stored, so we have to do a full update to be > > > safe. > > > > > > I'm attaching a patch that I think will resolve the issue (though I > > > can't test it as I don't have access to a server with USN values). At > > > the very least, it resolves the crash above for me. > > > > Ack > > I'm going to hold off pushing this until I get buy-in from Jan.
I'm sending my version of the correction. I already tested it and it seems to work for me. Stephen, can you run your test, which caused the crash? This patch already contains handling of the situation that came to my mind today. See commit message for some details. Jan
From 8d6b499fa0356c420d60d13e0a83b8dddf39378c Mon Sep 17 00:00:00 2001
From: Jan Zeleny <jzel...@redhat.com>
Date: Fri, 29 Apr 2011 09:45:10 -0400
Subject: [PATCH] Fixed lastUSN checking improvements
This patch fixes some issues with setting lastUSN attribute and it adds
check against the highest user/group USN after enumeration to keep
better track of the real highest USN. Optimal solution here would be to
schedule a check of rootDSE entry right after the enumeration finishes,
but for the moment this is good enough.
---
src/providers/ldap/ldap_id.c | 9 ++++++---
src/providers/ldap/ldap_id_enum.c | 15 +++++++++++++++
src/providers/ldap/sdap_id_op.c | 4 ++--
3 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
index de6183335bb47eb7508b8e4f18ea055caec7822d..a3c9c0cd451cb7290cefcd5f18d6c17c2e71319e 100644
--- a/src/providers/ldap/ldap_id.c
+++ b/src/providers/ldap/ldap_id.c
@@ -726,9 +726,12 @@ static void sdap_check_online_done(struct tevent_req *req)
} else {
dp_err = DP_ERR_OK;
- if (strcmp(srv_opts->server_id, ctx->srv_opts->server_id) == 0 &&
- srv_opts->supports_usn &&
- ctx->srv_opts->last_usn > srv_opts->last_usn) {
+ if (!ctx->srv_opts) {
+ srv_opts->max_user_value = 0;
+ srv_opts->max_group_value = 0;
+ } else if (strcmp(srv_opts->server_id, ctx->srv_opts->server_id) == 0
+ && srv_opts->supports_usn
+ && ctx->srv_opts->last_usn > srv_opts->last_usn) {
ctx->srv_opts->max_user_value = 0;
ctx->srv_opts->max_group_value = 0;
ctx->srv_opts->last_usn = srv_opts->last_usn;
diff --git a/src/providers/ldap/ldap_id_enum.c b/src/providers/ldap/ldap_id_enum.c
index d7dd33e43fb8c1498d09641451dda8c2f79faac1..68d113bf96a6db226c8a9541f5dbbd543ef043ae 100644
--- a/src/providers/ldap/ldap_id_enum.c
+++ b/src/providers/ldap/ldap_id_enum.c
@@ -511,6 +511,8 @@ static void enum_users_op_done(struct tevent_req *subreq)
struct enum_users_state *state = tevent_req_data(req,
struct enum_users_state);
char *usn_value;
+ char *endptr = NULL;
+ unsigned usn_number;
int ret;
ret = sdap_get_users_recv(subreq, state, &usn_value);
@@ -523,6 +525,12 @@ static void enum_users_op_done(struct tevent_req *subreq)
if (usn_value) {
talloc_zfree(state->ctx->srv_opts->max_user_value);
state->ctx->srv_opts->max_user_value = talloc_steal(state->ctx, usn_value);
+
+ usn_number = strtoul(usn_value, &endptr, 10);
+ if ((endptr == NULL || (*endptr == '\0' && endptr != usn_value))
+ && (usn_number > state->ctx->srv_opts->last_usn)) {
+ state->ctx->srv_opts->last_usn = usn_number;
+ }
}
DEBUG(4, ("Users higher USN value: [%s]\n",
@@ -629,6 +637,8 @@ static void enum_groups_op_done(struct tevent_req *subreq)
struct enum_groups_state *state = tevent_req_data(req,
struct enum_groups_state);
char *usn_value;
+ char *endptr = NULL;
+ unsigned usn_number;
int ret;
ret = sdap_get_groups_recv(subreq, state, &usn_value);
@@ -642,6 +652,11 @@ static void enum_groups_op_done(struct tevent_req *subreq)
talloc_zfree(state->ctx->srv_opts->max_group_value);
state->ctx->srv_opts->max_group_value =
talloc_steal(state->ctx, usn_value);
+ usn_number = strtoul(usn_value, &endptr, 10);
+ if ((endptr == NULL || (*endptr == '\0' && endptr != usn_value))
+ && (usn_number > state->ctx->srv_opts->last_usn)) {
+ state->ctx->srv_opts->last_usn = usn_number;
+ }
}
DEBUG(4, ("Groups higher USN value: [%s]\n",
diff --git a/src/providers/ldap/sdap_id_op.c b/src/providers/ldap/sdap_id_op.c
index 1f692a158e9f043ff905ad1a613cfa4a0eb95171..11a379cc96a77615bd62b1fe58daa2413f38087a 100644
--- a/src/providers/ldap/sdap_id_op.c
+++ b/src/providers/ldap/sdap_id_op.c
@@ -537,9 +537,9 @@ static void sdap_id_op_connect_done(struct tevent_req *subreq)
current_srv_opts->last_usn > srv_opts->last_usn) {
DEBUG(5, ("Server was probably re-initialized\n"));
- current_srv_opts->max_user_value= 0;
+ current_srv_opts->max_user_value = 0;
current_srv_opts->max_group_value = 0;
- current_srv_opts->last_usn = 0;
+ current_srv_opts->last_usn = srv_opts->last_usn;
}
}
ret = sdap_id_conn_data_set_expire_timer(conn_data);
--
1.7.4.1
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
