Mercurial > hg > freeDiameter
changeset 924:877592751fee
Fix (tentative) for invalid handling of sessions fast creation/destruction as pointed by Yusuke Okura -- http://lists.freediameter.net/pipermail/help/2013-February/000584.html and http://lists.freediameter.net/pipermail/help/2013-February/000589.html -- Thank you very much
| author | Sebastien Decugis <sdecugis@freediameter.net> |
|---|---|
| date | Thu, 14 Feb 2013 17:52:57 +0100 |
| parents | 6a4d08e239bd |
| children | e5a09fab5ef3 51c15f98a965 |
| files | extensions/app_radgw/rgwx_auth.c extensions/app_radgw/rgwx_sip.c extensions/app_sip/registrationtermination.c extensions/test_app/ta_bench.c extensions/test_app/ta_cli.c extensions/test_sip/locationinfo.c extensions/test_sip/locationinfosl.c extensions/test_sip/serverassignment.c extensions/test_sip/userauthorization.c include/freeDiameter/libfdproto.h libfdcore/messages.c libfdproto/fdproto-internal.h libfdproto/messages.c libfdproto/sessions.c |
| diffstat | 14 files changed, 64 insertions(+), 91 deletions(-) [+] |
line wrap: on
line diff
--- a/extensions/app_radgw/rgwx_auth.c Thu Feb 14 15:58:55 2013 +0100 +++ b/extensions/app_radgw/rgwx_auth.c Thu Feb 14 17:52:57 2013 +0100 @@ -457,7 +457,7 @@ if (si_len) { /* We already have the Session-Id, just use it */ - CHECK_FCT( fd_sess_fromsid ( si, si_len, session, NULL) ); + CHECK_FCT( fd_sess_fromsid_msg ( si, si_len, session, NULL) ); } else { /* Create a new Session-Id string */ @@ -495,6 +495,7 @@ value.os.len = sess_strlen; CHECK_FCT( fd_msg_avp_setvalue ( avp, &value ) ); CHECK_FCT( fd_msg_avp_add ( *diam_fw, MSG_BRW_FIRST_CHILD, avp) ); + CHECK_FCT( fd_msg_sess_set( *diam_fw, *session) ); }
--- a/extensions/app_radgw/rgwx_sip.c Thu Feb 14 15:58:55 2013 +0100 +++ b/extensions/app_radgw/rgwx_sip.c Thu Feb 14 17:52:57 2013 +0100 @@ -479,6 +479,7 @@ value.os.len = sidlen; CHECK_FCT( fd_msg_avp_setvalue ( avp, &value ) ); CHECK_FCT( fd_msg_avp_add ( *diam_fw, MSG_BRW_FIRST_CHILD, avp) ); + CHECK_FCT( fd_msg_sess_set( *diam_fw, *session) ); /* If the RADIUS Access-Request message does not
--- a/extensions/app_sip/registrationtermination.c Thu Feb 14 15:58:55 2013 +0100 +++ b/extensions/app_sip/registrationtermination.c Thu Feb 14 17:52:57 2013 +0100 @@ -147,15 +147,7 @@ // Create a new session { #define APP_SIP_SID_OPT "app_sip" - CHECK_FCT( fd_sess_new( &sess, fd_g_config->cnf_diamid, fd_g_config->cnf_diamid_len, (os0_t)APP_SIP_SID_OPT, CONSTSTRLEN(APP_SIP_SID_OPT) )); - os0_t sid; - size_t sidlen; - CHECK_FCT( fd_sess_getsid ( sess, &sid, &sidlen )); - CHECK_FCT( fd_msg_avp_new ( sip_dict.Session_Id, 0, &avp )); - value.os.data = sid; - value.os.len = sidlen; - CHECK_FCT( fd_msg_avp_setvalue( avp, &value )); - CHECK_FCT( fd_msg_avp_add( message, MSG_BRW_FIRST_CHILD, avp )); + CHECK_FCT( fd_msg_new_session( message, (os0_t)APP_SIP_SID_OPT, CONSTSTRLEN(APP_SIP_SID_OPT) ) ); } //Add the Auth-Application-Id
--- a/extensions/test_app/ta_bench.c Thu Feb 14 15:58:55 2013 +0100 +++ b/extensions/test_app/ta_bench.c Thu Feb 14 17:52:57 2013 +0100 @@ -121,7 +121,6 @@ struct avp * avp; union avp_value val; struct ta_mess_info * mi = NULL; - struct session *sess = NULL; TRACE_DEBUG(FULL, "Creating a new message for sending."); @@ -130,7 +129,7 @@ /* Create a new session */ #define TEST_APP_SID_OPT "app_testb" - CHECK_FCT_DO( fd_sess_new( &sess, fd_g_config->cnf_diamid, fd_g_config->cnf_diamid_len, (os0_t)TEST_APP_SID_OPT, CONSTSTRLEN(TEST_APP_SID_OPT) ), goto out ); + CHECK_FCT_DO( fd_msg_new_session( req, (os0_t)TEST_APP_SID_OPT, CONSTSTRLEN(TEST_APP_SID_OPT) ), goto out ); /* Create the random value to store with the session */ mi = malloc(sizeof(struct ta_mess_info)); @@ -143,19 +142,6 @@ /* Now set all AVPs values */ - /* Session-Id */ - { - os0_t sid; - size_t sidlen; - CHECK_FCT_DO( fd_sess_getsid ( sess, &sid, &sidlen ), goto out ); - CHECK_FCT_DO( fd_msg_avp_new ( ta_sess_id, 0, &avp ), goto out ); - val.os.data = sid; - val.os.len = sidlen; - CHECK_FCT_DO( fd_msg_avp_setvalue( avp, &val ), goto out ); - CHECK_FCT_DO( fd_msg_avp_add( req, MSG_BRW_FIRST_CHILD, avp ), goto out ); - - } - /* Set the Destination-Realm AVP */ { CHECK_FCT_DO( fd_msg_avp_new ( ta_dest_realm, 0, &avp ), goto out );
--- a/extensions/test_app/ta_cli.c Thu Feb 14 15:58:55 2013 +0100 +++ b/extensions/test_app/ta_cli.c Thu Feb 14 17:52:57 2013 +0100 @@ -150,7 +150,8 @@ /* Create a new session */ #define TEST_APP_SID_OPT "app_test" - CHECK_FCT_DO( fd_sess_new( &sess, fd_g_config->cnf_diamid, fd_g_config->cnf_diamid_len, (os0_t)TEST_APP_SID_OPT, CONSTSTRLEN(TEST_APP_SID_OPT) ), goto out ); + CHECK_FCT_DO( fd_msg_new_session( req, (os0_t)TEST_APP_SID_OPT, CONSTSTRLEN(TEST_APP_SID_OPT) ), goto out ); + CHECK_FCT_DO( fd_msg_sess_get(fd_g_config->cnf_dict, req, &sess, NULL), goto out ); /* Create the random value to store with the session */ mi = malloc(sizeof(struct ta_mess_info)); @@ -163,19 +164,6 @@ /* Now set all AVPs values */ - /* Session-Id */ - { - os0_t sid; - size_t sidlen; - CHECK_FCT_DO( fd_sess_getsid ( sess, &sid, &sidlen ), goto out ); - CHECK_FCT_DO( fd_msg_avp_new ( ta_sess_id, 0, &avp ), goto out ); - val.os.data = sid; - val.os.len = sidlen; - CHECK_FCT_DO( fd_msg_avp_setvalue( avp, &val ), goto out ); - CHECK_FCT_DO( fd_msg_avp_add( req, MSG_BRW_FIRST_CHILD, avp ), goto out ); - - } - /* Set the Destination-Realm AVP */ { CHECK_FCT_DO( fd_msg_avp_new ( ta_dest_realm, 0, &avp ), goto out );
--- a/extensions/test_sip/locationinfo.c Thu Feb 14 15:58:55 2013 +0100 +++ b/extensions/test_sip/locationinfo.c Thu Feb 14 17:52:57 2013 +0100 @@ -41,7 +41,6 @@ struct dict_object * lir_model=NULL; struct msg * message=NULL; struct avp *avp=NULL; - struct session *sess=NULL; union avp_value value; //Fake values START @@ -61,15 +60,7 @@ // Create a new session { - CHECK_FCT( fd_sess_new( &sess, fd_g_config->cnf_diamid, fd_g_config->cnf_diamid_len, (os0_t)"appsip", 6 )); - os0_t sid; - size_t sidlen; - CHECK_FCT( fd_sess_getsid ( sess, &sid, &sidlen )); - CHECK_FCT( fd_msg_avp_new ( sip_dict.Session_Id, 0, &avp )); - value.os.data = sid; - value.os.len = sidlen; - CHECK_FCT( fd_msg_avp_setvalue( avp, &value )); - CHECK_FCT( fd_msg_avp_add( message, MSG_BRW_FIRST_CHILD, avp )); + CHECK_FCT( fd_msg_new_session( message, (os0_t)"appsip", CONSTSTRLEN("appsip") ) ); } //Add the Auth-Application-Id
--- a/extensions/test_sip/locationinfosl.c Thu Feb 14 15:58:55 2013 +0100 +++ b/extensions/test_sip/locationinfosl.c Thu Feb 14 17:52:57 2013 +0100 @@ -41,7 +41,6 @@ struct dict_object * lir_model=NULL; struct msg * message=NULL; struct avp *avp=NULL; - struct session *sess=NULL; union avp_value value; //Fake values START @@ -61,15 +60,7 @@ // Create a new session { - CHECK_FCT( fd_sess_new( &sess, fd_g_config->cnf_diamid, fd_g_config->cnf_diamid_len, (os0_t)"appsip", 6 )); - os0_t sid; - size_t sidlen; - CHECK_FCT( fd_sess_getsid ( sess, &sid, &sidlen )); - CHECK_FCT( fd_msg_avp_new ( sip_dict.Session_Id, 0, &avp )); - value.os.data = sid; - value.os.len = sidlen; - CHECK_FCT( fd_msg_avp_setvalue( avp, &value )); - CHECK_FCT( fd_msg_avp_add( message, MSG_BRW_FIRST_CHILD, avp )); + CHECK_FCT( fd_msg_new_session( message, (os0_t)"appsip", CONSTSTRLEN("appsip") ) ); } //Add the Auth-Application-Id
--- a/extensions/test_sip/serverassignment.c Thu Feb 14 15:58:55 2013 +0100 +++ b/extensions/test_sip/serverassignment.c Thu Feb 14 17:52:57 2013 +0100 @@ -41,7 +41,6 @@ struct dict_object * sar_model=NULL; struct msg * message=NULL; struct avp *avp=NULL; - struct session *sess=NULL; union avp_value value; //Fake values START @@ -70,15 +69,7 @@ // Create a new session { - CHECK_FCT( fd_sess_new( &sess, fd_g_config->cnf_diamid, fd_g_config->cnf_diamid_len, (os0_t)"appsip", 6 )); - os0_t sid; - size_t sidlen; - CHECK_FCT( fd_sess_getsid ( sess, &sid, &sidlen )); - CHECK_FCT( fd_msg_avp_new ( sip_dict.Session_Id, 0, &avp )); - value.os.data = sid; - value.os.len = sidlen; - CHECK_FCT( fd_msg_avp_setvalue( avp, &value )); - CHECK_FCT( fd_msg_avp_add( message, MSG_BRW_FIRST_CHILD, avp )); + CHECK_FCT( fd_msg_new_session( message, (os0_t)"appsip", CONSTSTRLEN("appsip") ) ); } //Add the Auth-Application-Id
--- a/extensions/test_sip/userauthorization.c Thu Feb 14 15:58:55 2013 +0100 +++ b/extensions/test_sip/userauthorization.c Thu Feb 14 17:52:57 2013 +0100 @@ -41,7 +41,6 @@ struct dict_object * uar_model=NULL; struct msg * message=NULL; struct avp *avp=NULL; - struct session *sess=NULL; union avp_value value; //Fake values START @@ -66,15 +65,7 @@ // Create a new session { - CHECK_FCT( fd_sess_new( &sess, fd_g_config->cnf_diamid, fd_g_config->cnf_diamid_len, (os0_t)"appsip", 6 )); - os0_t sid; - size_t sidlen; - CHECK_FCT( fd_sess_getsid ( sess, &sid, &sidlen )); - CHECK_FCT( fd_msg_avp_new ( sip_dict.Session_Id, 0, &avp )); - value.os.data = sid; - value.os.len = sidlen; - CHECK_FCT( fd_msg_avp_setvalue( avp, &value )); - CHECK_FCT( fd_msg_avp_add( message, MSG_BRW_FIRST_CHILD, avp )); + CHECK_FCT( fd_msg_new_session( message, (os0_t)"appsip", CONSTSTRLEN("appsip") ) ); } //Add the Auth-Application-Id
--- a/include/freeDiameter/libfdproto.h Thu Feb 14 15:58:55 2013 +0100 +++ b/include/freeDiameter/libfdproto.h Thu Feb 14 17:52:57 2013 +0100 @@ -1815,9 +1815,9 @@ * If diamId is NULL, the string is exactly the content of opt. * * RETURN VALUE: - * 0 : The session is created. + * 0 : The session is created, the initial msg refcount is 1. * EINVAL : A parameter is invalid. - * EALREADY : A session with the same name already exists (returned in *session) + * EALREADY : A session with the same name already exists (returned in *session), the msg refcount is increased. * ENOMEM : Not enough memory to complete the operation */ int fd_sess_new ( struct session ** session, DiamId_t diamid, size_t diamidlen, uint8_t * opt, size_t optlen ); @@ -2504,6 +2504,10 @@ */ int fd_msg_sess_get(struct dictionary * dict, struct msg * msg, struct session ** session, int * isnew); +/* This one is used by the libfdcore, you should use fd_msg_new_session rather than fd_sess_new, when possible */ +int fd_msg_sess_set(struct msg * msg, struct session * session); + + /***************************************/ /* Manage AVP values */ /***************************************/
--- a/libfdcore/messages.c Thu Feb 14 15:58:55 2013 +0100 +++ b/libfdcore/messages.c Thu Feb 14 17:52:57 2013 +0100 @@ -158,6 +158,9 @@ /* Add it to the message */ CHECK_FCT( fd_msg_avp_add( msg, MSG_BRW_FIRST_CHILD, avp ) ); + /* Save the session associated with the message */ + CHECK_FCT( fd_msg_sess_set( msg, sess) ); + /* Done! */ return 0; }
--- a/libfdproto/fdproto-internal.h Thu Feb 14 15:58:55 2013 +0100 +++ b/libfdproto/fdproto-internal.h Thu Feb 14 17:52:57 2013 +0100 @@ -69,6 +69,7 @@ int fd_sess_ref_msg ( struct session * session ); int fd_sess_reclaim_msg ( struct session ** session ); + /* For dump routines into string buffers */ #include <stdarg.h> static __inline__ int dump_init_str(char **outstr, size_t *offset, size_t *outlen)
--- a/libfdproto/messages.c Thu Feb 14 15:58:55 2013 +0100 +++ b/libfdproto/messages.c Thu Feb 14 17:52:57 2013 +0100 @@ -1215,6 +1215,20 @@ return 0; } +/* Associate a session with a message, use only when the session was just created */ +int fd_msg_sess_set(struct msg * msg, struct session * session) +{ + TRACE_ENTRY("%p %p", msg, session); + + /* Check we received valid parameters */ + CHECK_PARAMS( CHECK_MSG(msg) ); + CHECK_PARAMS( session ); + CHECK_PARAMS( msg->msg_sess == NULL ); + + msg->msg_sess = session; + return 0; +} + /* Retrieve the session of the message */ int fd_msg_sess_get(struct dictionary * dict, struct msg * msg, struct session ** session, int * new)
--- a/libfdproto/sessions.c Thu Feb 14 15:58:55 2013 +0100 +++ b/libfdproto/sessions.c Thu Feb 14 17:52:57 2013 +0100 @@ -359,7 +359,7 @@ -/* Create a new session object with the default timeout value, and link it */ +/* Create a new session object with the default timeout value, and link it. The refcount is increased by 1, whether the session existed or not */ int fd_sess_new ( struct session ** session, DiamId_t diamid, size_t diamidlen, uint8_t * opt, size_t optlen ) { os0_t sid = NULL; @@ -460,16 +460,22 @@ } ); fd_list_insert_before(li, &sess->chain_h); /* hash table */ + sess->msg_cnt++; } else { free(sid); + + CHECK_POSIX( pthread_mutex_lock(&(*session)->stlock) ); + (*session)->msg_cnt++; + CHECK_POSIX( pthread_mutex_unlock(&(*session)->stlock) ); + /* it was found: was it previously destroyed? */ if ((*session)->is_destroyed == 0) { ret = EALREADY; goto out; } else { /* the session was marked destroyed, let's re-activate it. */ - TODO("Re-creating a deleted session. Should investigate if this can lead to an issue... (need more feedback)"); sess = *session; + sess->is_destroyed = 0; /* update the expiry time */ CHECK_SYS_DO( clock_gettime(CLOCK_REALTIME, &sess->timeout), { ASSERT(0); } ); @@ -500,7 +506,7 @@ CHECK_POSIX_DO( pthread_mutex_unlock( &exp_lock ), { ASSERT(0); } ); /* if it fails, we might not pop the cleanup handler, but this should not happen -- and we'd have a serious problem otherwise */ out: - ; + ; pthread_cleanup_pop(0); CHECK_POSIX( pthread_mutex_unlock( H_LOCK(hash) ) ); @@ -511,8 +517,8 @@ return 0; } -/* Find or create a session */ -int fd_sess_fromsid ( uint8_t * sid, size_t len, struct session ** session, int * new) +/* Find or create a session -- the msg refcount is increased */ +int fd_sess_fromsid_msg ( uint8_t * sid, size_t len, struct session ** session, int * new) { int ret; @@ -801,17 +807,19 @@ } /* For the messages module */ -int fd_sess_fromsid_msg ( uint8_t * sid, size_t len, struct session ** session, int * new) +int fd_sess_fromsid ( uint8_t * sid, size_t len, struct session ** session, int * new) { TRACE_ENTRY("%p %zd %p %p", sid, len, session, new); CHECK_PARAMS( sid && len && session ); /* Get the session object */ - CHECK_FCT( fd_sess_fromsid ( sid, len, session, new) ); + CHECK_FCT( fd_sess_fromsid_msg ( sid, len, session, new) ); - /* Increase count */ - CHECK_FCT( fd_sess_ref_msg ( *session ) ); - + /* Decrease the refcount */ + CHECK_POSIX( pthread_mutex_lock(&(*session)->stlock) ); + (*session)->msg_cnt--; /* was increased in fd_sess_new */ + CHECK_POSIX( pthread_mutex_unlock(&(*session)->stlock) ); + /* Done */ return 0; } @@ -832,16 +840,27 @@ int fd_sess_reclaim_msg ( struct session ** session ) { int reclaim; + uint32_t hash; TRACE_ENTRY("%p", session); CHECK_PARAMS( session && VALIDATE_SI(*session) ); + /* Lock the hash line to avoid possibility that session is freed while we are reclaiming */ + hash = (*session)->hash; + CHECK_POSIX( pthread_mutex_lock( H_LOCK(hash)) ); + pthread_cleanup_push( fd_cleanup_mutex, H_LOCK(hash) ); + /* Update the msg refcount */ CHECK_POSIX( pthread_mutex_lock(&(*session)->stlock) ); reclaim = (*session)->msg_cnt; (*session)->msg_cnt = reclaim - 1; CHECK_POSIX( pthread_mutex_unlock(&(*session)->stlock) ); + /* Ok, now unlock the hash line */ + pthread_cleanup_pop( 0 ); + CHECK_POSIX( pthread_mutex_unlock( H_LOCK(hash) ) ); + + /* and reclaim if no message references the session anymore */ if (reclaim == 1) { CHECK_FCT(fd_sess_reclaim ( session )); } else {