added support for EGAIN while trying to receive data on gTLS session

This maps to bugzilla bug 83: http://bugzilla.adiscon.com/show_bug.cgi?id=83
This is the first test version, posted to user for repro of the problem.

It contains code to handle the case, HOWEVER, I have not been able to test it
in a scenario where a retry actually happens while receiving (I dont't get this
in my environment). So I assume it is buggy and will probably not work.

6 files changed, 143 insertions, 31 deletions

diff --git a/configure.ac b/configure.ac
index 4417e8f5..b3c3e625 100644
--- a/configure.ac
+++ b/configure.ac
@@ -2,7 +2,7 @@
 # Process this file with autoconf to produce a configure script.
 
 AC_PREREQ(2.61)
-AC_INIT([rsyslog],[3.19.8],[rsyslog@lists.adiscon.com])
+AC_INIT([rsyslog],[3.19.8-Test1],[rsyslog@lists.adiscon.com])
 AM_INIT_AUTOMAKE
 AC_CONFIG_SRCDIR([ChangeLog])
 AC_CONFIG_HEADERS([config.h])
@@ -13,7 +13,7 @@ AC_GNU_SOURCE
 AC_PROG_CC
 AM_PROG_CC_C_O
 if test "$GCC" = "yes"
-then CFLAGS="$CFLAGS -fdiagnostics-show-option -W -Wall -Wformat-security -Wshadow -Wcast-align -Wpointer-arith -Wmissing-format-attribute -g"
+then CFLAGS="$CFLAGS -W -Wall -Wformat-security -Wshadow -Wcast-align -Wpointer-arith -Wmissing-format-attribute -g"
 fi
 AC_DISABLE_STATIC
 AC_PROG_LIBTOOL
diff --git a/runtime/nsd_gtls.c b/runtime/nsd_gtls.c
index 932bab62..5646c2cf 100644
--- a/runtime/nsd_gtls.c
+++ b/runtime/nsd_gtls.c
@@ -471,6 +471,38 @@ uchar *gtlsStrerror(int error)
 }
 
 
+/* try to receive a record from the remote peer. This works with
+ * our own abstraction and handles local buffering and EAGAIN.
+ * See details on local buffering in Rcv(9 header-comment.
+ * This function MUST only be called when the local buffer is
+ * empty. Calling it otherwise will cause losss of current buffer
+ * data.
+ * rgerhards, 2008-06-24
+ */
+rsRetVal
+gtlsRecordRecv(nsd_gtls_t *pThis)
+{
+	ssize_t lenRcvd;
+	DEFiRet;
+
+	ISOBJ_TYPE_assert(pThis, nsd_gtls);
+	lenRcvd = gnutls_record_recv(pThis->sess, pThis->pszRcvBuf, sizeof(pThis->pszRcvBuf));
+	if(lenRcvd >= 0) {
+		pThis->lenRcvBuf = lenRcvd;
+		pThis->ptrRcvBuf = 0;
+	} else if(lenRcvd == GNUTLS_E_AGAIN || lenRcvd == GNUTLS_E_INTERRUPTED) {
+		pThis->rtryCall = gtlsRtry_recv;
+		dbgprintf("GnuTLS receive requires a retry (this most probably is OK and no error condition)\n");
+		ABORT_FINALIZE(RS_RET_RETRY);
+	} else {
+		int gnuRet; /* TODO: build a specific function for GnuTLS error reporting */
+		CHKgnutls(lenRcvd); /* this will abort the function */
+	}
+finalize_it:
+	RETiRet;
+}
+
+
 /* add our own certificate to the certificate set, so that the peer
  * can identify us. Please note that we try to use mutual authentication,
  * so we always add a cert, even if we are in the client role (later,
@@ -1096,6 +1128,7 @@ gtlsSetTransportPtr(nsd_gtls_t *pThis, int sock)
 BEGINobjConstruct(nsd_gtls) /* be sure to specify the object type also in END macro! */
 	iRet = nsd_ptcp.Construct(&pThis->pTcp);
 	pThis->bReportAuthErr = 1;
+//pThis->lenRcvBuf = -1; /* important: -1 means not read, 0 means connection close! */
 	CHKiRet(gtlsAddOurCert());
 finalize_it:
 ENDobjConstruct(nsd_gtls)
@@ -1116,6 +1149,10 @@ CODESTARTobjDestruct(nsd_gtls)
 		free(pThis->pszConnectHost);
 	}
 
+	if(pThis->pszRcvBuf == NULL) {
+		free(pThis->pszRcvBuf);
+	}
+
 	if(pThis->bOurCertIsInit)
 		gnutls_x509_crt_deinit(pThis->ourCert);
 	if(pThis->bOurKeyIsInit)
