If multiple asynchronous simple paged results requests are received in one connection and if some of them are abandoned, the abandoned requests are not cleaned up until the connection is closed or all of the requests exceed the configured timelimit and the connection is closed.
2 simple paged results CI tests passed: ticket47664_test.py ticket47824_test.py
Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1228402
git patch file (master) 0001-Ticket-48192-Individual-abandoned-simple-paged-resul.patch
Reviewed by Rich (Thank you!!)
Pushed to master: 25c3b83..f3c3125 master -> master commit f3c3125
Pushed to 389-ds-base-1.3.3: a805e25..97c707d 389-ds-base-1.3.3 -> 389-ds-base-1.3.3 commit 97c707d
Pushed to 389-ds-base-1.2.11: 6024a77..490830f 389-ds-base-1.2.11 -> 389-ds-base-1.2.11 commit 490830f
Hi Noriko,
In pagedresult.c:179 there is "index = strtol(ptr, NULL, 10);" I do not know if it is possible for ldap client to create a cookie with negative value. But if it is, 'index' will access wrong index in prl_list and may crash.
Replying to [comment:8 tbordaz]:
Hi Noriko, In pagedresult.c:179 there is "index = strtol(ptr, NULL, 10);" I do not know if it is possible for ldap client to create a cookie with negative value. But if it is, 'index' will access wrong index in prl_list and may crash. Indeed. Thanks, Thierry! I should have checked if Iindex satisfies this condition before accessing any. ((index > -1) && (index < conn->c_pagedresults.prl_maxlen)) Adding another patch...
Indeed. Thanks, Thierry! I should have checked if Iindex satisfies this condition before accessing any. ((index > -1) && (index < conn->c_pagedresults.prl_maxlen)) Adding another patch...
git patch file (master) -- fixing a possible array out of bounds issue pointed out by Thierry (Thanks!) 0001-Ticket-48192-Individual-abandoned-simple-paged-resul.2.patch
The additional patch is looking good to me. ACK
Thanks to Thierry for pointing out the bug!
Pushed to master: f3c3125..298371d master -> master commit 298371d
Pushed to 389-ds-base-1.3.3: 97c707d..7718eb6 389-ds-base-1.3.3 -> 389-ds-base-1.3.3 commit 7718eb6
Pushed to 389-ds-base-1.2.11: 490830f..7db5fdd 389-ds-base-1.2.11 -> 389-ds-base-1.2.11 commit 7db5fdd
The patch looks ok for me, but I have two remarks:
In pagedresult.c:205, a new request will receive an index (cookie). At the same time if it is a very short timelimit, _pr_cleanup_one_slot may be called. So the cookie will refer to cleared slot. I think it should work and eventually the sent cookie will be empty. But IMHO it would be safer to return index=-1, if pagedresult.c:205 clears the allocated *index.
In opshared:907 and 919. On an abandoned page result request, I wonder if we are not going to return two pr_control ? One with the valid cookie (if there is still some entries to return) and one with an empty cookie.
tentative diff to fix the issues pointed out by Thierry. Thanks!! diffs.txt
Replacing the patch 3 with the additional fix pointed out by Thierry...
Hello Noriko,
The patch looks good to me. ACK. I have a minor remark, in pagedresults.c:176. In case of "Repeated paged results request.", if the cookie provided by the ldapclient is invalid (too large or negative), '*index' is set
{{{ index = strtol(ptr, NULL, 10); slapi_ch_free_string(&ptr); if ((conn->c_pagedresults.prl_maxlen <= index) || (index < 0)){ rc = LDAP_PROTOCOL_ERROR; LDAPDebug1Arg(LDAP_DEBUG_ANY, "pagedresults_parse_control_value: invalid cookie: %d\n", index); goto bail; } }}}
This invalid value will be stored in pblock[SLAPI_PAGED_RESULTS_INDEX].
{{{ / be is set only when this request is new. otherwise, prev be is honored. / rc = pagedresults_parse_control_value(pb, ctl_value, &pagesize, &pr_idx, be); / Let's set pr_idx even if it fails; in case, pr_idx == -1. / slapi_pblock_set(pb, SLAPI_PAGED_RESULTS_INDEX, &pr_idx); if ((LDAP_SUCCESS == rc) || (LDAP_CANCELLED == rc) || (0 == pagesize)) { ... } else { / parse paged-results-control failed / if (iscritical) { / return an error since it's critical / send_ldap_result(pb, LDAP_UNAVAILABLE_CRITICAL_EXTENSION, NULL, "Simple Paged Results Search failed", 0, NULL); goto free_and_return; } } }}}
If the control is not critical, the operation continue and it is difficult to predict what could be the consequence of this invalid value.
Is it possible to reset '*index=-1' if the cookie is found invalid.
thanks thierry
Thank you, Thierry! I'm adding this diff to the patch. Negative values are okay (I'm checking the index with "> -1"), but too large is BAD... {{{ @@ -174,8 +174,8 @@ pagedresults_parse_control_value( Slapi_PBlock pb, if ((conn->c_pagedresults.prl_maxlen <= index) || (index < 0)){ rc = LDAP_PROTOCOL_ERROR; LDAPDebug1Arg(LDAP_DEBUG_ANY, - "pagedresults_parse_control_value: invalid cookie: %d\n", - index); + "pagedresults_parse_control_value: invalid cookie: %d\n", index); + index = -1; / index is invalid. reinitializing it. / goto bail; } prp = conn->c_pagedresults.prl_list + *index; }}}
git patch file (master) -- fixing a memory leak as well as bugs found by Thierry (Thank you!!) 0001-Ticket-48192-Individual-abandoned-simple-paged-resul.3.patch
Thank you so much, Thierry, for reviewing the patch and giving me the useful suggestions!
Pushed to master: 298371d..1d18dd0 master -> master commit 1d18dd0
Pushed to 389-ds-base-1.3.3: 7718eb6..798eae4 389-ds-base-1.3.3 -> 389-ds-base-1.3.3 commit 798eae4
Pushed to 389-ds-base-1.2.11: 7db5fdd..16a8ef1 389-ds-base-1.2.11 -> 389-ds-base-1.2.11 commit 16a8ef1
{{{ 678 678 pr_search_result = pagedresults_get_search_result(pb->pb_conn, operation, pr_idx); 679 if (pr_search_result) { 679 if (pr_search_result && !pagedresults_is_abandoned(pb->pb_conn, pr_idx)) { 680 680 slapi_pblock_set( pb, SLAPI_SEARCH_RESULT_SET, pr_search_result ); 681 681 rc = send_results_ext (pb, 1, &pnentries, pagesize, &pr_stat); }}}
1) There is no locking done here, and pagedresults_is_abandoned does not lock. 2) Are you sure that a result does not need to be sent in this case, or that the correct result has already been sent? 3) Is there any other code or cleanup done in the if block starting at line 679 that we will now be skipping?
{{{ 992 int 993 pagedresults_is_abandoned( Connection conn, int index ) 994 { 995 PagedResults prp; 996 if (!conn || (index < 0) || (index >= conn->c_pagedresults.prl_maxlen)) { 997 return 1; / not abandoned, but do not want to proceed paged results op. / 998 } 999 prp = conn->c_pagedresults.prl_list + index; 1000 return prp->pr_flags & CONN_FLAG_PAGEDRESULTS_ABANDONED; 1001 } }}}
1) This function doesn't do any locking - either it should lock, or it should be noted that the caller must lock whatever needs to be locked. 2) If the function is checking for more than just PAGEDRESULTS_ABANDONED, it should be renamed.
git patch file (master) -- fixing issues pointed by Rich in comment:24 (Thank you!!) 0001-Ticket-48192-Individual-abandoned-simple-paged-resul.4.patch
I think the new patch covered the comments made by Rich except this: 3) Is there any other code or cleanup done in the if block starting at line 679 that we will now be skipping?
If abandoned, the paged results object and the search results are cleaned up. The problem we are trying to solve by this patch Attachment 0001-Ticket-48192-Individual-abandoned-simple-paged-resul.4.patch​ is double cleanup.
Ok, I see. Before the calls there is a pagedresults_lock which locks the current prp at the current index. However, this lock is acquired without first locking conn before accessing conn->c_pagedresults.prl_list
1) Is it necessary to lock conn before accessing conn->c_pagedresults.prl_list? 2) Is it necessary to lock conn in pagedresults_is_abandoned_or_notavailable if prp is already locked?
Replying to [comment:26 rmeggins]:
Ok, I see. Before the calls there is a pagedresults_lock which locks the current prp at the current index. However, this lock is acquired without first locking conn before accessing conn->c_pagedresults.prl_list 1) Is it necessary to lock conn before accessing conn->c_pagedresults.prl_list? Yes, prl_list could be realloc'ed when the last pr index reaches prl_maxlen in pagedresults_parse_control_value. The opeartion is in conn->c_mutex lock. 2) Is it necessary to lock conn in pagedresults_is_abandoned_or_notavailable if prp is already locked? A good question... If PagedResults *prp is NOT hidden, we could just call pagedresults_is_abandoned with prp and the prp lock protects it. The problem is even if prp is given from the list with pr_idx and the prp object is protected by the lock on prp, prl_list could be be updated due to the realloc in pagedresults_parse_control_value. So, to access it in pagedresults_is_abandoned_or_notavailable, I need to lock it, I think. :(
1) Is it necessary to lock conn before accessing conn->c_pagedresults.prl_list? Yes, prl_list could be realloc'ed when the last pr index reaches prl_maxlen in pagedresults_parse_control_value. The opeartion is in conn->c_mutex lock.
2) Is it necessary to lock conn in pagedresults_is_abandoned_or_notavailable if prp is already locked? A good question... If PagedResults *prp is NOT hidden, we could just call pagedresults_is_abandoned with prp and the prp lock protects it. The problem is even if prp is given from the list with pr_idx and the prp object is protected by the lock on prp, prl_list could be be updated due to the realloc in pagedresults_parse_control_value. So, to access it in pagedresults_is_abandoned_or_notavailable, I need to lock it, I think. :(
git patch file (master) -- additional fix to 0001-Ticket-48192-Individual-abandoned-simple-paged-resul.4.patch​ 0001-additional-fixes-this-will-be-merged-into-one-formal.patch
git patch file (master) -- .4 patch + additional-fixes patch 0001-Ticket-48192-Individual-abandoned-simple-paged-resul.5.patch
Pushed to master: b7b663c..e4d83c9 master -> master commit e4d83c9
Pushed to 389-ds-base-1.3.4: 7c73adc..b513a50 389-ds-base-1.3.4 -> 389-ds-base-1.3.4 commit b513a50
Pushed to 389-ds-base-1.3.3: a71b48c..7a05195 389-ds-base-1.3.3 -> 389-ds-base-1.3.3 commit 7a05195
Pushed to 389-ds-base-1.2.11: 84b03a8..8173360 389-ds-base-1.2.11 -> 389-ds-base-1.2.11 commit 8173360
git patch file (master) -- if abandoned, do not set search results. 0001-Ticket-48192-Individual-abandoned-simple-paged-resul.6.patch
Thank you for reviewing the patch, Rich!!
Pushed to master: f90c3a6..6e45391 master -> master commit 6e45391
Pushed to 389-ds-base-1.3.4: 56151ed..96b9b67 389-ds-base-1.3.4 -> 389-ds-base-1.3.4 commit 96b9b67
Pushed to 389-ds-base-1.3.3: 10ed80c..4a4a7ed 389-ds-base-1.3.3 -> 389-ds-base-1.3.3 commit 4a4a7ed
Pushed to 389-ds-base-1.2.11: 54e574a..fb94767 389-ds-base-1.2.11 -> 389-ds-base-1.2.11 commit fb94767
Metadata Update from @nhosoi: - Issue assigned to nhosoi - Issue set to the milestone: 1.2.11.33
389-ds-base is moving from Pagure to Github. This means that new issues and pull requests will be accepted only in 389-ds-base's github repository.
This issue has been cloned to Github and is available here: - https://github.com/389ds/389-ds-base/issues/1523
If you want to receive further updates on the issue, please navigate to the github issue and click on subscribe button.
subscribe
Thank you for understanding. We apologize for all inconvenience.
Metadata Update from @spichugi: - Issue close_status updated to: wontfix (was: Fixed)
Login to comment on this ticket.