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 ?)
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
Ticket has been cloned to Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1267750
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
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.
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.