[Asterisk-code-review] various: fix and test a double deref on a scheduled delete of an exec... (asterisk[16])

[Joshua Colp]

Attention is currently required from: Michael Bradeen.
Joshua Colp has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/17644 )

Change subject: various: fix and test a double deref on a scheduled delete of an executing call back.
......................................................................


Patch Set 2: Code-Review-1

(13 comments)

File include/asterisk/sched.h:

https://gerrit.asterisk.org/c/asterisk/+/17644/comment/d50d0bbe_9c4cb9a4 
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 handle the return value. I think a separate API call should exist which returns this more descriptive information for use by the AST_SCHED_DEL_UNREF macro and perhaps other things, and ast_sched_del should instead just return -1 for this case.


File main/sched.c:

https://gerrit.asterisk.org/c/asterisk/+/17644/comment/66d6d011_22993ecf 
PS2, Line 651: 			/* Do not sched_release() here because ast_sched_runq() will do it */
This is no longer accurate.


https://gerrit.asterisk.org/c/asterisk/+/17644/comment/68ede7f2_a56cd081 
PS2, Line 658: 			sched_release(con, s);
Add a comment that ast_sched_runq passes responsibility for release to us in this case.


https://gerrit.asterisk.org/c/asterisk/+/17644/comment/3a7715bc_e1982a12 
PS2, Line 807: 			current->rescheduled = res ? 1 : 0;
Add a comment explaining what has happened here (that another thread is waiting on this scheduled item, and thus will be responsible for destruction)


File tests/test_sched.c:

https://gerrit.asterisk.org/c/asterisk/+/17644/comment/1b7ec7ed_91b9bf57 
PS2, Line 366: 	usleep(3000);
Add a comment explaining why this sleep exists


https://gerrit.asterisk.org/c/asterisk/+/17644/comment/ae96ca32_d42aea7c 
PS2, Line 384: 			"This test forces a thread conflict issue on res mem sorcery.";
This is actually just testing the scheduler


https://gerrit.asterisk.org/c/asterisk/+/17644/comment/4f176e44_7c16c9fc 
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.


https://gerrit.asterisk.org/c/asterisk/+/17644/comment/fd521e8c_09f06bb6 
PS2, Line 406: 		return AST_TEST_FAIL;
obj is leaked


https://gerrit.asterisk.org/c/asterisk/+/17644/comment/806321e3_e358bc82 
PS2, Line 409: 	if(ast_sched_start_thread(con)) {
Space between if and (


https://gerrit.asterisk.org/c/asterisk/+/17644/comment/69c7175a_4b5b0048 
PS2, Line 412: 		return AST_TEST_FAIL;
obj is leaked


https://gerrit.asterisk.org/c/asterisk/+/17644/comment/612a4d25_6954b9f8 
PS2, Line 419: 		return AST_TEST_FAIL;
obj is leaked


https://gerrit.asterisk.org/c/asterisk/+/17644/comment/02543e9c_39046ae0 
PS2, Line 423: 	while(obj->scheduledCBstarted == 0) {
Add a comment explaining what this is waiting on and why


https://gerrit.asterisk.org/c/asterisk/+/17644/comment/8d57113d_42f64756 
PS2, Line 444: 
obj is leaked - the easiest way to destroy it is to probably loop until ao2_ref(obj, -1) returns 0.



-- 
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: 2
Gerrit-Owner: Michael Bradeen <mbradeen at sangoma.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Joshua Colp <jcolp at sangoma.com>
Gerrit-Attention: Michael Bradeen <mbradeen at sangoma.com>
Gerrit-Comment-Date: Tue, 14 Dec 2021 10:31:04 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20211214/71e20179/attachment.html>