[Asterisk-code-review] Crash during "pjsip show channelstats" execution (...asterisk[13])

[Kevin Harwell]

Kevin Harwell has posted comments on this change. ( https://gerrit.asterisk.org/c/asterisk/+/13044 )

Change subject: Crash during "pjsip show channelstats" execution
......................................................................


Patch Set 1: Code-Review-1

(3 comments)

Please cherry pick this to 16, 17, 17.0, and master branches.

https://gerrit.asterisk.org/#/c/13044/1/channels/pjsip/cli_commands.c 
File channels/pjsip/cli_commands.c:

https://gerrit.asterisk.org/#/c/13044/1/channels/pjsip/cli_commands.c@369 
PS1, Line 369: 	if (!media || !media->rtp) {
             : 		ast_str_append(&context->output_buffer, 0, " %s not valid\n", snapshot->name);
             : 		ast_channel_unlock(channel);
             : 		ao2_cleanup(channel);
             : 		return 0;
             : 	}
             : 
             : 	rtp = ao2_bump(media->rtp);
I think there still exists the potential for rtp to be NULL here. ao2_bump is NULL tolerant. It would be better to make sure the media is not NULL, bump the rtp ref, and then check that rtp is not NULL.


https://gerrit.asterisk.org/#/c/13044/1/channels/pjsip/cli_commands.c@383 
PS1, Line 383: 	stats_res = ast_rtp_instance_get_stats(rtp, &stats, AST_RTP_INSTANCE_STAT_ALL);
Why does this need to be done with the channel locked?


https://gerrit.asterisk.org/#/c/13044/1/channels/pjsip/cli_commands.c@418 
PS1, Line 418: 	ao2_cleanup(rtp);
Very minor, but you could clean up the ref right after the call to get_stats if you want as it appears to not be used after that. Also if you've NULL checked it above there is no reason to check again here using ao2_cleanup. You could just use ao2_ref(rtp, -1).



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

Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Change-Id: Ia918bfa3c8119060633e39b14852e8d983e0417e
Gerrit-Change-Number: 13044
Gerrit-PatchSet: 1
Gerrit-Owner: Salah Ahmed <txrubel at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Comment-Date: Thu, 10 Oct 2019 22:01:17 +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/20191010/cd8001b6/attachment-0001.html>