#48192 Individual abandoned simple paged results request has no chance to be cleaned up
Closed: Fixed None Opened 4 years ago by nhosoi.

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

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...

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

Hi Noriko,

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. :(

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

Reviewed by Rich (Thank you!!)

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

2 years ago

Login to comment on this ticket.