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

[Salah Ahmed]

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

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


Patch Set 2:

(3 comments)

> (3 comments)
 > 
 > Please cherry pick this to 16, 17, 17.0, and master branches.

It looks like in 16, 17 and master branch has the rtp instance bump already implemented. So, I think, those versions are safe, no need to apply this patch.

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. […]
Done


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?
My understanding is, As we bumped the rtp reference and lock rtp inside ast_rtp_instance_get_stats, this would be enough to hold rtp session until finish the status read. I have little confusion on, is there any chance to cleanup the channel just before the rtp session get locked? If it cleanup then does it destroy the rtp object either? Please correct me if I am thinking wrong.


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 appea […]
Done



-- 
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: 2
Gerrit-Owner: Salah Ahmed <txrubel at gmail.com>
Gerrit-Reviewer: Friendly Automation
Gerrit-Reviewer: Kevin Harwell <kharwell at digium.com>
Gerrit-Reviewer: Salah Ahmed <txrubel at gmail.com>
Gerrit-Comment-Date: Wed, 16 Oct 2019 06:56:36 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
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/20191016/c4e7650b/attachment.html>