From 17f36a76cbf15e088d2973ed5608f93e09827f8a Mon Sep 17 00:00:00 2001 From: Rainer Gerhards Date: Thu, 13 Mar 2008 11:04:33 +0000 Subject: bugfix: imgssapi segfaulted under some conditions; this fix is actually not just a fix but a change in the object model. Thanks to varmojfekoj for providing the bug report, an initial fix and lots of good discussion that lead to where we finally ended up. --- ChangeLog | 4 ++++ modules.c | 5 ----- plugins/imgssapi/imgssapi.c | 37 ++++++++++--------------------------- plugins/imtcp/imtcp.c | 10 ---------- tcps_sess.c | 11 +++++++++-- tcpsrv.c | 35 ++++++++++++++++++++++------------- tcpsrv.h | 4 ++-- 7 files changed, 47 insertions(+), 59 deletions(-) diff --git a/ChangeLog b/ChangeLog index 9d196446..5351cabc 100644 --- a/ChangeLog +++ b/ChangeLog @@ -11,6 +11,10 @@ Version 3.12.2 (rgerhards), 2008-03-?? to Johnny Tan for an excellent bug report - implemented dynamic module unload capability (not visible to end user) - some more internal cleanup +- bugfix: imgssapi segfaulted under some conditions; this fix is actually + not just a fix but a change in the object model. Thanks to varmojfekoj + for providing the bug report, an initial fix and lots of good discussion + that lead to where we finally ended up. --------------------------------------------------------------------------- Version 3.12.1 (rgerhards), 2008-03-06 - added library plugins, which can be automatically loaded diff --git a/modules.c b/modules.c index 70945cae..d5032772 100644 --- a/modules.c +++ b/modules.c @@ -484,12 +484,7 @@ modUnlinkAndDestroy(modInfo_t *pThis) /* finally, we are ready for the module to go away... */ dbgprintf("Unloading module %s\n", modGetName(pThis)); CHKiRet(modPrepareUnload(pThis)); -modInfo_t *prev, *next; -char *name = strdup(pThis->pszName); -prev = pThis->pPrev; next = pThis->pNext; moduleDestruct(pThis); -dbgprintf("end unload, pThis %p (%s), prev %p, next %p\n", pThis, name, prev, next); -free(name); finalize_it: RETiRet; diff --git a/plugins/imgssapi/imgssapi.c b/plugins/imgssapi/imgssapi.c index f551d616..8d406a7e 100644 --- a/plugins/imgssapi/imgssapi.c +++ b/plugins/imgssapi/imgssapi.c @@ -68,8 +68,8 @@ static rsRetVal addGSSListener(void __attribute__((unused)) *pVal, uchar *pNewVa static int TCPSessGSSInit(void); static void TCPSessGSSClose(tcps_sess_t* pSess); static int TCPSessGSSRecv(tcps_sess_t *pSess, void *buf, size_t buf_len); -static rsRetVal onSessAccept(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd); -static rsRetVal OnSessAcceptGSS(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd); +static rsRetVal onSessAccept(tcpsrv_t *pThis, tcps_sess_t *ppSess); +static rsRetVal OnSessAcceptGSS(tcpsrv_t *pThis, tcps_sess_t *ppSess); /* static data */ DEF_IMOD_STATIC_DATA @@ -184,7 +184,7 @@ isPermittedHost(struct sockaddr *addr, char *fromHostFQDN, void *pUsrSrv, void*p } static rsRetVal -onSessAccept(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) +onSessAccept(tcpsrv_t *pThis, tcps_sess_t *pSess) { DEFiRet; gsssrv_t *pGSrv; @@ -192,10 +192,8 @@ onSessAccept(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) pGSrv = (gsssrv_t*) pThis->pUsr; if(pGSrv->allowedMethods & ALLOWEDMETHOD_GSS) { - iRet = OnSessAcceptGSS(pThis, ppSess, fd); - } else { - iRet = tcpsrv.SessAccept(pThis, ppSess, fd); - } + iRet = OnSessAcceptGSS(pThis, pSess); + } RETiRet; } @@ -246,7 +244,7 @@ onErrClose(tcps_sess_t *pSess) static int* doOpenLstnSocks(tcpsrv_t *pSrv) { - int *pRet; + int *pRet = NULL; gsssrv_t *pGSrv; ISOBJ_TYPE_assert(pSrv, tcpsrv); @@ -363,11 +361,10 @@ static int TCPSessGSSInit(void) /* returns 0 if all went OK, -1 if it failed - * Calls tcpsrv's SessAccept() and then tries to guess if the connection uses - * gssapi. + * tries to guess if the connection uses gssapi. */ static rsRetVal -OnSessAcceptGSS(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) +OnSessAcceptGSS(tcpsrv_t *pThis, tcps_sess_t *pSess) { DEFiRet; gss_buffer_desc send_tok, recv_tok; @@ -378,13 +375,9 @@ OnSessAcceptGSS(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) int fdSess; char allowedMethods; gsssrv_t *pGSrv; - tcps_sess_t *pSess; gss_sess_t *pGSess; - assert(ppSess != NULL); - - /* first do the usual coding */ - CHKiRet(tcpsrv.SessAccept(pThis, &pSess, fd)); + assert(pSess != NULL); pGSrv = (gsssrv_t*) pThis->pUsr; pGSess = (gss_sess_t*) pSess->pUsr; @@ -413,7 +406,6 @@ OnSessAcceptGSS(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) } while (ret < 0 && errno == EINTR); if (ret < 0) { errmsg.LogError(NO_ERRCODE, "TCP session %p will be closed, error ignored\n", pSess); - tcps_sess.Close(pSess); ABORT_FINALIZE(RS_RET_ERR); // TODO: define good error codes } else if (ret == 0) { dbgprintf("GSS-API Reverting to plain TCP\n"); @@ -429,7 +421,6 @@ OnSessAcceptGSS(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) dbgprintf("GSS-API Connection closed by peer\n"); else errmsg.LogError(NO_ERRCODE, "TCP(GSS) session %p will be closed, error ignored\n", pSess); - tcps_sess.Close(pSess); ABORT_FINALIZE(RS_RET_ERR); // TODO: define good error codes } @@ -450,7 +441,6 @@ OnSessAcceptGSS(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) dbgprintf("GSS-API Connection closed by peer\n"); else errmsg.LogError(NO_ERRCODE, "TCP session %p will be closed, error ignored\n", pSess); - tcps_sess.Close(pSess); ABORT_FINALIZE(RS_RET_ERR); // TODO: define good error codes } } @@ -473,7 +463,6 @@ OnSessAcceptGSS(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) do { if (gssutil.recv_token(fdSess, &recv_tok) <= 0) { errmsg.LogError(NO_ERRCODE, "TCP session %p will be closed, error ignored\n", pSess); - tcps_sess.Close(pSess); ABORT_FINALIZE(RS_RET_ERR); // TODO: define good error codes } maj_stat = gss_accept_sec_context(&acc_sec_min_stat, context, gss_server_creds, @@ -493,16 +482,13 @@ OnSessAcceptGSS(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) dbgprintf("tcp session socket with new data: #%d\n", fdSess); if(tcps_sess.DataRcvd(pSess, buf, ret) == 0) { errmsg.LogError(NO_ERRCODE, "Tearing down TCP Session %p - see " - "previous messages for reason(s)\n", - pSess); - tcps_sess.Close(pSess); + "previous messages for reason(s)\n", pSess); ABORT_FINALIZE(RS_RET_ERR); // TODO: define good error codes } pGSess->allowedMethods = ALLOWEDMETHOD_TCP; ABORT_FINALIZE(RS_RET_OK); // TODO: define good error codes } gssutil.display_status("accepting context", maj_stat, acc_sec_min_stat); - tcps_sess.Close(pSess); ABORT_FINALIZE(RS_RET_ERR); // TODO: define good error codes } if (send_tok.length != 0) { @@ -511,7 +497,6 @@ OnSessAcceptGSS(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) errmsg.LogError(NO_ERRCODE, "TCP session %p will be closed, error ignored\n", pSess); if (*context != GSS_C_NO_CONTEXT) gss_delete_sec_context(&min_stat, context, GSS_C_NO_BUFFER); - tcps_sess.Close(pSess); ABORT_FINALIZE(RS_RET_ERR); // TODO: define good error codes } gss_release_buffer(&min_stat, &send_tok); @@ -531,8 +516,6 @@ OnSessAcceptGSS(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) pGSess->allowedMethods = ALLOWEDMETHOD_GSS; } - *ppSess = pSess; - finalize_it: RETiRet; } diff --git a/plugins/imtcp/imtcp.c b/plugins/imtcp/imtcp.c index ed0131b8..7baa95f2 100644 --- a/plugins/imtcp/imtcp.c +++ b/plugins/imtcp/imtcp.c @@ -70,15 +70,6 @@ isPermittedHost(struct sockaddr *addr, char *fromHostFQDN, void __attribute__((u } -static rsRetVal -onSessAccept(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) -{ - DEFiRet; - iRet = tcpsrv.SessAccept(pThis, ppSess, fd); - RETiRet; -} - - static int* doOpenLstnSocks(tcpsrv_t *pSrv) { @@ -132,7 +123,6 @@ static rsRetVal addTCPListener(void __attribute__((unused)) *pVal, uchar *pNewVa CHKiRet(tcpsrv.SetCBIsPermittedHost(pOurTcpsrv, isPermittedHost)); CHKiRet(tcpsrv.SetCBRcvData(pOurTcpsrv, doRcvData)); CHKiRet(tcpsrv.SetCBOpenLstnSocks(pOurTcpsrv, doOpenLstnSocks)); - CHKiRet(tcpsrv.SetCBOnSessAccept(pOurTcpsrv, onSessAccept)); CHKiRet(tcpsrv.SetCBOnRegularClose(pOurTcpsrv, onRegularClose)); CHKiRet(tcpsrv.SetCBOnErrClose(pOurTcpsrv, onErrClose)); tcpsrv.configureTCPListen(pOurTcpsrv, (char *) pNewVal); diff --git a/tcps_sess.c b/tcps_sess.c index 74de3c28..c8ac3a37 100644 --- a/tcps_sess.c +++ b/tcps_sess.c @@ -57,6 +57,10 @@ DEFobjStaticHelpers DEFobjCurrIf(errmsg) +/* forward definitions */ +static rsRetVal Close(tcps_sess_t *pThis); + + /* Standard-Constructor */ BEGINobjConstruct(tcps_sess) /* be sure to specify the object type also in END macro! */ @@ -86,6 +90,9 @@ finalize_it: /* destructor for the tcps_sess object */ BEGINobjDestruct(tcps_sess) /* be sure to specify the object type also in END and CODESTART macros! */ CODESTARTobjDestruct(tcps_sess) + if(pThis->sock != -1) + Close(pThis); + if(pThis->pSrv->pOnSessDestruct != NULL) { pThis->pSrv->pOnSessDestruct(&pThis->pUsr); } @@ -211,8 +218,8 @@ finalize_it: } -/* Closes a TCP session and marks its slot in the session - * table as unused. No attention is paid to the return code +/* Closes a TCP session + * No attention is paid to the return code * of close, so potential-double closes are not detected. */ static rsRetVal diff --git a/tcpsrv.c b/tcpsrv.c index 0f87165d..4030b206 100644 --- a/tcpsrv.c +++ b/tcpsrv.c @@ -422,13 +422,16 @@ static int *create_tcp_socket(tcpsrv_t *pThis) * connection is immediately dropped. * ppSess has a pointer to the newly created session, if it succeds. * If it does not succeed, no session is created and ppSess is - * undefined. -- rgerhards, 2008-03-02 + * undefined. If the user has provided an OnSessAccept Callback, + * this one is executed immediately after creation of the + * session object, so that it can do its own initializatin. + * rgerhards, 2008-03-02 */ static rsRetVal SessAccept(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) { DEFiRet; - + tcps_sess_t *pSess; int newConn; int iSess = -1; struct sockaddr_storage addr; @@ -455,13 +458,10 @@ SessAccept(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) //was: return -1; } else { /* we found a free spot and can construct our session object */ - CHKiRet(tcps_sess.Construct(ppSess)); - CHKiRet(tcps_sess.SetTcpsrv(*ppSess, pThis)); + CHKiRet(tcps_sess.Construct(&pSess)); + CHKiRet(tcps_sess.SetTcpsrv(pSess, pThis)); } - - pThis->pSessions[iSess] = *ppSess; - /* OK, we have a "good" index... */ /* get the host name */ if(net.cvthname(&addr, fromHost, fromHostFQDN) != RS_RET_OK) { @@ -494,10 +494,18 @@ SessAccept(tcpsrv_t *pThis, tcps_sess_t **ppSess, int fd) /* OK, we have an allowed sender, so let's continue, what * means we can finally fill in the session object. */ - CHKiRet(tcps_sess.SetHost(pThis->pSessions[iSess], fromHost)); - CHKiRet(tcps_sess.SetSock(pThis->pSessions[iSess], newConn)); - CHKiRet(tcps_sess.SetMsgIdx(pThis->pSessions[iSess], 0)); - CHKiRet(tcps_sess.ConstructFinalize(pThis->pSessions[iSess])); + CHKiRet(tcps_sess.SetHost(pSess, fromHost)); + CHKiRet(tcps_sess.SetSock(pSess, newConn)); + CHKiRet(tcps_sess.SetMsgIdx(pSess, 0)); + CHKiRet(tcps_sess.ConstructFinalize(pSess)); + + /* check if we need to call our callback */ + if(pThis->pOnSessAccept != NULL) { + CHKiRet(pThis->pOnSessAccept(pThis, pSess)); + } + + *ppSess = pSess; + pThis->pSessions[iSess] = pSess; finalize_it: if(iRet != RS_RET_OK) { @@ -578,7 +586,8 @@ Run(tcpsrv_t *pThis) for (i = 0; i < *pThis->pSocksLstn; i++) { if (FD_ISSET(pThis->pSocksLstn[i+1], &readfds)) { dbgprintf("New connect on TCP inetd socket: #%d\n", pThis->pSocksLstn[i+1]); - pThis->pOnSessAccept(pThis, &pNewSess, pThis->pSocksLstn[i+1]); + //pThis->pOnSessAccept(pThis, &pNewSess, pThis->pSocksLstn[i+1]); + SessAccept(pThis, &pNewSess, pThis->pSocksLstn[i+1]); --nfds; /* indicate we have processed one */ } } @@ -687,7 +696,7 @@ SetCBOnListenDeinit(tcpsrv_t *pThis, int (*pCB)(void*)) } static rsRetVal -SetCBOnSessAccept(tcpsrv_t *pThis, rsRetVal (*pCB)(tcpsrv_t*, tcps_sess_t**, int)) +SetCBOnSessAccept(tcpsrv_t *pThis, rsRetVal (*pCB)(tcpsrv_t*, tcps_sess_t*)) { DEFiRet; pThis->pOnSessAccept = pCB; diff --git a/tcpsrv.h b/tcpsrv.h index fb9f4d21..20a7ea25 100644 --- a/tcpsrv.h +++ b/tcpsrv.h @@ -42,7 +42,7 @@ typedef struct tcpsrv_s { rsRetVal (*pOnRegularClose)(tcps_sess_t *pSess); rsRetVal (*pOnErrClose)(tcps_sess_t *pSess); /* session specific callbacks */ - rsRetVal (*pOnSessAccept)(struct tcpsrv_s *, tcps_sess_t**, int fd); + rsRetVal (*pOnSessAccept)(struct tcpsrv_s *, tcps_sess_t*); rsRetVal (*OnSessConstructFinalize)(void*); rsRetVal (*pOnSessDestruct)(void*); } tcpsrv_t; @@ -68,7 +68,7 @@ BEGINinterface(tcpsrv) /* name must also be changed in ENDinterface macro! */ rsRetVal (*SetCBOnRegularClose)(tcpsrv_t*, rsRetVal (*) (tcps_sess_t*)); rsRetVal (*SetCBOnErrClose)(tcpsrv_t*, rsRetVal (*) (tcps_sess_t*)); /* session specifics */ - rsRetVal (*SetCBOnSessAccept)(tcpsrv_t*, rsRetVal (*) (tcpsrv_t*, tcps_sess_t**, int)); + rsRetVal (*SetCBOnSessAccept)(tcpsrv_t*, rsRetVal (*) (tcpsrv_t*, tcps_sess_t*)); rsRetVal (*SetCBOnSessDestruct)(tcpsrv_t*, rsRetVal (*) (void*)); rsRetVal (*SetCBOnSessConstructFinalize)(tcpsrv_t*, rsRetVal (*) (void*)); ENDinterface(tcpsrv) -- cgit