@@ -1364,18 +1401,31 @@ finalize_it:
 
 /* receive data from a tcp socket
  * The lenBuf parameter must contain the max buffer size on entry and contains
- * the number of octets read (or -1 in case of error) on exit. This function
+ * the number of octets read on exit. This function
  * never blocks, not even when called on a blocking socket. That is important
  * for client sockets, which are set to block during send, but should not
- * block when trying to read data. If *pLenBuf is -1, an error occured and
- * errno holds the exact error cause.
- * rgerhards, 2008-03-17
+ * block when trying to read data. -- rgerhards, 2008-03-17
+ * The function now follows the usual iRet calling sequence.
+ * With GnuTLS, we may need to restart a recv() system call. If so, we need
+ * to supply the SAME buffer on the retry. We can not assure this, as the
+ * caller is free to call us with any buffer location (and in current
+ * implementation, it is on the stack and extremely likely to change). To
+ * work-around this problem, we allocate a buffer ourselfs and always receive
+ * into that buffer. We pass data on to the caller only after we have received it.
+ * To save some space, we allocate that internal buffer only when it is actually 
+ * needed, which means when we reach this function for the first time. To keep
+ * the algorithm simple, we always supply data only from the internal buffer,
+ * even if it is a single byte. As we have a stream, the caller must be prepared
+ * to accept messages in any order, so we do not need to take care about this.
+ * Please note that the logic also forces us to do some "faking" in select(), as
+ * we must provide a fake "is ready for readign" status if we have data inside our
+ * buffer. -- rgerhards, 2008-06-23
  */
 static rsRetVal
 Rcv(nsd_t *pNsd, uchar *pBuf, ssize_t *pLenBuf)
 {
 	DEFiRet;
-	ssize_t lenRcvd;
+	ssize_t iBytesCopy; /* how many bytes are to be copied to the client buffer? */
 	nsd_gtls_t *pThis = (nsd_gtls_t*) pNsd;
 	ISOBJ_TYPE_assert(pThis, nsd_gtls);
 
@@ -1387,21 +1437,43 @@ Rcv(nsd_t *pNsd, uchar *pBuf, ssize_t *pLenBuf)
 		FINALIZE;
 	}
 
