This bug was probably introduced with the subdomain patches. The problem was that sss_dp_get_domains_send() is called even for the local provider. There are certainly many possible solutions of this issue. I decided to modify sss_dp_issue_request() to call the callback immediately if it is issued for local domain. This way, I believe, we can avoid further problems with issuing request for local domains.
On Wed, Aug 08, 2012 at 02:23:04PM +0200, Pavel Březina wrote:
This bug was probably introduced with the subdomain patches. The problem was that sss_dp_get_domains_send() is called even for the local provider. There are certainly many possible solutions of this issue. I decided to modify sss_dp_issue_request() to call the callback immediately if it is issued for local domain. This way, I believe, we can avoid further problems with issuing request for local domains.
I don't think this is the best solution.
First, it leaves the check_provider of the per-domain structures we use in responders around while duplicating the same functionality in the DP. At the very least, the patch should remove the "check_provider" tests from the existing loops.
What I think would be even better solution is to 1) in the RC just fix the subdomains code so that it shortcuts or doesn't run at all for LOCAL lookups. 2) The work we'll be doing later on #1126 would merge all the loops into one common place.
This approach would have the benefit of all the checks being done at one common place, so that we don't forget to add them like we did in the subdomains case and at would be faster at the same time because it wouldn't even contact the DP at all, much like it's done now in the separate loops.
On Wed, Aug 08, 2012 at 04:24:47PM +0200, Jakub Hrozek wrote:
2) The work we'll be doing later on #1126 would merge all the loops into one common place.
There is a WIP code for the merge in the "refactor" branch in my fedorapeople.org repository. I started working on it earlier and then got sidetracked.
On Wed, 2012-08-08 at 16:24 +0200, Jakub Hrozek wrote:
On Wed, Aug 08, 2012 at 02:23:04PM +0200, Pavel Březina wrote:
This bug was probably introduced with the subdomain patches. The problem was that sss_dp_get_domains_send() is called even for the local provider. There are certainly many possible solutions of this issue. I decided to modify sss_dp_issue_request() to call the callback immediately if it is issued for local domain. This way, I believe, we can avoid further problems with issuing request for local domains.
I don't think this is the best solution.
First, it leaves the check_provider of the per-domain structures we use in responders around while duplicating the same functionality in the DP. At the very least, the patch should remove the "check_provider" tests from the existing loops.
What I think would be even better solution is to 1) in the RC just fix the subdomains code so that it shortcuts or doesn't run at all for LOCAL lookups. 2) The work we'll be doing later on #1126 would merge all the loops into one common place.
This approach would have the benefit of all the checks being done at one common place, so that we don't forget to add them like we did in the subdomains case and at would be faster at the same time because it wouldn't even contact the DP at all, much like it's done now in the separate loops.
I think it might be time to more seriously consider decoupling the LOCAL provider into a real provider type, instead of having so many special-cases throughout the code (i.e. if !local then goto data provider)
On 08/08/2012 04:36 PM, Stephen Gallagher wrote:
On Wed, 2012-08-08 at 16:24 +0200, Jakub Hrozek wrote:
On Wed, Aug 08, 2012 at 02:23:04PM +0200, Pavel Březina wrote:
This bug was probably introduced with the subdomain patches. The problem was that sss_dp_get_domains_send() is called even for the local provider. There are certainly many possible solutions of this issue. I decided to modify sss_dp_issue_request() to call the callback immediately if it is issued for local domain. This way, I believe, we can avoid further problems with issuing request for local domains.
I don't think this is the best solution.
First, it leaves the check_provider of the per-domain structures we use in responders around while duplicating the same functionality in the DP. At the very least, the patch should remove the "check_provider" tests from the existing loops.
What I think would be even better solution is to 1) in the RC just fix the subdomains code so that it shortcuts or doesn't run at all for LOCAL lookups. 2) The work we'll be doing later on #1126 would merge all the loops into one common place.
This approach would have the benefit of all the checks being done at one common place, so that we don't forget to add them like we did in the subdomains case and at would be faster at the same time because it wouldn't even contact the DP at all, much like it's done now in the separate loops.
I think it might be time to more seriously consider decoupling the LOCAL provider into a real provider type, instead of having so many special-cases throughout the code (i.e. if !local then goto data provider)
This patch removes the needs of having special case all around the code. It just isn't in the scope of this ticket to actually remove the existing special cases. But if you want to do it in #1126, I'll just fix the subdomains.
On Thu, Aug 09, 2012 at 09:40:36AM +0200, Pavel Březina wrote:
On 08/08/2012 04:36 PM, Stephen Gallagher wrote:
On Wed, 2012-08-08 at 16:24 +0200, Jakub Hrozek wrote:
On Wed, Aug 08, 2012 at 02:23:04PM +0200, Pavel Březina wrote:
This bug was probably introduced with the subdomain patches. The problem was that sss_dp_get_domains_send() is called even for the local provider. There are certainly many possible solutions of this issue. I decided to modify sss_dp_issue_request() to call the callback immediately if it is issued for local domain. This way, I believe, we can avoid further problems with issuing request for local domains.
I don't think this is the best solution.
First, it leaves the check_provider of the per-domain structures we use in responders around while duplicating the same functionality in the DP. At the very least, the patch should remove the "check_provider" tests from the existing loops.
What I think would be even better solution is to 1) in the RC just fix the subdomains code so that it shortcuts or doesn't run at all for LOCAL lookups. 2) The work we'll be doing later on #1126 would merge all the loops into one common place.
This approach would have the benefit of all the checks being done at one common place, so that we don't forget to add them like we did in the subdomains case and at would be faster at the same time because it wouldn't even contact the DP at all, much like it's done now in the separate loops.
I think it might be time to more seriously consider decoupling the LOCAL provider into a real provider type, instead of having so many special-cases throughout the code (i.e. if !local then goto data provider)
This patch removes the needs of having special case all around the code. It just isn't in the scope of this ticket to actually remove the existing special cases. But if you want to do it in #1126, I'll just fix the subdomains.
Sure, we're not doing the local provider now. We should just file a ticket: https://fedorahosted.org/sssd/ticket/1469
But even with the current patch, you still need to remove the "check_provider" checks all around the code. They duplicate the same logic as your patch.
On 08/08/2012 04:24 PM, Jakub Hrozek wrote:
On Wed, Aug 08, 2012 at 02:23:04PM +0200, Pavel Březina wrote:
This bug was probably introduced with the subdomain patches. The problem was that sss_dp_get_domains_send() is called even for the local provider. There are certainly many possible solutions of this issue. I decided to modify sss_dp_issue_request() to call the callback immediately if it is issued for local domain. This way, I believe, we can avoid further problems with issuing request for local domains.
I don't think this is the best solution.
First, it leaves the check_provider of the per-domain structures we use in responders around while duplicating the same functionality in the DP. At the very least, the patch should remove the "check_provider" tests from the existing loops.
What I think would be even better solution is to 1) in the RC just fix the subdomains code so that it shortcuts or doesn't run at all for LOCAL lookups.
Patch is attached.
2) The work we'll be doing later on #1126 would merge all the loops into one common place.This approach would have the benefit of all the checks being done at one common place, so that we don't forget to add them like we did in the subdomains case and at would be faster at the same time because it wouldn't even contact the DP at all, much like it's done now in the separate loops.
On Fri, Aug 10, 2012 at 09:26:49AM +0200, Pavel Březina wrote:
On 08/08/2012 04:24 PM, Jakub Hrozek wrote:
On Wed, Aug 08, 2012 at 02:23:04PM +0200, Pavel Březina wrote:
This bug was probably introduced with the subdomain patches. The problem was that sss_dp_get_domains_send() is called even for the local provider. There are certainly many possible solutions of this issue. I decided to modify sss_dp_issue_request() to call the callback immediately if it is issued for local domain. This way, I believe, we can avoid further problems with issuing request for local domains.
I don't think this is the best solution.
First, it leaves the check_provider of the per-domain structures we use in responders around while duplicating the same functionality in the DP. At the very least, the patch should remove the "check_provider" tests from the existing loops.
What I think would be even better solution is to 1) in the RC just fix the subdomains code so that it shortcuts or doesn't run at all for LOCAL lookups.
Patch is attached.
Thank you, this works for me.
I'm seeing this warning when LOCAL users are enumerated (as opposed to a direct lookup): (Mon Aug 13 11:06:58 2012) [sssd[nss]] [setent_notify] (0x0010): BUG: a callback did not free its request. May leak memory
Given that the LOCAL domain is mostly useful for testing right now, I've just filed a ticket: https://fedorahosted.org/sssd/ticket/1477
Ack
On Mon, Aug 13, 2012 at 11:10:36AM +0200, Jakub Hrozek wrote:
On Fri, Aug 10, 2012 at 09:26:49AM +0200, Pavel Březina wrote:
On 08/08/2012 04:24 PM, Jakub Hrozek wrote:
On Wed, Aug 08, 2012 at 02:23:04PM +0200, Pavel Březina wrote:
This bug was probably introduced with the subdomain patches. The problem was that sss_dp_get_domains_send() is called even for the local provider. There are certainly many possible solutions of this issue. I decided to modify sss_dp_issue_request() to call the callback immediately if it is issued for local domain. This way, I believe, we can avoid further problems with issuing request for local domains.
I don't think this is the best solution.
First, it leaves the check_provider of the per-domain structures we use in responders around while duplicating the same functionality in the DP. At the very least, the patch should remove the "check_provider" tests from the existing loops.
What I think would be even better solution is to 1) in the RC just fix the subdomains code so that it shortcuts or doesn't run at all for LOCAL lookups.
Patch is attached.
Thank you, this works for me.
I'm seeing this warning when LOCAL users are enumerated (as opposed to a direct lookup): (Mon Aug 13 11:06:58 2012) [sssd[nss]] [setent_notify] (0x0010): BUG: a callback did not free its request. May leak memory
Given that the LOCAL domain is mostly useful for testing right now, I've just filed a ticket: https://fedorahosted.org/sssd/ticket/1477
Ack
Nack, upon further testing. I'm seeing segfaults when looking up non-LOCAL users:
Core was generated by `/usr/libexec/sssd/sssd_nss -d 0x17f0'. Program terminated with signal 11, Segmentation fault. #0 0x000000000043acb1 in get_domains_next (req=0xf95ca0) at src/responder/common/responder_get_domains.c:124 124 tevent_req_post(req, info->rctx->ev); Missing separate debuginfos, use: debuginfo-install nss-softokn-freebl-3.13.5-1.fc17.x86_64 openldap-2.4.31-3.fc17.x86_64 (gdb) bt #0 0x000000000043acb1 in get_domains_next (req=0xf95ca0) at src/responder/common/responder_get_domains.c:124 #1 0x000000000043b461 in sss_dp_get_domains_callback (subreq=0xf968b0) at src/responder/common/responder_get_domains.c:215 #2 0x0000000000439dd4 in sss_dp_internal_get_done (pending=0xf944e0, ptr=0xf99220) at src/responder/common/responder_dp.c:731 #3 0x000000342fa0c42a in complete_pending_call_and_unlock (connection=connection@entry=0xf95e40, pending=0xf944e0, message=message@entry=0xf98a80) at dbus-connection.c:2308 #4 0x000000342fa0f5ea in dbus_connection_dispatch (connection=0xf95e40) at dbus-connection.c:4593 #5 0x0000000000460432 in sbus_dispatch (ev=0xf89390, te=0xf98bc0, tv=..., data=0xf979a0) at src/sbus/sssd_dbus_connection.c:104 #6 0x000000385b407600 in tevent_common_loop_timer_delay (ev=ev@entry=0xf89390) at ../tevent_timed.c:254 #7 0x000000385b406cac in std_event_loop_once (ev=0xf89390, location=<optimized out>) at ../tevent_standard.c:560 #8 0x000000385b403ed0 in _tevent_loop_once (ev=ev@entry=0xf89390, location=location@entry=0x492f97 "src/util/server.c:554") at ../tevent.c:506 #9 0x000000385b40405b in tevent_common_loop_wait (ev=0xf89390, location=0x492f97 "src/util/server.c:554") at ../tevent.c:607 #10 0x0000000000467bf4 in server_loop (main_ctx=0xf8a4a0) at src/util/server.c:554 #11 0x0000000000408d5c in main (argc=3, argv=0x7fff2dde9cc8) at src/responder/nss/nsssrv.c:422 (gdb) p *info $1 = {rctx = 0x3133623833347830, dom = 0x306d61736c3a313a, hint = 0x696e403334346336 <Address 0x696e403334346336 out of bounds>, force = 114, state = 0x0}
It looks like "info" was not initialized.
On 08/13/2012 11:42 AM, Jakub Hrozek wrote:
On Mon, Aug 13, 2012 at 11:10:36AM +0200, Jakub Hrozek wrote:
On Fri, Aug 10, 2012 at 09:26:49AM +0200, Pavel Březina wrote:
On 08/08/2012 04:24 PM, Jakub Hrozek wrote:
On Wed, Aug 08, 2012 at 02:23:04PM +0200, Pavel Březina wrote:
This bug was probably introduced with the subdomain patches. The problem was that sss_dp_get_domains_send() is called even for the local provider. There are certainly many possible solutions of this issue. I decided to modify sss_dp_issue_request() to call the callback immediately if it is issued for local domain. This way, I believe, we can avoid further problems with issuing request for local domains.
I don't think this is the best solution.
First, it leaves the check_provider of the per-domain structures we use in responders around while duplicating the same functionality in the DP. At the very least, the patch should remove the "check_provider" tests from the existing loops.
What I think would be even better solution is to 1) in the RC just fix the subdomains code so that it shortcuts or doesn't run at all for LOCAL lookups.
Patch is attached.
Thank you, this works for me.
I'm seeing this warning when LOCAL users are enumerated (as opposed to a direct lookup): (Mon Aug 13 11:06:58 2012) [sssd[nss]] [setent_notify] (0x0010): BUG: a callback did not free its request. May leak memory
Given that the LOCAL domain is mostly useful for testing right now, I've just filed a ticket: https://fedorahosted.org/sssd/ticket/1477
Ack
Nack, upon further testing. I'm seeing segfaults when looking up non-LOCAL users:
Core was generated by `/usr/libexec/sssd/sssd_nss -d 0x17f0'. Program terminated with signal 11, Segmentation fault. #0 0x000000000043acb1 in get_domains_next (req=0xf95ca0) at src/responder/common/responder_get_domains.c:124 124 tevent_req_post(req, info->rctx->ev); Missing separate debuginfos, use: debuginfo-install nss-softokn-freebl-3.13.5-1.fc17.x86_64 openldap-2.4.31-3.fc17.x86_64 (gdb) bt #0 0x000000000043acb1 in get_domains_next (req=0xf95ca0) at src/responder/common/responder_get_domains.c:124 #1 0x000000000043b461 in sss_dp_get_domains_callback (subreq=0xf968b0) at src/responder/common/responder_get_domains.c:215 #2 0x0000000000439dd4 in sss_dp_internal_get_done (pending=0xf944e0, ptr=0xf99220) at src/responder/common/responder_dp.c:731 #3 0x000000342fa0c42a in complete_pending_call_and_unlock (connection=connection@entry=0xf95e40, pending=0xf944e0, message=message@entry=0xf98a80) at dbus-connection.c:2308 #4 0x000000342fa0f5ea in dbus_connection_dispatch (connection=0xf95e40) at dbus-connection.c:4593 #5 0x0000000000460432 in sbus_dispatch (ev=0xf89390, te=0xf98bc0, tv=..., data=0xf979a0) at src/sbus/sssd_dbus_connection.c:104 #6 0x000000385b407600 in tevent_common_loop_timer_delay (ev=ev@entry=0xf89390) at ../tevent_timed.c:254 #7 0x000000385b406cac in std_event_loop_once (ev=0xf89390, location=<optimized out>) at ../tevent_standard.c:560 #8 0x000000385b403ed0 in _tevent_loop_once (ev=ev@entry=0xf89390, location=location@entry=0x492f97 "src/util/server.c:554") at ../tevent.c:506 #9 0x000000385b40405b in tevent_common_loop_wait (ev=0xf89390, location=0x492f97 "src/util/server.c:554") at ../tevent.c:607 #10 0x0000000000467bf4 in server_loop (main_ctx=0xf8a4a0) at src/util/server.c:554 #11 0x0000000000408d5c in main (argc=3, argv=0x7fff2dde9cc8) at src/responder/nss/nsssrv.c:422 (gdb) p *info $1 = {rctx = 0x3133623833347830, dom = 0x306d61736c3a313a, hint = 0x696e403334346336 <Address 0x696e403334346336 out of bounds>, force = 114, state = 0x0}
It looks like "info" was not initialized.
Thanks.
The problem was that callback is invoked immediately when tevent_req_done() is called. And because req is freed in this callback, info was pointing to invalid data.
New patch is attached.
On Tue, Aug 14, 2012 at 01:38:15PM +0200, Pavel Březina wrote:
On 08/13/2012 11:42 AM, Jakub Hrozek wrote:
On Mon, Aug 13, 2012 at 11:10:36AM +0200, Jakub Hrozek wrote:
On Fri, Aug 10, 2012 at 09:26:49AM +0200, Pavel Březina wrote:
On 08/08/2012 04:24 PM, Jakub Hrozek wrote:
On Wed, Aug 08, 2012 at 02:23:04PM +0200, Pavel Březina wrote:
This bug was probably introduced with the subdomain patches. The problem was that sss_dp_get_domains_send() is called even for the local provider. There are certainly many possible solutions of this issue. I decided to modify sss_dp_issue_request() to call the callback immediately if it is issued for local domain. This way, I believe, we can avoid further problems with issuing request for local domains.
I don't think this is the best solution.
First, it leaves the check_provider of the per-domain structures we use in responders around while duplicating the same functionality in the DP. At the very least, the patch should remove the "check_provider" tests from the existing loops.
What I think would be even better solution is to 1) in the RC just fix the subdomains code so that it shortcuts or doesn't run at all for LOCAL lookups.
Patch is attached.
Thank you, this works for me.
I'm seeing this warning when LOCAL users are enumerated (as opposed to a direct lookup): (Mon Aug 13 11:06:58 2012) [sssd[nss]] [setent_notify] (0x0010): BUG: a callback did not free its request. May leak memory
Given that the LOCAL domain is mostly useful for testing right now, I've just filed a ticket: https://fedorahosted.org/sssd/ticket/1477
Ack
Nack, upon further testing. I'm seeing segfaults when looking up non-LOCAL users:
Core was generated by `/usr/libexec/sssd/sssd_nss -d 0x17f0'. Program terminated with signal 11, Segmentation fault. #0 0x000000000043acb1 in get_domains_next (req=0xf95ca0) at src/responder/common/responder_get_domains.c:124 124 tevent_req_post(req, info->rctx->ev); Missing separate debuginfos, use: debuginfo-install nss-softokn-freebl-3.13.5-1.fc17.x86_64 openldap-2.4.31-3.fc17.x86_64 (gdb) bt #0 0x000000000043acb1 in get_domains_next (req=0xf95ca0) at src/responder/common/responder_get_domains.c:124 #1 0x000000000043b461 in sss_dp_get_domains_callback (subreq=0xf968b0) at src/responder/common/responder_get_domains.c:215 #2 0x0000000000439dd4 in sss_dp_internal_get_done (pending=0xf944e0, ptr=0xf99220) at src/responder/common/responder_dp.c:731 #3 0x000000342fa0c42a in complete_pending_call_and_unlock (connection=connection@entry=0xf95e40, pending=0xf944e0, message=message@entry=0xf98a80) at dbus-connection.c:2308 #4 0x000000342fa0f5ea in dbus_connection_dispatch (connection=0xf95e40) at dbus-connection.c:4593 #5 0x0000000000460432 in sbus_dispatch (ev=0xf89390, te=0xf98bc0, tv=..., data=0xf979a0) at src/sbus/sssd_dbus_connection.c:104 #6 0x000000385b407600 in tevent_common_loop_timer_delay (ev=ev@entry=0xf89390) at ../tevent_timed.c:254 #7 0x000000385b406cac in std_event_loop_once (ev=0xf89390, location=<optimized out>) at ../tevent_standard.c:560 #8 0x000000385b403ed0 in _tevent_loop_once (ev=ev@entry=0xf89390, location=location@entry=0x492f97 "src/util/server.c:554") at ../tevent.c:506 #9 0x000000385b40405b in tevent_common_loop_wait (ev=0xf89390, location=0x492f97 "src/util/server.c:554") at ../tevent.c:607 #10 0x0000000000467bf4 in server_loop (main_ctx=0xf8a4a0) at src/util/server.c:554 #11 0x0000000000408d5c in main (argc=3, argv=0x7fff2dde9cc8) at src/responder/nss/nsssrv.c:422 (gdb) p *info $1 = {rctx = 0x3133623833347830, dom = 0x306d61736c3a313a, hint = 0x696e403334346336 <Address 0x696e403334346336 out of bounds>, force = 114, state = 0x0}
It looks like "info" was not initialized.
Thanks.
The problem was that callback is invoked immediately when tevent_req_done() is called. And because req is freed in this callback, info was pointing to invalid data.
New patch is attached.
I think it would be much cleaner to remove the tevent_req_post completely from get_domains_next() and only call it in sss_dp_get_domains_send() if get_domains_next() returned EOK.
I wouldn't expect tevent_req_post to be called in any other function that *_send. Also checking the SSSD code shows that we only call tevent_req_post from *_send elsewhere.
On 08/15/2012 03:56 PM, Jakub Hrozek wrote:
On Tue, Aug 14, 2012 at 01:38:15PM +0200, Pavel Březina wrote:
On 08/13/2012 11:42 AM, Jakub Hrozek wrote:
On Mon, Aug 13, 2012 at 11:10:36AM +0200, Jakub Hrozek wrote:
On Fri, Aug 10, 2012 at 09:26:49AM +0200, Pavel Březina wrote:
On 08/08/2012 04:24 PM, Jakub Hrozek wrote:
On Wed, Aug 08, 2012 at 02:23:04PM +0200, Pavel Březina wrote: > This bug was probably introduced with the subdomain patches. The > problem was that sss_dp_get_domains_send() is called even for the local > provider. There are certainly many possible solutions of this issue. I > decided to modify sss_dp_issue_request() to call the callback > immediately if it is issued for local domain. This way, I believe, we > can avoid further problems with issuing request for local domains.
I don't think this is the best solution.
First, it leaves the check_provider of the per-domain structures we use in responders around while duplicating the same functionality in the DP. At the very least, the patch should remove the "check_provider" tests from the existing loops.
What I think would be even better solution is to 1) in the RC just fix the subdomains code so that it shortcuts or doesn't run at all for LOCAL lookups.
Patch is attached.
Thank you, this works for me.
I'm seeing this warning when LOCAL users are enumerated (as opposed to a direct lookup): (Mon Aug 13 11:06:58 2012) [sssd[nss]] [setent_notify] (0x0010): BUG: a callback did not free its request. May leak memory
Given that the LOCAL domain is mostly useful for testing right now, I've just filed a ticket: https://fedorahosted.org/sssd/ticket/1477
Ack
Nack, upon further testing. I'm seeing segfaults when looking up non-LOCAL users:
Core was generated by `/usr/libexec/sssd/sssd_nss -d 0x17f0'. Program terminated with signal 11, Segmentation fault. #0 0x000000000043acb1 in get_domains_next (req=0xf95ca0) at src/responder/common/responder_get_domains.c:124 124 tevent_req_post(req, info->rctx->ev); Missing separate debuginfos, use: debuginfo-install nss-softokn-freebl-3.13.5-1.fc17.x86_64 openldap-2.4.31-3.fc17.x86_64 (gdb) bt #0 0x000000000043acb1 in get_domains_next (req=0xf95ca0) at src/responder/common/responder_get_domains.c:124 #1 0x000000000043b461 in sss_dp_get_domains_callback (subreq=0xf968b0) at src/responder/common/responder_get_domains.c:215 #2 0x0000000000439dd4 in sss_dp_internal_get_done (pending=0xf944e0, ptr=0xf99220) at src/responder/common/responder_dp.c:731 #3 0x000000342fa0c42a in complete_pending_call_and_unlock (connection=connection@entry=0xf95e40, pending=0xf944e0, message=message@entry=0xf98a80) at dbus-connection.c:2308 #4 0x000000342fa0f5ea in dbus_connection_dispatch (connection=0xf95e40) at dbus-connection.c:4593 #5 0x0000000000460432 in sbus_dispatch (ev=0xf89390, te=0xf98bc0, tv=..., data=0xf979a0) at src/sbus/sssd_dbus_connection.c:104 #6 0x000000385b407600 in tevent_common_loop_timer_delay (ev=ev@entry=0xf89390) at ../tevent_timed.c:254 #7 0x000000385b406cac in std_event_loop_once (ev=0xf89390, location=<optimized out>) at ../tevent_standard.c:560 #8 0x000000385b403ed0 in _tevent_loop_once (ev=ev@entry=0xf89390, location=location@entry=0x492f97 "src/util/server.c:554") at ../tevent.c:506 #9 0x000000385b40405b in tevent_common_loop_wait (ev=0xf89390, location=0x492f97 "src/util/server.c:554") at ../tevent.c:607 #10 0x0000000000467bf4 in server_loop (main_ctx=0xf8a4a0) at src/util/server.c:554 #11 0x0000000000408d5c in main (argc=3, argv=0x7fff2dde9cc8) at src/responder/nss/nsssrv.c:422 (gdb) p *info $1 = {rctx = 0x3133623833347830, dom = 0x306d61736c3a313a, hint = 0x696e403334346336 <Address 0x696e403334346336 out of bounds>, force = 114, state = 0x0}
It looks like "info" was not initialized.
Thanks.
The problem was that callback is invoked immediately when tevent_req_done() is called. And because req is freed in this callback, info was pointing to invalid data.
New patch is attached.
I think it would be much cleaner to remove the tevent_req_post completely from get_domains_next() and only call it in sss_dp_get_domains_send() if get_domains_next() returned EOK.
I wouldn't expect tevent_req_post to be called in any other function that *_send. Also checking the SSSD code shows that we only call tevent_req_post from *_send elsewhere.
As you wish. Patch is attached.
On Wed, Aug 15, 2012 at 04:17:21PM +0200, Pavel Březina wrote:
On 08/15/2012 03:56 PM, Jakub Hrozek wrote:
On Tue, Aug 14, 2012 at 01:38:15PM +0200, Pavel Březina wrote:
On 08/13/2012 11:42 AM, Jakub Hrozek wrote:
On Mon, Aug 13, 2012 at 11:10:36AM +0200, Jakub Hrozek wrote:
On Fri, Aug 10, 2012 at 09:26:49AM +0200, Pavel Březina wrote:
On 08/08/2012 04:24 PM, Jakub Hrozek wrote: >On Wed, Aug 08, 2012 at 02:23:04PM +0200, Pavel Březina wrote: >>This bug was probably introduced with the subdomain patches. The >>problem was that sss_dp_get_domains_send() is called even for the local >>provider. There are certainly many possible solutions of this issue. I >>decided to modify sss_dp_issue_request() to call the callback >>immediately if it is issued for local domain. This way, I believe, we >>can avoid further problems with issuing request for local domains. > >I don't think this is the best solution. > >First, it leaves the check_provider of the per-domain structures we use >in responders around while duplicating the same functionality in the DP. >At the very least, the patch should remove the "check_provider" tests from >the existing loops. > >What I think would be even better solution is to > 1) in the RC just fix the subdomains code so that it shortcuts or > doesn't run at all for LOCAL lookups.
Patch is attached.
Thank you, this works for me.
I'm seeing this warning when LOCAL users are enumerated (as opposed to a direct lookup): (Mon Aug 13 11:06:58 2012) [sssd[nss]] [setent_notify] (0x0010): BUG: a callback did not free its request. May leak memory
Given that the LOCAL domain is mostly useful for testing right now, I've just filed a ticket: https://fedorahosted.org/sssd/ticket/1477
Ack
Nack, upon further testing. I'm seeing segfaults when looking up non-LOCAL users:
Core was generated by `/usr/libexec/sssd/sssd_nss -d 0x17f0'. Program terminated with signal 11, Segmentation fault. #0 0x000000000043acb1 in get_domains_next (req=0xf95ca0) at src/responder/common/responder_get_domains.c:124 124 tevent_req_post(req, info->rctx->ev); Missing separate debuginfos, use: debuginfo-install nss-softokn-freebl-3.13.5-1.fc17.x86_64 openldap-2.4.31-3.fc17.x86_64 (gdb) bt #0 0x000000000043acb1 in get_domains_next (req=0xf95ca0) at src/responder/common/responder_get_domains.c:124 #1 0x000000000043b461 in sss_dp_get_domains_callback (subreq=0xf968b0) at src/responder/common/responder_get_domains.c:215 #2 0x0000000000439dd4 in sss_dp_internal_get_done (pending=0xf944e0, ptr=0xf99220) at src/responder/common/responder_dp.c:731 #3 0x000000342fa0c42a in complete_pending_call_and_unlock (connection=connection@entry=0xf95e40, pending=0xf944e0, message=message@entry=0xf98a80) at dbus-connection.c:2308 #4 0x000000342fa0f5ea in dbus_connection_dispatch (connection=0xf95e40) at dbus-connection.c:4593 #5 0x0000000000460432 in sbus_dispatch (ev=0xf89390, te=0xf98bc0, tv=..., data=0xf979a0) at src/sbus/sssd_dbus_connection.c:104 #6 0x000000385b407600 in tevent_common_loop_timer_delay (ev=ev@entry=0xf89390) at ../tevent_timed.c:254 #7 0x000000385b406cac in std_event_loop_once (ev=0xf89390, location=<optimized out>) at ../tevent_standard.c:560 #8 0x000000385b403ed0 in _tevent_loop_once (ev=ev@entry=0xf89390, location=location@entry=0x492f97 "src/util/server.c:554") at ../tevent.c:506 #9 0x000000385b40405b in tevent_common_loop_wait (ev=0xf89390, location=0x492f97 "src/util/server.c:554") at ../tevent.c:607 #10 0x0000000000467bf4 in server_loop (main_ctx=0xf8a4a0) at src/util/server.c:554 #11 0x0000000000408d5c in main (argc=3, argv=0x7fff2dde9cc8) at src/responder/nss/nsssrv.c:422 (gdb) p *info $1 = {rctx = 0x3133623833347830, dom = 0x306d61736c3a313a, hint = 0x696e403334346336 <Address 0x696e403334346336 out of bounds>, force = 114, state = 0x0}
It looks like "info" was not initialized.
Thanks.
The problem was that callback is invoked immediately when tevent_req_done() is called. And because req is freed in this callback, info was pointing to invalid data.
New patch is attached.
I think it would be much cleaner to remove the tevent_req_post completely from get_domains_next() and only call it in sss_dp_get_domains_send() if get_domains_next() returned EOK.
I wouldn't expect tevent_req_post to be called in any other function that *_send. Also checking the SSSD code shows that we only call tevent_req_post from *_send elsewhere.
As you wish. Patch is attached.
Ack
On Wed, Aug 15, 2012 at 05:04:23PM +0200, Jakub Hrozek wrote:
On Wed, Aug 15, 2012 at 04:17:21PM +0200, Pavel Březina wrote:
On 08/15/2012 03:56 PM, Jakub Hrozek wrote:
On Tue, Aug 14, 2012 at 01:38:15PM +0200, Pavel Březina wrote:
On 08/13/2012 11:42 AM, Jakub Hrozek wrote:
On Mon, Aug 13, 2012 at 11:10:36AM +0200, Jakub Hrozek wrote:
On Fri, Aug 10, 2012 at 09:26:49AM +0200, Pavel Březina wrote: >On 08/08/2012 04:24 PM, Jakub Hrozek wrote: >>On Wed, Aug 08, 2012 at 02:23:04PM +0200, Pavel Březina wrote: >>>This bug was probably introduced with the subdomain patches. The >>>problem was that sss_dp_get_domains_send() is called even for the local >>>provider. There are certainly many possible solutions of this issue. I >>>decided to modify sss_dp_issue_request() to call the callback >>>immediately if it is issued for local domain. This way, I believe, we >>>can avoid further problems with issuing request for local domains. >> >>I don't think this is the best solution. >> >>First, it leaves the check_provider of the per-domain structures we use >>in responders around while duplicating the same functionality in the DP. >>At the very least, the patch should remove the "check_provider" tests from >>the existing loops. >> >>What I think would be even better solution is to >> 1) in the RC just fix the subdomains code so that it shortcuts or >> doesn't run at all for LOCAL lookups. > >Patch is attached.
Thank you, this works for me.
I'm seeing this warning when LOCAL users are enumerated (as opposed to a direct lookup): (Mon Aug 13 11:06:58 2012) [sssd[nss]] [setent_notify] (0x0010): BUG: a callback did not free its request. May leak memory
Given that the LOCAL domain is mostly useful for testing right now, I've just filed a ticket: https://fedorahosted.org/sssd/ticket/1477
Ack
Nack, upon further testing. I'm seeing segfaults when looking up non-LOCAL users:
Core was generated by `/usr/libexec/sssd/sssd_nss -d 0x17f0'. Program terminated with signal 11, Segmentation fault. #0 0x000000000043acb1 in get_domains_next (req=0xf95ca0) at src/responder/common/responder_get_domains.c:124 124 tevent_req_post(req, info->rctx->ev); Missing separate debuginfos, use: debuginfo-install nss-softokn-freebl-3.13.5-1.fc17.x86_64 openldap-2.4.31-3.fc17.x86_64 (gdb) bt #0 0x000000000043acb1 in get_domains_next (req=0xf95ca0) at src/responder/common/responder_get_domains.c:124 #1 0x000000000043b461 in sss_dp_get_domains_callback (subreq=0xf968b0) at src/responder/common/responder_get_domains.c:215 #2 0x0000000000439dd4 in sss_dp_internal_get_done (pending=0xf944e0, ptr=0xf99220) at src/responder/common/responder_dp.c:731 #3 0x000000342fa0c42a in complete_pending_call_and_unlock (connection=connection@entry=0xf95e40, pending=0xf944e0, message=message@entry=0xf98a80) at dbus-connection.c:2308 #4 0x000000342fa0f5ea in dbus_connection_dispatch (connection=0xf95e40) at dbus-connection.c:4593 #5 0x0000000000460432 in sbus_dispatch (ev=0xf89390, te=0xf98bc0, tv=..., data=0xf979a0) at src/sbus/sssd_dbus_connection.c:104 #6 0x000000385b407600 in tevent_common_loop_timer_delay (ev=ev@entry=0xf89390) at ../tevent_timed.c:254 #7 0x000000385b406cac in std_event_loop_once (ev=0xf89390, location=<optimized out>) at ../tevent_standard.c:560 #8 0x000000385b403ed0 in _tevent_loop_once (ev=ev@entry=0xf89390, location=location@entry=0x492f97 "src/util/server.c:554") at ../tevent.c:506 #9 0x000000385b40405b in tevent_common_loop_wait (ev=0xf89390, location=0x492f97 "src/util/server.c:554") at ../tevent.c:607 #10 0x0000000000467bf4 in server_loop (main_ctx=0xf8a4a0) at src/util/server.c:554 #11 0x0000000000408d5c in main (argc=3, argv=0x7fff2dde9cc8) at src/responder/nss/nsssrv.c:422 (gdb) p *info $1 = {rctx = 0x3133623833347830, dom = 0x306d61736c3a313a, hint = 0x696e403334346336 <Address 0x696e403334346336 out of bounds>, force = 114, state = 0x0}
It looks like "info" was not initialized.
Thanks.
The problem was that callback is invoked immediately when tevent_req_done() is called. And because req is freed in this callback, info was pointing to invalid data.
New patch is attached.
I think it would be much cleaner to remove the tevent_req_post completely from get_domains_next() and only call it in sss_dp_get_domains_send() if get_domains_next() returned EOK.
I wouldn't expect tevent_req_post to be called in any other function that *_send. Also checking the SSSD code shows that we only call tevent_req_post from *_send elsewhere.
As you wish. Patch is attached.
Ack
Pushed to master.
On Wed, 2012-08-15 at 15:56 +0200, Jakub Hrozek wrote:
I wouldn't expect tevent_req_post to be called in any other function that *_send. Also checking the SSSD code shows that we only call tevent_req_post from *_send elsewhere.
This is a mandatory part of the tevent_req style. tevent_req_post() is only ever safe during a _send() function. Once the code re-enters the mainloop, you must never call tevent_req_post(). To that end, we only allow _send() functions to call it.
The problem would be if you reused a function later, you might call tevent_req_post() after re-entering the mainloop, and then all sorts of things will break. It's happened before.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Pavel Březina -
Stephen Gallagher