[Asterisk-code-review] rtp engine: rtcp report to json can overflow the ssrc intege... (asterisk[13])

[Corey Farrell]

Corey Farrell has posted comments on this change. ( https://gerrit.asterisk.org/10149 )

Change subject: rtp_engine: rtcp_report_to_json can overflow the ssrc integer value
......................................................................


Patch Set 3: Code-Review-1

(2 comments)

https://gerrit.asterisk.org/#/c/10149/3/include/asterisk/json.h
File include/asterisk/json.h:

https://gerrit.asterisk.org/#/c/10149/3/include/asterisk/json.h@116
PS3, Line 116: #if (HAVE_LLONG_MAX == 1 && HAVE_STRTOLL == 1)
Richard's comment about this on the master review is correct, we need to check if these are defined.  This would cause a compile error if 'long long' is not supported.

To see this for yourself you can try compiling the following:
#if (HAVE_THIS_DOES_NOT_EXIST == 1)
#endif


https://gerrit.asterisk.org/#/c/10149/3/main/json.c
File main/json.c:

https://gerrit.asterisk.org/#/c/10149/3/main/json.c@59
PS3, Line 59: 	int json_int_check[1 / (sizeof(ast_json_int_t) == sizeof(json_int_t))];
> This needs serious explanation.
If the sizes do not equal then for example (8 == 4) evaluates to 0, this would cause a divide by zero error (I think).

I think the following would be better:
#if (JSON_INTEGER_IS_LONG_LONG && !(defined(HAVE_LLONG_MAX) && defined(HAVE_STRTOLL)) || (!JSON_INTEGER_IS_LONG_LONG && (defined(HAVE_LLONG_MAX) && defined(HAVE_STRTOLL))
#error "Mismatched long long usage between jansson and Asterisk."
#endif


Honestly I'm not sure we need to bother with this check.  The chances of jansson changing the way it detects support for 'long long' is nearly non-existent.  That's the only way this would ever throw an error is if jansson added a new requirement which is unavailable on the system.

In theory configure.ac should detect JSON_INTEGER_IS_LONG_LONG but that would require some tweaking of the way bundled jansson is built.



-- 
To view, visit https://gerrit.asterisk.org/10149
To unsubscribe, or for help writing mail filters, visit https://gerrit.asterisk.org/settings

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-MessageType: comment
Gerrit-Change-Id: I2af275286ee5e795b79f0c3d450d9e4b28e958b0
Gerrit-Change-Number: 10149
Gerrit-PatchSet: 3
Gerrit-Owner: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Corey Farrell <git at cfware.com>
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Jenkins2
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Comment-Date: Thu, 20 Sep 2018 17:33:11 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-code-review/attachments/20180920/b64c5396/attachment.html>