ehlo,
Attached patches should fix https://fedorahosted.org/sssd/ticket/1980
The first patch adds check after sysdb_getnetgr. If sysdb_getnetgr returns more result than 1, sssd will return error. sysdb_getpwnam has already had this check.
The second patch removes function call sss_cmd_done inside of check_cache, because function is sss_cmd_done is called in parent functions. This was a reason of sssd crash.
How to reproduce this crash. 1.Add Netgroup to sysdb cache with base cn=Netgroups,cn=<domain>,cn=sysdb This netgroup should have the same attribute (name or nameAlias or memberOf) as another netgroup. 2. call sudo with user, which is member of ^^^ netgroup.
Those patches fix only sssd crash, but we should find out: Why were those netgroups stored in sysdb.
LS
On Tue, Jun 25, 2013 at 10:36:14AM +0200, Lukas Slebodnik wrote:
ehlo,
Attached patches should fix https://fedorahosted.org/sssd/ticket/1980
The first patch adds check after sysdb_getnetgr. If sysdb_getnetgr returns more result than 1, sssd will return error. sysdb_getpwnam has already had this check.
The second patch removes function call sss_cmd_done inside of check_cache, because function is sss_cmd_done is called in parent functions. This was a reason of sssd crash.
How to reproduce this crash. 1.Add Netgroup to sysdb cache with base cn=Netgroups,cn=<domain>,cn=sysdb This netgroup should have the same attribute (name or nameAlias or memberOf) as another netgroup. 2. call sudo with user, which is member of ^^^ netgroup.
Those patches fix only sssd crash, but we should find out: Why were those netgroups stored in sysdb.
LS
From 5877db33fc0d52cf31f8b052a8a10ed16a307b0c Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Tue, 25 Jun 2013 09:01:45 +0200 Subject: [PATCH 1/2] Handle too many results from getnetgr.
src/responder/nss/nsssrv_netgroup.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c index 4ec4161cf9c043792fffede1aa95acaf929a7071..12be52bf832cc5315f29b8faac22d4b6c44b3b22 100644 --- a/src/responder/nss/nsssrv_netgroup.c +++ b/src/responder/nss/nsssrv_netgroup.c @@ -363,7 +363,14 @@ static errno_t setnetgrent_retry(struct tevent_req *req) }
ret = lookup_netgr_step(step_ctx);
if (ret != EOK) {
switch (ret) {case EOK:break;case EMSGSIZE:state->netgr->ready = true;
Do we need to set the entry to hash? Can't we just shortcut to tevent_req_error ?
ret = ENOENT;/* FALLTHROUGH */default: goto done; } tevent_req_done(req);
[PATCH 2/2] Do not call sss_cmd_done in function check_cache. Ack I'm glad there is one less FIXME in the code base.
On (25/06/13 15:16), Jakub Hrozek wrote:
On Tue, Jun 25, 2013 at 10:36:14AM +0200, Lukas Slebodnik wrote:
ehlo,
Attached patches should fix https://fedorahosted.org/sssd/ticket/1980
The first patch adds check after sysdb_getnetgr. If sysdb_getnetgr returns more result than 1, sssd will return error. sysdb_getpwnam has already had this check.
The second patch removes function call sss_cmd_done inside of check_cache, because function is sss_cmd_done is called in parent functions. This was a reason of sssd crash.
How to reproduce this crash. 1.Add Netgroup to sysdb cache with base cn=Netgroups,cn=<domain>,cn=sysdb This netgroup should have the same attribute (name or nameAlias or memberOf) as another netgroup. 2. call sudo with user, which is member of ^^^ netgroup.
Those patches fix only sssd crash, but we should find out: Why were those netgroups stored in sysdb.
LS
From 5877db33fc0d52cf31f8b052a8a10ed16a307b0c Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Tue, 25 Jun 2013 09:01:45 +0200 Subject: [PATCH 1/2] Handle too many results from getnetgr.
src/responder/nss/nsssrv_netgroup.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c index 4ec4161cf9c043792fffede1aa95acaf929a7071..12be52bf832cc5315f29b8faac22d4b6c44b3b22 100644 --- a/src/responder/nss/nsssrv_netgroup.c +++ b/src/responder/nss/nsssrv_netgroup.c @@ -363,7 +363,14 @@ static errno_t setnetgrent_retry(struct tevent_req *req) }
ret = lookup_netgr_step(step_ctx);
if (ret != EOK) {
switch (ret) {case EOK:break;case EMSGSIZE:state->netgr->ready = true;Do we need to set the entry to hash? Can't we just shortcut to tevent_req_error ?
Here is pseudocode: ret = get_netgroup_entry // get getgroup from hash if (ret == EOK) { /* snip */ } else if (ret == ENOENT) { // netgr is stored in hash /* snip */ ret = lookup_netgr_step(step_ctx); // my changes }
At first, lookup_netgr_step will return EMSGSIZE and will jump to label done with ret == EMSGSIZE. Second time, get_netgroup_entry will found netgroup in hash, netgroup will have both flags (found, ready) set to false. Flag ready will be false. /* Result object is still being constructed * Register for notification when it's ready */ And function will jump to done label with ret == EOK. There will not be any notification after object construction, because netgroup is created and therefore sss_cmd_done will not be called. SSSD will wain until expiration timeout.
With my patch, Flag ready will be true and tevent_req_error(req, ENOENT) will be called.
ret = ENOENT;/* FALLTHROUGH */default: goto done; } tevent_req_done(req);
LS
On Wed, Jun 26, 2013 at 10:42:01AM +0200, Lukas Slebodnik wrote:
On (25/06/13 15:16), Jakub Hrozek wrote:
On Tue, Jun 25, 2013 at 10:36:14AM +0200, Lukas Slebodnik wrote:
ehlo,
Attached patches should fix https://fedorahosted.org/sssd/ticket/1980
The first patch adds check after sysdb_getnetgr. If sysdb_getnetgr returns more result than 1, sssd will return error. sysdb_getpwnam has already had this check.
The second patch removes function call sss_cmd_done inside of check_cache, because function is sss_cmd_done is called in parent functions. This was a reason of sssd crash.
How to reproduce this crash. 1.Add Netgroup to sysdb cache with base cn=Netgroups,cn=<domain>,cn=sysdb This netgroup should have the same attribute (name or nameAlias or memberOf) as another netgroup. 2. call sudo with user, which is member of ^^^ netgroup.
Those patches fix only sssd crash, but we should find out: Why were those netgroups stored in sysdb.
LS
From 5877db33fc0d52cf31f8b052a8a10ed16a307b0c Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Tue, 25 Jun 2013 09:01:45 +0200 Subject: [PATCH 1/2] Handle too many results from getnetgr.
src/responder/nss/nsssrv_netgroup.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c index 4ec4161cf9c043792fffede1aa95acaf929a7071..12be52bf832cc5315f29b8faac22d4b6c44b3b22 100644 --- a/src/responder/nss/nsssrv_netgroup.c +++ b/src/responder/nss/nsssrv_netgroup.c @@ -363,7 +363,14 @@ static errno_t setnetgrent_retry(struct tevent_req *req) }
ret = lookup_netgr_step(step_ctx);
if (ret != EOK) {
switch (ret) {case EOK:break;case EMSGSIZE:state->netgr->ready = true;Do we need to set the entry to hash? Can't we just shortcut to tevent_req_error ?
Here is pseudocode: ret = get_netgroup_entry // get getgroup from hash if (ret == EOK) { /* snip */ } else if (ret == ENOENT) { // netgr is stored in hash /* snip */ ret = lookup_netgr_step(step_ctx); // my changes }
At first, lookup_netgr_step will return EMSGSIZE and will jump to label done with ret == EMSGSIZE. Second time, get_netgroup_entry will found netgroup in hash, netgroup will have both flags (found, ready) set to false. Flag ready will be false. /* Result object is still being constructed * Register for notification when it's ready */ And function will jump to done label with ret == EOK. There will not be any notification after object construction, because netgroup is created and therefore sss_cmd_done will not be called. SSSD will wain until expiration timeout.
With my patch, Flag ready will be true and tevent_req_error(req, ENOENT) will be called.
Yes, I understood that, I was just thinking if we can just fail without setting the entry in the hash table.
But this patch gets rid of the segfault and is correct. We can think about setting the entry or not later.
Ack.
On Thu, Jun 27, 2013 at 08:00:02PM +0200, Jakub Hrozek wrote:
On Wed, Jun 26, 2013 at 10:42:01AM +0200, Lukas Slebodnik wrote:
On (25/06/13 15:16), Jakub Hrozek wrote:
On Tue, Jun 25, 2013 at 10:36:14AM +0200, Lukas Slebodnik wrote:
ehlo,
Attached patches should fix https://fedorahosted.org/sssd/ticket/1980
The first patch adds check after sysdb_getnetgr. If sysdb_getnetgr returns more result than 1, sssd will return error. sysdb_getpwnam has already had this check.
The second patch removes function call sss_cmd_done inside of check_cache, because function is sss_cmd_done is called in parent functions. This was a reason of sssd crash.
How to reproduce this crash. 1.Add Netgroup to sysdb cache with base cn=Netgroups,cn=<domain>,cn=sysdb This netgroup should have the same attribute (name or nameAlias or memberOf) as another netgroup. 2. call sudo with user, which is member of ^^^ netgroup.
Those patches fix only sssd crash, but we should find out: Why were those netgroups stored in sysdb.
LS
From 5877db33fc0d52cf31f8b052a8a10ed16a307b0c Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Tue, 25 Jun 2013 09:01:45 +0200 Subject: [PATCH 1/2] Handle too many results from getnetgr.
src/responder/nss/nsssrv_netgroup.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c index 4ec4161cf9c043792fffede1aa95acaf929a7071..12be52bf832cc5315f29b8faac22d4b6c44b3b22 100644 --- a/src/responder/nss/nsssrv_netgroup.c +++ b/src/responder/nss/nsssrv_netgroup.c @@ -363,7 +363,14 @@ static errno_t setnetgrent_retry(struct tevent_req *req) }
ret = lookup_netgr_step(step_ctx);
if (ret != EOK) {
switch (ret) {case EOK:break;case EMSGSIZE:state->netgr->ready = true;Do we need to set the entry to hash? Can't we just shortcut to tevent_req_error ?
Here is pseudocode: ret = get_netgroup_entry // get getgroup from hash if (ret == EOK) { /* snip */ } else if (ret == ENOENT) { // netgr is stored in hash /* snip */ ret = lookup_netgr_step(step_ctx); // my changes }
At first, lookup_netgr_step will return EMSGSIZE and will jump to label done with ret == EMSGSIZE. Second time, get_netgroup_entry will found netgroup in hash, netgroup will have both flags (found, ready) set to false. Flag ready will be false. /* Result object is still being constructed * Register for notification when it's ready */ And function will jump to done label with ret == EOK. There will not be any notification after object construction, because netgroup is created and therefore sss_cmd_done will not be called. SSSD will wain until expiration timeout.
With my patch, Flag ready will be true and tevent_req_error(req, ENOENT) will be called.
Yes, I understood that, I was just thinking if we can just fail without setting the entry in the hash table.
But this patch gets rid of the segfault and is correct. We can think about setting the entry or not later.
Ack.
Pushed to master.
On Thu, Jun 27, 2013 at 08:57:36PM +0200, Jakub Hrozek wrote:
On Thu, Jun 27, 2013 at 08:00:02PM +0200, Jakub Hrozek wrote:
On Wed, Jun 26, 2013 at 10:42:01AM +0200, Lukas Slebodnik wrote:
On (25/06/13 15:16), Jakub Hrozek wrote:
On Tue, Jun 25, 2013 at 10:36:14AM +0200, Lukas Slebodnik wrote:
ehlo,
Attached patches should fix https://fedorahosted.org/sssd/ticket/1980
The first patch adds check after sysdb_getnetgr. If sysdb_getnetgr returns more result than 1, sssd will return error. sysdb_getpwnam has already had this check.
The second patch removes function call sss_cmd_done inside of check_cache, because function is sss_cmd_done is called in parent functions. This was a reason of sssd crash.
How to reproduce this crash. 1.Add Netgroup to sysdb cache with base cn=Netgroups,cn=<domain>,cn=sysdb This netgroup should have the same attribute (name or nameAlias or memberOf) as another netgroup. 2. call sudo with user, which is member of ^^^ netgroup.
Those patches fix only sssd crash, but we should find out: Why were those netgroups stored in sysdb.
LS
From 5877db33fc0d52cf31f8b052a8a10ed16a307b0c Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Tue, 25 Jun 2013 09:01:45 +0200 Subject: [PATCH 1/2] Handle too many results from getnetgr.
src/responder/nss/nsssrv_netgroup.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c index 4ec4161cf9c043792fffede1aa95acaf929a7071..12be52bf832cc5315f29b8faac22d4b6c44b3b22 100644 --- a/src/responder/nss/nsssrv_netgroup.c +++ b/src/responder/nss/nsssrv_netgroup.c @@ -363,7 +363,14 @@ static errno_t setnetgrent_retry(struct tevent_req *req) }
ret = lookup_netgr_step(step_ctx);
if (ret != EOK) {
switch (ret) {case EOK:break;case EMSGSIZE:state->netgr->ready = true;Do we need to set the entry to hash? Can't we just shortcut to tevent_req_error ?
Here is pseudocode: ret = get_netgroup_entry // get getgroup from hash if (ret == EOK) { /* snip */ } else if (ret == ENOENT) { // netgr is stored in hash /* snip */ ret = lookup_netgr_step(step_ctx); // my changes }
At first, lookup_netgr_step will return EMSGSIZE and will jump to label done with ret == EMSGSIZE. Second time, get_netgroup_entry will found netgroup in hash, netgroup will have both flags (found, ready) set to false. Flag ready will be false. /* Result object is still being constructed * Register for notification when it's ready */ And function will jump to done label with ret == EOK. There will not be any notification after object construction, because netgroup is created and therefore sss_cmd_done will not be called. SSSD will wain until expiration timeout.
With my patch, Flag ready will be true and tevent_req_error(req, ENOENT) will be called.
Yes, I understood that, I was just thinking if we can just fail without setting the entry in the hash table.
But this patch gets rid of the segfault and is correct. We can think about setting the entry or not later.
Ack.
Pushed to master.
Also pushed to sssd-1-9
On (08/08/13 17:07), Jakub Hrozek wrote:
On Thu, Jun 27, 2013 at 08:57:36PM +0200, Jakub Hrozek wrote:
On Thu, Jun 27, 2013 at 08:00:02PM +0200, Jakub Hrozek wrote:
On Wed, Jun 26, 2013 at 10:42:01AM +0200, Lukas Slebodnik wrote:
On (25/06/13 15:16), Jakub Hrozek wrote:
On Tue, Jun 25, 2013 at 10:36:14AM +0200, Lukas Slebodnik wrote:
ehlo,
Attached patches should fix https://fedorahosted.org/sssd/ticket/1980
The first patch adds check after sysdb_getnetgr. If sysdb_getnetgr returns more result than 1, sssd will return error. sysdb_getpwnam has already had this check.
The second patch removes function call sss_cmd_done inside of check_cache, because function is sss_cmd_done is called in parent functions. This was a reason of sssd crash.
How to reproduce this crash. 1.Add Netgroup to sysdb cache with base cn=Netgroups,cn=<domain>,cn=sysdb This netgroup should have the same attribute (name or nameAlias or memberOf) as another netgroup. 2. call sudo with user, which is member of ^^^ netgroup.
Those patches fix only sssd crash, but we should find out: Why were those netgroups stored in sysdb.
LS
From 5877db33fc0d52cf31f8b052a8a10ed16a307b0c Mon Sep 17 00:00:00 2001 From: Lukas Slebodnik lslebodn@redhat.com Date: Tue, 25 Jun 2013 09:01:45 +0200 Subject: [PATCH 1/2] Handle too many results from getnetgr.
src/responder/nss/nsssrv_netgroup.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c index 4ec4161cf9c043792fffede1aa95acaf929a7071..12be52bf832cc5315f29b8faac22d4b6c44b3b22 100644 --- a/src/responder/nss/nsssrv_netgroup.c +++ b/src/responder/nss/nsssrv_netgroup.c @@ -363,7 +363,14 @@ static errno_t setnetgrent_retry(struct tevent_req *req) }
ret = lookup_netgr_step(step_ctx);
if (ret != EOK) {
switch (ret) {case EOK:break;case EMSGSIZE:state->netgr->ready = true;Do we need to set the entry to hash? Can't we just shortcut to tevent_req_error ?
Here is pseudocode: ret = get_netgroup_entry // get getgroup from hash if (ret == EOK) { /* snip */ } else if (ret == ENOENT) { // netgr is stored in hash /* snip */ ret = lookup_netgr_step(step_ctx); // my changes }
At first, lookup_netgr_step will return EMSGSIZE and will jump to label done with ret == EMSGSIZE. Second time, get_netgroup_entry will found netgroup in hash, netgroup will have both flags (found, ready) set to false. Flag ready will be false. /* Result object is still being constructed * Register for notification when it's ready */ And function will jump to done label with ret == EOK. There will not be any notification after object construction, because netgroup is created and therefore sss_cmd_done will not be called. SSSD will wain until expiration timeout.
With my patch, Flag ready will be true and tevent_req_error(req, ENOENT) will be called.
Yes, I understood that, I was just thinking if we can just fail without setting the entry in the hash table.
But this patch gets rid of the segfault and is correct. We can think about setting the entry or not later.
Ack.
Pushed to master.
Also pushed to sssd-1-9
Only one patch was pushed to sssd-1-9 6e2c5a81b6af083d7909a18881971b5d907d65b1 Do not call sss_cmd_done in function check_cache. This patch fixes crash, but uninitialized netgroup will be stored in hash table. Another query for this netgroup will find uninitialized netgroup in hash table, but this netgroup will never be initialized (because of previous error EMSGSIZE) and therefore sss_cmd_done will not be called.
The second patch should be backported too. f2022f7ba55973ae8b8baf2d4307322a180357b9 Handle too many results from getnetgr. With this patch, uninitialized netgroup will be removed from hash table in case of error EMSGSIZE.
It is my failure, because I was supposed to add text "resolves: https://fedorahosted.org/sssd/ticket/1980" to the second patch.
LS
On Thu, Aug 08, 2013 at 06:32:11PM +0200, Lukas Slebodnik wrote:
On (08/08/13 17:07), Jakub Hrozek wrote:
On Thu, Jun 27, 2013 at 08:57:36PM +0200, Jakub Hrozek wrote:
On Thu, Jun 27, 2013 at 08:00:02PM +0200, Jakub Hrozek wrote:
On Wed, Jun 26, 2013 at 10:42:01AM +0200, Lukas Slebodnik wrote:
On (25/06/13 15:16), Jakub Hrozek wrote:
On Tue, Jun 25, 2013 at 10:36:14AM +0200, Lukas Slebodnik wrote: > ehlo, > > Attached patches should fix https://fedorahosted.org/sssd/ticket/1980 > > The first patch adds check after sysdb_getnetgr. If sysdb_getnetgr returns more > result than 1, sssd will return error. sysdb_getpwnam has already had > this check. > > The second patch removes function call sss_cmd_done inside of check_cache, > because function is sss_cmd_done is called in parent functions. > This was a reason of sssd crash. > > How to reproduce this crash. > 1.Add Netgroup to sysdb cache with base cn=Netgroups,cn=<domain>,cn=sysdb > This netgroup should have the same attribute (name or nameAlias or memberOf) > as another netgroup. > 2. call sudo with user, which is member of ^^^ netgroup. > > Those patches fix only sssd crash, but we should find out: > Why were those netgroups stored in sysdb. > > LS
> From 5877db33fc0d52cf31f8b052a8a10ed16a307b0c Mon Sep 17 00:00:00 2001 > From: Lukas Slebodnik lslebodn@redhat.com > Date: Tue, 25 Jun 2013 09:01:45 +0200 > Subject: [PATCH 1/2] Handle too many results from getnetgr. > > --- > src/responder/nss/nsssrv_netgroup.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/src/responder/nss/nsssrv_netgroup.c b/src/responder/nss/nsssrv_netgroup.c > index 4ec4161cf9c043792fffede1aa95acaf929a7071..12be52bf832cc5315f29b8faac22d4b6c44b3b22 100644 > --- a/src/responder/nss/nsssrv_netgroup.c > +++ b/src/responder/nss/nsssrv_netgroup.c > @@ -363,7 +363,14 @@ static errno_t setnetgrent_retry(struct tevent_req *req) > } > > ret = lookup_netgr_step(step_ctx); > - if (ret != EOK) { > + switch (ret) { > + case EOK: > + break; > + case EMSGSIZE: > + state->netgr->ready = true;
Do we need to set the entry to hash? Can't we just shortcut to tevent_req_error ?
Here is pseudocode: ret = get_netgroup_entry // get getgroup from hash if (ret == EOK) { /* snip */ } else if (ret == ENOENT) { // netgr is stored in hash /* snip */ ret = lookup_netgr_step(step_ctx); // my changes }
At first, lookup_netgr_step will return EMSGSIZE and will jump to label done with ret == EMSGSIZE. Second time, get_netgroup_entry will found netgroup in hash, netgroup will have both flags (found, ready) set to false. Flag ready will be false. /* Result object is still being constructed * Register for notification when it's ready */ And function will jump to done label with ret == EOK. There will not be any notification after object construction, because netgroup is created and therefore sss_cmd_done will not be called. SSSD will wain until expiration timeout.
With my patch, Flag ready will be true and tevent_req_error(req, ENOENT) will be called.
Yes, I understood that, I was just thinking if we can just fail without setting the entry in the hash table.
But this patch gets rid of the segfault and is correct. We can think about setting the entry or not later.
Ack.
Pushed to master.
Also pushed to sssd-1-9
Only one patch was pushed to sssd-1-9 6e2c5a81b6af083d7909a18881971b5d907d65b1 Do not call sss_cmd_done in function check_cache. This patch fixes crash, but uninitialized netgroup will be stored in hash table. Another query for this netgroup will find uninitialized netgroup in hash table, but this netgroup will never be initialized (because of previous error EMSGSIZE) and therefore sss_cmd_done will not be called.
The second patch should be backported too. f2022f7ba55973ae8b8baf2d4307322a180357b9 Handle too many results from getnetgr. With this patch, uninitialized netgroup will be removed from hash table in case of error EMSGSIZE.
It is my failure, because I was supposed to add text "resolves: https://fedorahosted.org/sssd/ticket/1980" to the second patch.
LS
Well, I should also check the full threads before just applying a patch..
f2022f7ba55973ae8b8baf2d4307322a180357b9 applies cleanly, I'll push it to sssd-1-9
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik