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 f3c3125fc9fd4dc1cbd41027d6442cb525efd014

Pushed to 389-ds-base-1.3.3:
a805e25..97c707d 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 97c707df4e11936202310e99bcf5f45d15b49f0f

Pushed to 389-ds-base-1.2.11:
6024a77..490830f 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit 490830fab691c904f5f88f503cf510a9c68477a4

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 298371d372678cf553594ae73ae57a6ea35358bf

Pushed to 389-ds-base-1.3.3:
97c707d..7718eb6 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 7718eb6a6714d1a284c3c706e621a7eb0ca5655a

Pushed to 389-ds-base-1.2.11:
490830f..7db5fdd 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit 7db5fdd5f1c5e1979784833173864e9c8c49c89d

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 1d18dd0107d48ac1d79f7c9988adf18b0905bbdb

Pushed to 389-ds-base-1.3.3:
7718eb6..798eae4 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 798eae4f2240a5b47963a2bb09a2a17acfc488ec

Pushed to 389-ds-base-1.2.11:
7db5fdd..16a8ef1 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit 16a8ef16724f1f657366169a7fd0ae1686b61d4b

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

git patch file (master) -- .4 patch + additional-fixes patch
0001-Ticket-48192-Individual-abandoned-simple-paged-resul.5.patch

Reviewed by Rich (Thank you!!)

Pushed to master:
b7b663c..e4d83c9 master -> master
commit e4d83c91fc88fcf9e6c823c608c629ac10e362f8

Pushed to 389-ds-base-1.3.4:
7c73adc..b513a50 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit b513a502250f93cfb43df000c2140b27c4ef0d39

Pushed to 389-ds-base-1.3.3:
a71b48c..7a05195 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 7a05195020124690111cd27724efe0bf6d2d63c9

Pushed to 389-ds-base-1.2.11:
84b03a8..8173360 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit 81733602478133b3847914f30733440d1d1608b2

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 6e453918e82af6c597390aebf92a8eb3283c3591

Pushed to 389-ds-base-1.3.4:
56151ed..96b9b67 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit 96b9b6794e0a6bfa0d74c84f6c80131c4f820fa7

Pushed to 389-ds-base-1.3.3:
10ed80c..4a4a7ed 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 4a4a7ed06d3271f7337e5fac5f85498dfe79b0db

Pushed to 389-ds-base-1.2.11:
54e574a..fb94767 389-ds-base-1.2.11 -> 389-ds-base-1.2.11
commit fb94767d78e15617101ff8299c724194f0c858ea

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.

Thank you for understanding. We apologize for all inconvenience.