[Asterisk-code-review] json: Audit ast json * usage for thread safety. (asterisk[13])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: json: Audit ast_json_* usage for thread safety.
......................................................................


Patch Set 1:

(6 comments)

https://gerrit.asterisk.org/#/c/1828/1/main/aoc.c
File main/aoc.c:

Line 1670: 		"{s:s, s:s, s:s, s:o}",
This use of O was safe as obj was just created above.  Therefore, there is no other thread which could interfere with the json ref count even in extreme cases of malloc failure within ast_json_pack().

The ref stealing 'o' now introduces a ref leak of obj if ast_json_pack() should suffer a malloc failure.  (DEBUG_CHAOS anyone?)  You don't know if the ref was stolen yet when the malloc failure occurs.  In fact if a malloc failure occurs you will usually wind up with the json library unrefing objects outside of thread safety.  Any destroyed json objects would be placed on a free_list and not be freed until that thread later uses ast_json_unref on another json object.

The "{s:s}" creates a json object container and a string object held within the container.  Both are allocated by ast_json_pack().  On a malloc failure those objects are unreffed without thread safety.

For "{s:o}" and "{s:O}" when the last ref to the json container object returned by ast_json_pack() is unreffed those external object refs will be unreffed unsafely if other threads have a ref to the external objects.


Line 1741: 			type = ast_json_pack("{s:o, s:s, s:o, s:o}", "Currency", ast_json_ref(currency), "ChargingType",
These O usages are safe for the same reason.  currency, time, and granularity were just created.


https://gerrit.asterisk.org/#/c/1828/1/main/loader.c
File main/loader.c:

Line 855: 	json_object = ast_json_pack("{s: s, s: i, s: o}",
This was safe


https://gerrit.asterisk.org/#/c/1828/1/main/rtp_engine.c
File main/rtp_engine.c:

Line 2011: 	json_rtcp_report = ast_json_pack("{s: i, s: i, s: i, s: o, s: o}",
This was safe


https://gerrit.asterisk.org/#/c/1828/1/res/res_fax.c
File res/res_fax.c:

Line 2031: 		json_object = ast_json_pack("{s: s, s: s, s: s, s: s, s: s, s: s, s: o}",
This was safe


https://gerrit.asterisk.org/#/c/1828/1/res/stasis/app.c
File res/stasis/app.c:

Line 613: 	app_send(app, ast_json_pack("{s: s, s: o, s: o, s: o}",
This may have been safe depending upon if all callers just created json_msg before calling.


-- 
To view, visit https://gerrit.asterisk.org/1828
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I06550d8b0cc1bfeb56cab580a4e608ae4f1ec7d1
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes