https://bugs.openldap.org/show_bug.cgi?id=9990
Issue ID: 9990 Summary: Part of the ITS#8698 fix breaks exop overlays that set a callback Product: OpenLDAP Version: 2.5.13 Hardware: All OS: All Status: UNCONFIRMED Keywords: needs_review Severity: normal Priority: --- Component: slapd Assignee: bugs@openldap.org Reporter: subbarao@computer.org Target Milestone: ---
We have a password exop overlay that sets up a callback, which has stopped working when upgrading to 2.5.13. and I tracked it down to a change to servers/slapd/passwd.c implemented as part of the fix for ITS#8698:
https://git.openldap.org/openldap/openldap/-/merge_requests/304/diffs?commit...
It appears that the intent of this change was to loop through the o_callback list and only remove the cb callback created in this section of the code. But that isn't necessary because the cb callback never gets added to the original list. With this change, line 295 clobbers the original o_callback list which never gets restored -- that's why our exop overlay stopped working.
Fortunately, the fix is very simple -- just revert this part of the change. The original code already saved/restored the o_callback list properly.
When I reverted this part of the change, our exop overlay resumed working, and the rest of the ITS#8698 functionality (messages from the pwdCheckModule module being returned to the user) also worked as expected.
https://bugs.openldap.org/show_bug.cgi?id=9990
subbarao@computer.org subbarao@computer.org changed:
What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.openldap.org/s | |how_bug.cgi?id=8698
https://bugs.openldap.org/show_bug.cgi?id=9990
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Target Milestone|--- |2.5.15 Keywords|needs_review |
https://bugs.openldap.org/show_bug.cgi?id=9990
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|bugs@openldap.org |ondra@mistotebe.net
https://bugs.openldap.org/show_bug.cgi?id=9990
--- Comment #1 from Ondřej Kuzník ondra@mistotebe.net --- On Thu, Jan 26, 2023 at 10:55:26PM +0000, openldap-its@openldap.org wrote:
We have a password exop overlay that sets up a callback, which has stopped working when upgrading to 2.5.13. and I tracked it down to a change to servers/slapd/passwd.c implemented as part of the fix for ITS#8698:
https://git.openldap.org/openldap/openldap/-/merge_requests/304/diffs?commit...
It appears that the intent of this change was to loop through the o_callback list and only remove the cb callback created in this section of the code. But that isn't necessary because the cb callback never gets added to the original list. With this change, line 295 clobbers the original o_callback list which never gets restored -- that's why our exop overlay stopped working.
Fortunately, the fix is very simple -- just revert this part of the change. The original code already saved/restored the o_callback list properly.
When I reverted this part of the change, our exop overlay resumed working, and the rest of the ITS#8698 functionality (messages from the pwdCheckModule module being returned to the user) also worked as expected.
Hi, unfortunately that commit makes sure e.g. sr_text is not leaked by preserving other callbacks that might have been added in front of it (I have double checked now), so reverting it is not an option. Can you check whether ppolicy_text_cleanup is actually at fault, changing the line: op->o_callback = sc->sc_next;
To op->o_callback = NULL;
If that doesn't improve things, you might need to provide a stripped down version of your overlay that demonstrates the issue (you don't have to provide a password checking module alongside for now).
Thanks,
https://bugs.openldap.org/show_bug.cgi?id=9990
--- Comment #2 from subbarao@computer.org subbarao@computer.org --- Hi Ondrej,
It looks like we have a communication disconnect :-) Please note that there are *two* commits in this merge request:
1) commit #1 to ppolicy_text_cleanup() in ppolicy.c. This commit is fine -- it only cleans up its own callback, no problems there. (This is what I meant by "the rest of the ITS#8698 functionality...worked as expected")
2) commit #2 to passwd_extop() in passwd.c. This is the commit that is causing the problem. I am only asking to revert this part of the change.
Does this help clarify my request? If I'm missing something, please let me know.
Thanks,
-Kartik
On 1/31/23 7:16 AM, openldap-its@openldap.org wrote:
https://bugs.openldap.org/show_bug.cgi?id=9990
--- Comment #1 from Ondřej Kuzník ondra@mistotebe.net --- On Thu, Jan 26, 2023 at 10:55:26PM +0000, openldap-its@openldap.org wrote:
We have a password exop overlay that sets up a callback, which has stopped working when upgrading to 2.5.13. and I tracked it down to a change to servers/slapd/passwd.c implemented as part of the fix for ITS#8698:
https://git.openldap.org/openldap/openldap/-/merge_requests/304/diffs?commit...
It appears that the intent of this change was to loop through the o_callback list and only remove the cb callback created in this section of the code. But that isn't necessary because the cb callback never gets added to the original list. With this change, line 295 clobbers the original o_callback list which never gets restored -- that's why our exop overlay stopped working.
Fortunately, the fix is very simple -- just revert this part of the change. The original code already saved/restored the o_callback list properly.
When I reverted this part of the change, our exop overlay resumed working, and the rest of the ITS#8698 functionality (messages from the pwdCheckModule module being returned to the user) also worked as expected.
Hi, unfortunately that commit makes sure e.g. sr_text is not leaked by preserving other callbacks that might have been added in front of it (I have double checked now), so reverting it is not an option. Can you check whether ppolicy_text_cleanup is actually at fault, changing the line: op->o_callback = sc->sc_next;
To op->o_callback = NULL;
If that doesn't improve things, you might need to provide a stripped down version of your overlay that demonstrates the issue (you don't have to provide a password checking module alongside for now).
Thanks,
https://bugs.openldap.org/show_bug.cgi?id=9990
--- Comment #3 from Ondřej Kuzník ondra@mistotebe.net --- On Tue, Jan 31, 2023 at 02:25:21PM +0000, openldap-its@openldap.org wrote:
Hi Ondrej,
It looks like we have a communication disconnect :-) Please note that there are *two* commits in this merge request:
- commit #1 to ppolicy_text_cleanup() in ppolicy.c. This commit is fine
-- it only cleans up its own callback, no problems there. (This is what I meant by "the rest of the ITS#8698 functionality...worked as expected")
- commit #2 to passwd_extop() in passwd.c. This is the commit that is
causing the problem. I am only asking to revert this part of the change.
Hi Kartik, ppolicy_text_cleanup is added onto op->o_callback, however it wasn't being called because passwd_extop would remove it and the memory still wasn't being freed, hence the other patch you're objecting to.
As mentioned, it might be that ppolicy_text_cleanup doesn't do the right thing when it's removing itself afterwards and that's what you're seeing?
https://bugs.openldap.org/show_bug.cgi?id=9990
--- Comment #4 from subbarao@computer.org subbarao@computer.org --- Created attachment 948 --> https://bugs.openldap.org/attachment.cgi?id=948&action=edit exop overlay that demonstrates the problem
In 2.5.13, the overlay only prints the its_exop_passwd trace message. The its9990_exop_response() function is never called.
With the passwd.c portion of ITS#8698 reverted, the overlay prints both the its_exop_passwd and its_exop_response trace messages. I recognize though that the fix may be more complex.
https://bugs.openldap.org/show_bug.cgi?id=9990
--- Comment #5 from subbarao@computer.org subbarao@computer.org --- On 1/31/23 10:54 AM, openldap-its@openldap.org wrote:
Hi Kartik, ppolicy_text_cleanup is added onto op->o_callback, however it wasn't being called because passwd_extop would remove it and the memory still wasn't being freed, hence the other patch you're objecting to.
As mentioned, it might be that ppolicy_text_cleanup doesn't do the right thing when it's removing itself afterwards and that's what you're seeing?
Reading your comments and reviewing the code further, it looks like things may be more complex than my first read. I didn't stop to consider if line 302 op->o_be->be_modify(...) could introduce *new* items onto the op->o_callback list. Sorry about my confusion :-)
Rather than trying to unravel the code further on my own, let me provide an example overlay that no longer works after the ITS#8698 fix. Please see the attached its9990.c file, let me know what you see as the best solution.
Thanks,
-Kartik
https://bugs.openldap.org/show_bug.cgi?id=9990
--- Comment #6 from Ondřej Kuzník ondra@mistotebe.net --- On Tue, Jan 31, 2023 at 08:52:25PM +0000, openldap-its@openldap.org wrote:
Reading your comments and reviewing the code further, it looks like things may be more complex than my first read. I didn't stop to consider if line 302 op->o_be->be_modify(...) could introduce *new* items onto the op->o_callback list. Sorry about my confusion :-)
Rather than trying to unravel the code further on my own, let me provide an example overlay that no longer works after the ITS#8698 fix. Please see the attached its9990.c file, let me know what you see as the best solution.
Hi Kartik, thanks for the example code, can you try this: https://git.openldap.org/ondra/openldap/-/commit/5a8f6a825d47af7e413b9432b29...
https://bugs.openldap.org/show_bug.cgi?id=9990
--- Comment #7 from subbarao@computer.org subbarao@computer.org --- On 2/1/23 6:44 AM, openldap-its@openldap.org wrote:
Hi Kartik, thanks for the example code, can you try this: https://git.openldap.org/ondra/openldap/-/commit/5a8f6a825d47af7e413b9432b29...
It works! Glad it was a simple fix :-)
Thanks,
-Kartik
https://bugs.openldap.org/show_bug.cgi?id=9990
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |TEST Status|UNCONFIRMED |RESOLVED
--- Comment #8 from Quanah Gibson-Mount quanah@openldap.org --- • fa64703e by Ondřej Kuzník at 2023-02-01T16:56:37+00:00 ITS#9990 Preserve callbacks added already
https://bugs.openldap.org/show_bug.cgi?id=9990
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|TEST |FIXED
--- Comment #9 from Quanah Gibson-Mount quanah@openldap.org --- RE26:
• 85e2c3fb by Ondřej Kuzník at 2023-04-17T18:42:12+00:00 ITS#9990 Preserve callbacks added already
RE25:
• 2e79e2d0 by Ondřej Kuzník at 2023-04-17T18:43:26+00:00 ITS#9990 Preserve callbacks added already
https://bugs.openldap.org/show_bug.cgi?id=9990
Quanah Gibson-Mount quanah@openldap.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |VERIFIED
-
openldap-its＠openldap.org