[Asterisk-code-review] res rtp asterisk.c: don't call dtls perform handshake while ... (asterisk[13])

[Joshua Colp]

Joshua Colp has posted comments on this change.

Change subject: res_rtp_asterisk.c: don't call dtls_perform_handshake while passive
......................................................................


Patch Set 1: Code-Review-1

(4 comments)

https://gerrit.asterisk.org/#/c/490/1//COMMIT_MSG
Commit Message:

Line 7: res_rtp_asterisk.c: don't call dtls_perform_handshake while passive
I like capitalization.


Line 9: calling dtls_perform_handshake when dtls->dtls_setup ==
      : AST_RTP_DTLS_SETUP_PASSIVE may lead to seg-faults
      : within openssl.
I think this would be better as:

Performing a handshake when we are operating in
DTLS passive mode may cause a crash within OpenSSL.


https://gerrit.asterisk.org/#/c/490/1/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

Line 1630: 	if (&rtp->dtls && (rtp->dtls.dtls_setup != AST_RTP_DTLS_SETUP_PASSIVE)) {
The &rtp->dtls is not needed, it will always be true.


Line 1631: 		dtls_perform_handshake(instance, &rtp->dtls, 0);
dtls_perform_handshake also changes our internal state if DTLS is re-negotiated after already being established. I think this should still be done. As well - is OpenSSL fine with us not calling SSL_set_accept_state when re-negotiating? According to the documentation:

"When beginning a new handshake, the SSL engine must know whether it must call the connect (client) or accept (server) routines. Even though it may be clear from the method chosen, whether client or server mode was requested, the handshake routines must be explicitly set."

Of course, I don't know whether OpenSSL considers re-negotiation a new handshake or not. It probably does.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I156bd07323eb8c7f99f59fcfe560ce9c3ae60e36
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Stefan Engström <stefanen at kth.se>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-HasComments: Yes