[Asterisk-code-review] sched: fix and test a double deref on a scheduled delete of an execut... (asterisk[16])
Michael Bradeen
asteriskteam at digium.com
Fri Jan 7 11:26:29 CST 2022
Attention is currently required from: Sean Bright, Joshua Colp, Kevin Harwell.
Michael Bradeen has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/17644 )
Change subject: sched: fix and test a double deref on a scheduled delete of an executing call back.
......................................................................
Patch Set 9:
(30 comments)
Commit Message:
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/f8dd33c8_f3e63860
PS3, Line 7: various: fix and test a double deref on a scheduled delete of an
: executing call back.
> Try to shorten this line to 72 characters. Also use sched or sched. […]
Thanks, also needed to be updated vs initial commit.
Patchset:
PS8:
> Looks like you've addressed my feedback, but giving a -1 as it appears Sean's has still not been add […]
Thanks, I had changed it earlier but didn't update on later amends which over-wrote the change. It has been reverted to reflect Sean's comments.
File include/asterisk/sched.h:
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/9530e4f8_4ec8b850
PS7, Line 75: * Only calls unref function if the delete succeeded.
> I think this could use additional clarification: […]
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/73667d4f_df18fb56
PS7, Line 315: * \retval -2 event was running and not rescheduled
> Reword this a bit to say something like "-2 event was running, but deleted via non rescheduling.
Done
File include/asterisk/sched.h:
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/daeab037_acc8be37
PS5, Line 304: * \brief Deletes a scheduled event provided the event is not running
> I don't think this is descriptive enough, because the event may be running and then get deleted rega […]
Done
File include/asterisk/sched.h:
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/1438d198_e6ba71e9
PS2, Line 296: * \retval -2 event was running and not rescheduled
> I don't think changing this API call is a good idea, because other users of it may not be written to […]
Done
File main/sched.c:
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/290d4b3b_2e445fda
PS2, Line 651: /* Do not sched_release() here because ast_sched_runq() will do it */
> This is no longer accurate.
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/0d5f338c_d8ec7ed6
PS2, Line 658: sched_release(con, s);
> "s" gets freed in sched_release, and should be set to NULL after calling sched_release since "s" is […]
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/c96cdace_a737b737
PS2, Line 658: sched_release(con, s);
> Add a comment that ast_sched_runq passes responsibility for release to us in this case.
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/a8e927e4_34654fad
PS2, Line 807: current->rescheduled = res ? 1 : 0;
> Add a comment explaining what has happened here (that another thread is waiting on this scheduled it […]
Done
File main/sched.c:
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/38c63089_d7bc00cc
PS6, Line 623: * schedule the relase.
> release
Done
File main/sched.c:
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/5b6dded2_9a32d92f
PS7, Line 666: /* This was not rescheduled so the caller of ast_sched_del can not remove any
: * references as they already were.
: */
> I think this comment can be deleted. […]
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/10be68c4_5619e8bf
PS7, Line 673: * it's destruction to us
> s/it's/its
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/68c578f5_a8487e66
PS7, Line 688: if (!s && *last_id != id) {
: ast_debug(1, "Attempted to delete nonexistent schedule entry %d!\n", id);
: /* Removing nonexistent schedule entry shouldn't trigger assert (it was enabled in DEV_MODE);
: * because in many places entries is deleted without having valid id. */
: *last_id = id;
: return -1;
: } else if (!s) {
: return -1;
: }
:
: return res;
> When you set "s" to NULL above this might need to be reworked as this function will now always retur […]
Done
File tests/test_sched.c:
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/a54b6cd0_7dc147f3
PS2, Line 366: usleep(3000);
> Add a comment explaining why this sleep exists
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/066ea8bd_5c645431
PS2, Line 366: usleep(3000);
> usleep can be interrupted, so this probably needs to be in a loop to ensure the full 3 seconds passe […]
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/55c53ed1_66fcafe0
PS2, Line 384: "This test forces a thread conflict issue on res mem sorcery.";
> This is actually just testing the scheduler
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/093690bb_e9adc68a
PS2, Line 391: if (!(obj = ao2_alloc(sizeof(struct test_obj), test_obj_cleanup))) {
> We generally put the obj = ao2_alloc on a separate line from the if.
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/2f243e63_611726df
PS2, Line 406: return AST_TEST_FAIL;
> obj is leaked
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/10bec16c_119f00e4
PS2, Line 409: if(ast_sched_start_thread(con)) {
> Space between if and (
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/1c38eb22_299a39b1
PS2, Line 412: return AST_TEST_FAIL;
> obj is leaked
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/29587423_60bd2954
PS2, Line 419: return AST_TEST_FAIL;
> obj is leaked
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/4b7107f6_06d27773
PS2, Line 423: while(obj->scheduledCBstarted == 0) {
> Add a comment explaining what this is waiting on and why
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/a6b24cb5_31d8baf4
PS2, Line 439: ast_test_status_update(test, "Inorrect number of references '%d'\n",
> Inorrect is incorrect
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/ac68df8d_c231736e
PS2, Line 444:
> obj is leaked - the easiest way to destroy it is to probably loop until ao2_ref(obj, -1) returns 0.
Done
File tests/test_sched.c:
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/5e96835d_bc7f651d
PS5, Line 457: ao2_cleanup(obj);
> This can still leak. […]
Done
File tests/test_sched.c:
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/cb4ab88a_b50529ec
PS6, Line 426: while(!ao2_ref(obj, -1));
> The ao2_ref function returns the current reference count before the operation, so this isn't correct […]
Done
File tests/test_sched.c:
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/b6737288_e7b17e68
PS7, Line 405: ao2_ref(obj, +1);
> If you move this to just before sched_add then you'll no longer leak "obj" in the below off nominal […]
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/336b7eab_3acd4aff
PS7, Line 423: ao2_bump(obj);
> If you move the add ref above this can become ao2_ref(obj, +2); […]
Done
https://gerrit.asterisk.org/c/asterisk/+/17644/comment/4e4afa8a_15756383
PS7, Line 426: while(ao2_ref(obj, -1) > 1);
> If this fails you know you have 3 additional refs currently, so ao2_ref(obj, -3) should be sufficien […]
Done
--
To view, visit https://gerrit.asterisk.org/c/asterisk/+/17644
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings
Gerrit-Project: asterisk
Gerrit-Branch: 16
Gerrit-Change-Id: Icfb16b3acbc29cf5b4cef74183f7531caaefe21d
Gerrit-Change-Number: 17644
Gerrit-PatchSet: 9
Gerrit-Owner: Michael Bradeen <mbradeen at sangoma.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Sean Bright <sean at seanbright.com>
Gerrit-Attention: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Fri, 07 Jan 2022 17:26:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Sean Bright <sean at seanbright.com>
Comment-In-Reply-To: Joshua Colp <jcolp at sangoma.com>
Comment-In-Reply-To: Kevin Harwell <kharwell at digium.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20220107/aa1a498c/attachment-0001.html>
More information about the asterisk-code-review
mailing list