-	/* in TLS mode now */
-	lenRcvd = gnutls_record_recv(pThis->sess, pBuf, *pLenBuf);
-	if(lenRcvd < 0) {
-		if(lenRcvd == GNUTLS_E_AGAIN || lenRcvd == GNUTLS_E_INTERRUPTED) {
-			pThis->rtryCall = gtlsRtry_recv;
-			dbgprintf("GnuTLS receive requires a retry (this most probably is OK and no error condition)\n");
-			iRet = RS_RET_RETRY;
-		} else {
-			int gnuRet; /* TODO: build a specific function for GnuTLS error reporting */
-			*pLenBuf = -1;
-			CHKgnutls(lenRcvd); /* this will abort the function */
-		}
+	/* --- in TLS mode now --- */
+
+	/* Buffer logic applies only if we are in TLS mode. Here we 
+	 * assume that we will switch from plain to TLS, but never back. This
+	 * assumption may be unsafe, but it is the model for the time being and I
+	 * do not see any valid reason why we should switch back to plain TCP after
+	 * we were in TLS mode. However, in that case we may lose something that
+	 * is already in the receive buffer ... risk accepted. -- rgerhards, 2008-06-23
+	 */
+
+	if(pThis->pszRcvBuf == NULL) {
+		/* we have no buffer, so we need to malloc one */
+		CHKmalloc(pThis->pszRcvBuf = malloc(NSD_GTLS_MAX_RCVBUF));
+		pThis->lenRcvBuf = -1;
+	}
+
+	/* now check if we have something in our buffer. If so, we satisfy
+	 * the request from buffer contents.
+	 */
+	if(pThis->lenRcvBuf == 0) { /* EOS */
+		*pLenBuf = 0;
+		FINALIZE;
+	} else if(pThis->lenRcvBuf == -1) { /* no data present, must read */
+		CHKiRet(gtlsRecordRecv(pThis));
+	}
+
+	/* if we reach this point, data is present in the buffer and must be copied */
+	iBytesCopy = pThis->lenRcvBuf - pThis->ptrRcvBuf;
+	if(iBytesCopy > *pLenBuf) {
+		iBytesCopy = *pLenBuf;
+	} else {
+		pThis->lenRcvBuf = -1; /* buffer will be emptied below */
 	}
 
-	*pLenBuf = lenRcvd;
+	memcpy(pBuf, pThis->pszRcvBuf + pThis->ptrRcvBuf, iBytesCopy);
+	pThis->ptrRcvBuf += iBytesCopy;
+	*pLenBuf = iBytesCopy;
 
 finalize_it:
 	RETiRet;
diff --git a/runtime/nsd_gtls.h b/runtime/nsd_gtls.h
index d6821dce..52eea8ee 100644
--- a/runtime/nsd_gtls.h
+++ b/runtime/nsd_gtls.h
@@ -26,6 +26,8 @@
 
 #include "nsd.h"
 
+#define NSD_GTLS_MAX_RCVBUF 8 * 1024 /* max size of buffer for message reception */
+
 typedef enum {
 	gtlsRtry_None = 0,	/**< no call needs to be retried */
 	gtlsRtry_handshake = 1,
@@ -60,6 +62,9 @@ struct nsd_gtls_s {
 	gnutls_x509_privkey ourKey;	/**< our private key, if in client mode (unused in server mode) */
 	short	bOurCertIsInit;	/**< 1 if our certificate is initialized and must be deinit on destruction */
 	short	bOurKeyIsInit;	/**< 1 if our private key is initialized and must be deinit on destruction */
+	char *pszRcvBuf;
+	int lenRcvBuf;		/**< -1: empty, 0: connection closed, 1..NSD_GTLS_MAX_RCVBUF-1: data of that size present */
+	int ptrRcvBuf;		/**< offset for next recv operation if 0 < lenRcvBuf < NSD_GTLS_MAX_RCVBUF */
 };
 
 /* interface is defined in nsd.h, we just implement it! */
@@ -70,6 +75,16 @@ PROTOTYPEObj(nsd_gtls);
 /* some prototypes for things used by our nsdsel_gtls helper class */
 uchar *gtlsStrerror(int error);
 rsRetVal gtlsChkPeerAuth(nsd_gtls_t *pThis);
+rsRetVal gtlsRecordRecv(nsd_gtls_t *pThis);
+static inline rsRetVal gtlsHasRcvInBuffer(nsd_gtls_t *pThis) {
+	/* we have a valid receive buffer one such is allocated and 
+	 * NOT exhausted!
+	 */
+	dbgprintf("hasRcvInBuffer on nsd %p: pszRcvBuf %p, lenRcvBuf %d\n", pThis,
+		pThis->pszRcvBuf, pThis->lenRcvBuf);
+	return(pThis->pszRcvBuf != NULL && pThis->lenRcvBuf != -1);
+	}
+
 
 /* the name of our library binary */
 #define LM_NSD_GTLS_FILENAME "lmnsd_gtls"
diff --git a/runtime/nsdsel_gtls.c b/runtime/nsdsel_gtls.c
index 7b359950..368c4df3 100644
--- a/runtime/nsdsel_gtls.c
+++ b/runtime/nsdsel_gtls.c
@@ -74,6 +74,10 @@ Add(nsdsel_t *pNsdsel, nsd_t *pNsd, nsdsel_waitOp_t waitOp)
 	ISOBJ_TYPE_assert(pThis, nsdsel_gtls);
 	ISOBJ_TYPE_assert(pNsdGTLS, nsd_gtls);
 	if(pNsdGTLS->iMode == 1) {
+		if(waitOp == NSDSEL_RD && gtlsHasRcvInBuffer(pNsdGTLS)) {
+			++pThis->iBufferRcvReady;
+			FINALIZE;
+		}
 		if(pNsdGTLS->rtryCall != gtlsRtry_None) {
 			if(gnutls_record_get_direction(pNsdGTLS->sess) == 0) {
 				CHKiRet(nsdsel_ptcp.Add(pThis->pTcp, pNsdGTLS->pTcp, NSDSEL_RD));
@@ -87,6 +91,7 @@ Add(nsdsel_t *pNsdsel, nsd_t *pNsd, nsdsel_waitOp_t waitOp)
 	/* if we reach this point, we need no special handling */
 	CHKiRet(nsdsel_ptcp.Add(pThis->pTcp, pNsdGTLS->pTcp, waitOp));
 
+RUNLOG_VAR("%d", pThis->iBufferRcvReady);
 finalize_it:
 	RETiRet;
 }
@@ -102,7 +107,14 @@ Select(nsdsel_t *pNsdsel, int *piNumReady)
 	nsdsel_gtls_t *pThis = (nsdsel_gtls_t*) pNsdsel;
 
 	ISOBJ_TYPE_assert(pThis, nsdsel_gtls);
-	iRet = nsdsel_ptcp.Select(pThis->pTcp, piNumReady);
+RUNLOG_VAR("%d", pThis->iBufferRcvReady);
+	if(pThis->iBufferRcvReady > 0) {
+		/* we still have data ready! */
+		*piNumReady = pThis->iBufferRcvReady;
+	} else {
+		iRet = nsdsel_ptcp.Select(pThis->pTcp, piNumReady);
+	}
+
 	RETiRet;
 }
 
@@ -134,6 +146,10 @@ doRetry(nsd_gtls_t *pNsd)
 				CHKiRet(gtlsChkPeerAuth(pNsd));
 			}
 			break;
+		case gtlsRtry_recv:
+			dbgprintf("retrying gtls recv, nsd: %p\n", pNsd);
+			CHKiRet(gtlsRecordRecv(pNsd));
+			break;
 		default:
 			assert(0); /* this shall not happen! */
 			dbgprintf("ERROR: pNsd->rtryCall invalid in nsdsel_gtls.c:%d\n", __LINE__);
@@ -172,6 +188,10 @@ IsReady(nsdsel_t *pNsdsel, nsd_t *pNsd, nsdsel_waitOp_t waitOp, int *pbIsReady)
 	ISOBJ_TYPE_assert(pThis, nsdsel_gtls);
 	ISOBJ_TYPE_assert(pNsdGTLS, nsd_gtls);
 	if(pNsdGTLS->iMode == 1) {
+		if(waitOp == NSDSEL_RD && gtlsHasRcvInBuffer(pNsdGTLS)) {
+			*pbIsReady = 1;
+			FINALIZE;
+		}
 		if(pNsdGTLS->rtryCall != gtlsRtry_None) {
 			CHKiRet(doRetry(pNsdGTLS));
 			/* we used this up for our own internal processing, so the socket
diff --git a/runtime/nsdsel_gtls.h b/runtime/nsdsel_gtls.h
index 7c2df684..709ccd03 100644
--- a/runtime/nsdsel_gtls.h
+++ b/runtime/nsdsel_gtls.h
@@ -31,6 +31,7 @@ typedef nsdsel_if_t nsdsel_gtls_if_t; /* we just *implement* this interface */
 struct nsdsel_gtls_s {
 	BEGINobjInstance;	/* Data to implement generic object - MUST be the first data element! */
 	nsdsel_t *pTcp;		/* our aggregated ptcp sel handler (which does almost everything) */
+	int iBufferRcvReady;	/* number of descriptiors where no RD select is needed because we have data in buf */
 };
 
 /* interface is defined in nsd.h, we just implement it! */
diff --git a/tcpsrv.c b/tcpsrv.c
index 94bcaaa7..b9f45723 100644
--- a/tcpsrv.c
+++ b/tcpsrv.c
@@ -406,7 +406,6 @@ Run(tcpsrv_t *pThis)
 	int bIsReady;
 	tcps_sess_t *pNewSess;
 	nssel_t *pSel;
-	int state;
 	ssize_t iRcvd;
 
 	ISOBJ_TYPE_assert(pThis, tcpsrv);
@@ -457,18 +456,15 @@ Run(tcpsrv_t *pThis)
 
 				/* Receive message */
 				iRet = pThis->pRcvData(pThis->pSessions[iTCPSess], buf, sizeof(buf), &iRcvd);
-				if(iRet == RS_RET_CLOSED) {
+				switch(iRet) {
+				case RS_RET_CLOSED:
 					pThis->pOnRegularClose(pThis->pSessions[iTCPSess]);
 					tcps_sess.Destruct(&pThis->pSessions[iTCPSess]);
-				} else if(iRet == RS_RET_RETRY) {
+					break;
+				case RS_RET_RETRY:
 					/* we simply ignore retry - this is not an error, but we also have not received anything */
-				} else if(iRet == RS_RET_OK) {
-					errno = 0;
-					errmsg.LogError(NO_ERRCODE, "netstream session %p will be closed due to error\n",
-							pThis->pSessions[iTCPSess]->pStrm);
-					pThis->pOnErrClose(pThis->pSessions[iTCPSess]);
-					tcps_sess.Destruct(&pThis->pSessions[iTCPSess]);
-				} else {
+					break;
+				case RS_RET_OK:
 					/* valid data received, process it! */
 					if(tcps_sess.DataRcvd(pThis->pSessions[iTCPSess], buf, iRcvd) != RS_RET_OK) {
 						/* in this case, something went awfully wrong.
@@ -479,6 +475,14 @@ Run(tcpsrv_t *pThis)
 						pThis->pOnErrClose(pThis->pSessions[iTCPSess]);
 						tcps_sess.Destruct(&pThis->pSessions[iTCPSess]);
 					}
+					break;
+				default:
+					errno = 0;
+					errmsg.LogError(NO_ERRCODE, "netstream session %p will be closed due to error\n",
+							pThis->pSessions[iTCPSess]->pStrm);
+					pThis->pOnErrClose(pThis->pSessions[iTCPSess]);
+					tcps_sess.Destruct(&pThis->pSessions[iTCPSess]);
+					break;
 				}
 				--nfds; /* indicate we have processed one */
 			}