#48299 pagedresults - when timed out, search results could have been already freed.
Closed: wontfix None Opened 5 years ago by nhosoi.

Possible to free search results twice.


Hi noriko,

The fix looks good, just two remarks
* pagedresults_set_search_result_pb is called each time search result is freed. Should not it be called in opshared.c:722 (I find this line suspicious should not it be SLAPI_PAGED_RESULTS_INDEX ?)
* in pagedresults_set_search_result_pb, you call slapi_pblock_set(pb, SLAPI_PAGED_RESULTS_INDEX, &index); . My understanding it is to reset the pr_idx (index==-1). But later you test 'index'. Did you mean slapi_pblock_'''get'''(pb, SLAPI_PAGED_RESULTS_INDEX, &index).

Thank you, Thierry!

Replying to [comment:4 tbordaz]:

Hi noriko,

The fix looks good, just two remarks
* pagedresults_set_search_result_pb is called each time search result is freed. Should not it be called in opshared.c:722 (I find this line suspicious should not it be SLAPI_PAGED_RESULTS_INDEX ?)

This is more of the protection from double free and/or NULL dereference. The search result is freed in the paged results API: pagedresults_free_one(pb->pb_conn, operation, pr_idx), so by setting NULL to SLAPI_SEARCH_RESULT_SET in pblock, no other place in the thread sharing the pblock has no chance to touch it.

  • in pagedresults_set_search_result_pb, you call slapi_pblock_set(pb, SLAPI_PAGED_RESULTS_INDEX, &index); . My understanding it is to reset the pr_idx (index==-1). But later you test 'index'. Did you mean slapi_pblock_'''get'''(pb, SLAPI_PAGED_RESULTS_INDEX, &index).

Agh... Thank you soooo much, Thierry. shame shame I'm running tests with the fix... A revised patch is coming.

git patch file (master) -- revised; fixed a bug pointed out by Thierry (Thank you!!) and a minor memory leak.
0001-Ticket-48299-pagedresults-when-timed-out-search-resu.patch

Hi Noriko,

The fix looks good to me. Good catch for the leak.
ACK

Reviewed and a bug found by Thierry (Thank you!!)

Pushed to master:
3d0ae27..f90c3a6 master -> master
commit f90c3a6

Pushed to 389-ds-base-1.3.4:
5bc311b..56151ed 389-ds-base-1.3.4 -> 389-ds-base-1.3.4
commit 56151ed

Pushed to 389-ds-base-1.3.3:
def55f1..10ed80c 389-ds-base-1.3.3 -> 389-ds-base-1.3.3
commit 10ed80c

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

Metadata Update from @nhosoi:
- Issue assigned to nhosoi
- Issue set to the milestone: 1.2.11.33

3 years ago

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/1630

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.

Metadata Update from @spichugi:
- Issue close_status updated to: wontfix (was: Fixed)

a month ago

Login to comment on this ticket.

Metadata