#48299 pagedresults - when timed out, search results could have been already freed.
Closed: Fixed None Opened 4 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

2 years ago

Login to comment on this ticket.

Metadata