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.

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)