On 06/25/2011 12:14 AM, Steve Singer wrote:
> I'm not a libpq guru but I've given your patch a look over.
>
Thanks for the review!
I have since simplified the patch to assume that partial SSL writes are
disabled -- according to SSL_write(3) this is the default behaviour.
Now the SSL retry buffer only holds the data to be retried, the
remainder is moved to the new outBuffer.
> -The patch doesn't compile with non-ssl builds, the debug at the bottom
> of PQSendSome isn't in an #ifdef
>
Ack, there was another one in pqFlush. Fixed that.
> -I don't think your handling the return code properly. Consider this case.
>
> pqSendSome(some data)
> sslRetryBuf = some Data
> return 1
> pqSendSome(more data)
> it sends all of 'some data'
> returns 0
>
> I think 1 should be returned because all of 'more data' still needs to
> be sent. I think returning a 0 will break PQsetnonblocking if you call
> it when there is data in both sslRetryBuf and outputBuffer.
Hmm, I thought I thought about that. There was a check in the original
patch: "if (conn->sslRetryBytes || (conn->outCount - remaining) > 0)"
So if the SSL retry buffer was emptied it would return 1 if there was
something left in the regular output buffer. Or did I miss something
there?
> We might even want to try sending the data in outputBuffer after we've
> sent all the data sitting in sslRetryBuf.
>
IMHO that'd add unnecessary complexity to the pqSendSome. As this only
happens in non-blocking mode the caller should be well prepared to
handle the retry.
> If you close the connection with an outstanding sslRetryBuf you need to
> free it.
Fixed.
New version of the patch attached.
regards,
Martin
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 9e4807e..8dc0226 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2792,6 +2792,10 @@ freePGconn(PGconn *conn)
if (conn->gsslib)
free(conn->gsslib);
#endif
+#if defined(USE_SSL)
+ if (conn->sslOutBufPtr)
+ free(conn->sslOutBufPtr);
+#endif
/* Note that conn->Pfdebug is not ours to close or free */
if (conn->last_query)
free(conn->last_query);
diff --git a/src/interfaces/libpq/fe-misc.c b/src/interfaces/libpq/fe-misc.c
index 17dde4a..d0c7812 100644
--- a/src/interfaces/libpq/fe-misc.c
+++ b/src/interfaces/libpq/fe-misc.c
@@ -781,6 +781,8 @@ pqSendSome(PGconn *conn, int len)
char *ptr = conn->outBuffer;
int remaining = conn->outCount;
int result = 0;
+ int sent = 0;
+ bool shiftOutBuffer = true;
if (conn->sock < 0)
{
@@ -789,23 +791,34 @@ pqSendSome(PGconn *conn, int len)
return -1;
}
+#ifdef USE_SSL
+ if (conn->sslRetryPos)
+ {
+ /*
+ * We have some leftovers from previous SSL write attempts, these need
+ * to be sent before the regular output buffer.
+ */
+ len = remaining = conn->sslRetrySize;
+ ptr = conn->sslRetryPos;
+ shiftOutBuffer = false;
+ }
+#endif
+
/* while there's still data to send */
while (len > 0)
{
- int sent;
char sebuf[256];
+ int write_size = len;
-#ifndef WIN32
- sent = pqsecure_write(conn, ptr, len);
-#else
-
+#ifdef WIN32
/*
* Windows can fail on large sends, per KB article Q201213. The
* failure-point appears to be different in different versions of
* Windows, but 64k should always be safe.
*/
- sent = pqsecure_write(conn, ptr, Min(len, 65536));
+ write_size = Min(len, 65536);
#endif
+ sent = pqsecure_write(conn, ptr, write_size);
if (sent < 0)
{
@@ -857,6 +870,38 @@ pqSendSome(PGconn *conn, int len)
return -1;
}
}
+#ifdef USE_SSL
+ else if (sent == 0 && conn->ssl && !conn->sslRetryPos)
+ {
+ /*
+ * pqsecure_write indicates that a SSL write needs to be retried.
+ * With non-blocking connections we need to ensure that the buffer
+ * passed to the SSL_write() retry is the the exact same buffer as
+ * in previous write -- see SSL_write(3SSL) for more on this. For
+ * this we need to save the the original buffer and allocate a new
+ * buffer for outgoing data.
+ */
+ conn->sslOutBufPtr = conn->outBuffer;
+ conn->sslRetryPos = ptr;
+ conn->sslRetrySize = write_size;
+
+ /*
+ * Allocate a new output buffer and prepare to move the remainder
+ * of the original outBuffer there.
+ */
+ remaining -= write_size;
+ ptr += write_size;
+
+ conn->outBufSize = Max(16 * 1024, remaining);
+ conn->outBuffer = malloc(conn->outBufSize);
+ if (conn->outBuffer == NULL)
+ {
+ printfPQExpBuffer(&conn->errorMessage,
+ "cannot allocate memory for output buffer\n");
+ return -1;
+ }
+ }
+#endif
else
{
ptr += sent;
@@ -903,10 +948,36 @@ pqSendSome(PGconn *conn, int len)
}
}
- /* shift the remaining contents of the buffer */
- if (remaining > 0)
- memmove(conn->outBuffer, ptr, remaining);
- conn->outCount = remaining;
+#ifdef USE_SSL
+ if (conn->sslRetryPos && sent > 0)
+ {
+ /*
+ * A SSL write was successfully retried, free the retry buffer and
+ * switch to the regular outBuffer. Note that we expect that the entire
+ * buffer was written since SSL partial writes ought to be disabled.
+ */
+ free(conn->sslOutBufPtr);
+ conn->sslRetryPos = NULL;
+ conn->sslOutBufPtr = NULL;
+ conn->sslRetrySize = 0;
+
+ /*
+ * Indicate that a retry is needed if we still have some data left in
+ * output buffers. We are in non-blocking mode, so this shouldn't
+ * be a surprise.
+ */
+ if (conn->outCount > 0)
+ result = 1;
+ }
+#endif
+
+ /* Shift the remaining contents of the buffer. */
+ if (shiftOutBuffer)
+ {
+ if (remaining > 0)
+ memmove(conn->outBuffer, ptr, remaining);
+ conn->outCount = remaining;
+ }
return result;
}
@@ -924,7 +995,11 @@ pqFlush(PGconn *conn)
if (conn->Pfdebug)
fflush(conn->Pfdebug);
+#ifdef USE_SSL
+ if (conn->outCount > 0 || conn->sslRetrySize > 0)
+#else
if (conn->outCount > 0)
+#endif
return pqSendSome(conn, conn->outCount);
return 0;
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index d56ef5d..da1f2da 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -407,6 +407,10 @@ struct pg_conn
X509 *peer; /* X509 cert of server */
char peer_dn[256 + 1]; /* peer distinguished name */
char peer_cn[SM_USER + 1]; /* peer common name */
+
+ char *sslRetryPos; /* last buffer address passed SSL write */
+ char *sslOutBufPtr; /* pointer to the original outBuffer */
+ int sslRetrySize; /* last buffer length passed SSL write */
#ifdef USE_SSL_ENGINE
ENGINE *engine; /* SSL engine, if any */
#else
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
