Re: 12.1 not useable: clientlib fails after a dozen queries (GSSAPI ?) - Mailing list pgsql-hackers

I wrote:
> (Still doesn't touch the static-buffer issue, though.)

And here's a delta patch for that.  This fixes an additional portability
hazard, which is that there were various places doing stuff like

        input.length = ntohl(*(uint32 *) PqGSSRecvBuffer);

That's a SIGBUS hazard on alignment-picky hardware, because there is
no guarantee that a variable that's just declared "char ...[...]"
will have any particular alignment.  But malloc'ing the space will
provide maxaligned storage.

My FreeBSD testing has now given me enough confidence in these patches
that I'm just going to go ahead and push them.  But, if you'd like to
do some more testing in advance of 12.2 release, please do.

            regards, tom lane

diff --git a/src/backend/libpq/be-secure-gssapi.c b/src/backend/libpq/be-secure-gssapi.c
index 87cb6ea..c25cfda 100644
--- a/src/backend/libpq/be-secure-gssapi.c
+++ b/src/backend/libpq/be-secure-gssapi.c
@@ -50,27 +50,30 @@
 #define PQ_GSS_SEND_BUFFER_SIZE 16384
 #define PQ_GSS_RECV_BUFFER_SIZE 16384

-/* PqGSSSendBuffer is for *encrypted* data */
-static char PqGSSSendBuffer[PQ_GSS_SEND_BUFFER_SIZE];
+/*
+ * Since we manage at most one GSS-encrypted connection per backend,
+ * we can just keep all this state in static variables.  The char *
+ * variables point to buffers that are allocated once and re-used.
+ */
+static char *PqGSSSendBuffer;    /* Encrypted data waiting to be sent */
 static int    PqGSSSendLength;    /* End of data available in PqGSSSendBuffer */
 static int    PqGSSSendNext;        /* Next index to send a byte from
                                  * PqGSSSendBuffer */
 static int    PqGSSSendConsumed;    /* Number of *unencrypted* bytes consumed for
                                  * current contents of PqGSSSendBuffer */

-/* PqGSSRecvBuffer is for *encrypted* data */
-static char PqGSSRecvBuffer[PQ_GSS_RECV_BUFFER_SIZE];
+static char *PqGSSRecvBuffer;    /* Received, encrypted data */
 static int    PqGSSRecvLength;    /* End of data available in PqGSSRecvBuffer */

-/* PqGSSResultBuffer is for *unencrypted* data */
-static char PqGSSResultBuffer[PQ_GSS_RECV_BUFFER_SIZE];
+static char *PqGSSResultBuffer; /* Decryption of data in gss_RecvBuffer */
 static int    PqGSSResultLength;    /* End of data available in PqGSSResultBuffer */
 static int    PqGSSResultNext;    /* Next index to read a byte from
                                  * PqGSSResultBuffer */

-uint32        max_packet_size;    /* Maximum size we can encrypt and fit the
+static uint32 PqGSSMaxPktSize;    /* Maximum size we can encrypt and fit the
                                  * results into our output buffer */

+
 /*
  * Attempt to write len bytes of data from ptr to a GSSAPI-encrypted connection.
  *
@@ -175,8 +178,8 @@ be_gssapi_write(Port *port, void *ptr, size_t len)
          * we will have to loop and possibly be called multiple times to get
          * through all the data.
          */
-        if (bytes_to_encrypt > max_packet_size)
-            input.length = max_packet_size;
+        if (bytes_to_encrypt > PqGSSMaxPktSize)
+            input.length = PqGSSMaxPktSize;
         else
             input.length = bytes_to_encrypt;

@@ -461,7 +464,20 @@ secure_open_gssapi(Port *port)
     OM_uint32    major,
                 minor;

-    /* initialize state variables */
+    /*
+     * Allocate buffers and initialize state variables.  By malloc'ing the
+     * buffers at this point, we avoid wasting static data space in processes
+     * that will never use them, and we ensure that the buffers are
+     * sufficiently aligned for the length-word accesses that we do in some
+     * places in this file.
+     */
+    PqGSSSendBuffer = malloc(PQ_GSS_SEND_BUFFER_SIZE);
+    PqGSSRecvBuffer = malloc(PQ_GSS_RECV_BUFFER_SIZE);
+    PqGSSResultBuffer = malloc(PQ_GSS_RECV_BUFFER_SIZE);
+    if (!PqGSSSendBuffer || !PqGSSRecvBuffer || !PqGSSResultBuffer)
+        ereport(FATAL,
+                (errcode(ERRCODE_OUT_OF_MEMORY),
+                 errmsg("out of memory")));
     PqGSSSendLength = PqGSSSendNext = PqGSSSendConsumed = 0;
     PqGSSRecvLength = PqGSSResultLength = PqGSSResultNext = 0;

@@ -608,7 +624,7 @@ secure_open_gssapi(Port *port)
      */
     major = gss_wrap_size_limit(&minor, port->gss->ctx, 1, GSS_C_QOP_DEFAULT,
                                 PQ_GSS_SEND_BUFFER_SIZE - sizeof(uint32),
-                                &max_packet_size);
+                                &PqGSSMaxPktSize);

     if (GSS_ERROR(major))
         pg_GSS_error(FATAL, gettext_noop("GSSAPI size check error"),
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 89b1346..80b54bc 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -462,7 +462,7 @@ pqDropConnection(PGconn *conn, bool flushInput)
     /* Always discard any unsent data */
     conn->outCount = 0;

-    /* Free authentication state */
+    /* Free authentication/encryption state */
 #ifdef ENABLE_GSS
     {
         OM_uint32    min_s;
@@ -471,6 +471,21 @@ pqDropConnection(PGconn *conn, bool flushInput)
             gss_delete_sec_context(&min_s, &conn->gctx, GSS_C_NO_BUFFER);
         if (conn->gtarg_nam)
             gss_release_name(&min_s, &conn->gtarg_nam);
+        if (conn->gss_SendBuffer)
+        {
+            free(conn->gss_SendBuffer);
+            conn->gss_SendBuffer = NULL;
+        }
+        if (conn->gss_RecvBuffer)
+        {
+            free(conn->gss_RecvBuffer);
+            conn->gss_RecvBuffer = NULL;
+        }
+        if (conn->gss_ResultBuffer)
+        {
+            free(conn->gss_ResultBuffer);
+            conn->gss_ResultBuffer = NULL;
+        }
     }
 #endif
 #ifdef ENABLE_SSPI
diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libpq/fe-secure-gssapi.c
index 054cf92..9fd06ea 100644
--- a/src/interfaces/libpq/fe-secure-gssapi.c
+++ b/src/interfaces/libpq/fe-secure-gssapi.c
@@ -53,26 +53,22 @@
 #define PQ_GSS_SEND_BUFFER_SIZE 16384
 #define PQ_GSS_RECV_BUFFER_SIZE 16384

-/* PqGSSSendBuffer is for *encrypted* data */
-static char PqGSSSendBuffer[PQ_GSS_SEND_BUFFER_SIZE];
-static int    PqGSSSendLength;    /* End of data available in PqGSSSendBuffer */
-static int    PqGSSSendNext;        /* Next index to send a byte from
-                                 * PqGSSSendBuffer */
-static int    PqGSSSendConsumed;    /* Number of *unencrypted* bytes consumed for
-                                 * current contents of PqGSSSendBuffer */
-
-/* PqGSSRecvBuffer is for *encrypted* data */
-static char PqGSSRecvBuffer[PQ_GSS_RECV_BUFFER_SIZE];
-static int    PqGSSRecvLength;    /* End of data available in PqGSSRecvBuffer */
-
-/* PqGSSResultBuffer is for *unencrypted* data */
-static char PqGSSResultBuffer[PQ_GSS_RECV_BUFFER_SIZE];
-static int    PqGSSResultLength;    /* End of data available in PqGSSResultBuffer */
-static int    PqGSSResultNext;    /* Next index to read a byte from
-                                 * PqGSSResultBuffer */
-
-uint32        max_packet_size;    /* Maximum size we can encrypt and fit the
-                                 * results into our output buffer */
+/*
+ * We need these state variables per-connection.  To allow the functions
+ * in this file to look mostly like those in be-secure-gssapi.c, set up
+ * these macros.
+ */
+#define PqGSSSendBuffer (conn->gss_SendBuffer)
+#define PqGSSSendLength (conn->gss_SendLength)
+#define PqGSSSendNext (conn->gss_SendNext)
+#define PqGSSSendConsumed (conn->gss_SendConsumed)
+#define PqGSSRecvBuffer (conn->gss_RecvBuffer)
+#define PqGSSRecvLength (conn->gss_RecvLength)
+#define PqGSSResultBuffer (conn->gss_ResultBuffer)
+#define PqGSSResultLength (conn->gss_ResultLength)
+#define PqGSSResultNext (conn->gss_ResultNext)
+#define PqGSSMaxPktSize (conn->gss_MaxPktSize)
+

 /*
  * Attempt to write len bytes of data from ptr to a GSSAPI-encrypted connection.
@@ -184,8 +180,8 @@ pg_GSS_write(PGconn *conn, const void *ptr, size_t len)
          * we will have to loop and possibly be called multiple times to get
          * through all the data.
          */
-        if (bytes_to_encrypt > max_packet_size)
-            input.length = max_packet_size;
+        if (bytes_to_encrypt > PqGSSMaxPktSize)
+            input.length = PqGSSMaxPktSize;
         else
             input.length = bytes_to_encrypt;

@@ -477,7 +473,6 @@ gss_read(PGconn *conn, void *recv_buffer, size_t length, ssize_t *ret)
 PostgresPollingStatusType
 pqsecure_open_gss(PGconn *conn)
 {
-    static int    first = 1;
     ssize_t        ret;
     OM_uint32    major,
                 minor;
@@ -486,12 +481,25 @@ pqsecure_open_gss(PGconn *conn)
     gss_buffer_desc input = GSS_C_EMPTY_BUFFER,
                 output = GSS_C_EMPTY_BUFFER;

-    /* Initialize, if first time through */
-    if (first)
+    /*
+     * If first time through for this connection, allocate buffers and
+     * initialize state variables.  By malloc'ing the buffers separately, we
+     * ensure that they are sufficiently aligned for the length-word accesses
+     * that we do in some places in this file.
+     */
+    if (PqGSSSendBuffer == NULL)
     {
+        PqGSSSendBuffer = malloc(PQ_GSS_SEND_BUFFER_SIZE);
+        PqGSSRecvBuffer = malloc(PQ_GSS_RECV_BUFFER_SIZE);
+        PqGSSResultBuffer = malloc(PQ_GSS_RECV_BUFFER_SIZE);
+        if (!PqGSSSendBuffer || !PqGSSRecvBuffer || !PqGSSResultBuffer)
+        {
+            printfPQExpBuffer(&conn->errorMessage,
+                              libpq_gettext("out of memory\n"));
+            return PGRES_POLLING_FAILED;
+        }
         PqGSSSendLength = PqGSSSendNext = PqGSSSendConsumed = 0;
         PqGSSRecvLength = PqGSSResultLength = PqGSSResultNext = 0;
-        first = 0;
     }

     /*
@@ -651,7 +659,7 @@ pqsecure_open_gss(PGconn *conn)
          */
         major = gss_wrap_size_limit(&minor, conn->gctx, 1, GSS_C_QOP_DEFAULT,
                                     PQ_GSS_SEND_BUFFER_SIZE - sizeof(uint32),
-                                    &max_packet_size);
+                                    &PqGSSMaxPktSize);

         if (GSS_ERROR(major))
         {
diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
index 1e36be2..79bc378 100644
--- a/src/interfaces/libpq/libpq-int.h
+++ b/src/interfaces/libpq/libpq-int.h
@@ -493,6 +493,23 @@ struct pg_conn
     bool        try_gss;        /* GSS attempting permitted */
     bool        gssenc;            /* GSS encryption is usable */
     gss_cred_id_t gcred;        /* GSS credential temp storage. */
+
+    /* GSS encryption I/O state --- see fe-secure-gssapi.c */
+    char       *gss_SendBuffer; /* Encrypted data waiting to be sent */
+    int            gss_SendLength; /* End of data available in gss_SendBuffer */
+    int            gss_SendNext;    /* Next index to send a byte from
+                                 * gss_SendBuffer */
+    int            gss_SendConsumed;    /* Number of *unencrypted* bytes consumed
+                                     * for current contents of gss_SendBuffer */
+    char       *gss_RecvBuffer; /* Received, encrypted data */
+    int            gss_RecvLength; /* End of data available in gss_RecvBuffer */
+    char       *gss_ResultBuffer;    /* Decryption of data in gss_RecvBuffer */
+    int            gss_ResultLength;    /* End of data available in
+                                     * gss_ResultBuffer */
+    int            gss_ResultNext; /* Next index to read a byte from
+                                 * gss_ResultBuffer */
+    uint32        gss_MaxPktSize; /* Maximum size we can encrypt and fit the
+                                 * results into our output buffer */
 #endif

 #ifdef ENABLE_SSPI