Re: [PATCH v12] GSSAPI encryption support - Mailing list pgsql-hackers

On Tue, Apr 5, 2016 at 9:06 AM, Robbie Harwood <rharwood@redhat.com> wrote:
> Here's v12, both here and on my github:
> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt12
>
> What changed:
>
> - The code is aware of memory contexts now.  I actually really like the
>   memory context stuff; just didn't see any indication of its existence
>   in the code I had read.  Anyway, we allocate server buffers in the
>   connection-lifetime context.  The other alternative that we discussed
>   on IRC a bit was to avoid palloc()/pfree() entirely in favor of raw
>   calloc()/free(), but I think if possible I prefer this approach since
>   I find the StringInfo handy to work with.  This eliminates the
>   traceback for me with --enable-cassert.

Affirnative, I am not seeing the failure anymore.

+#ifdef ENABLE_GSS
+   {
+       MemoryContext save = CurrentMemoryContext;
+       CurrentMemoryContext = TopMemoryContext;
+
+       initStringInfo(&port->gss->buf);
+       initStringInfo(&port->gss->writebuf);
+
+       CurrentMemoryContext = save;
+   }
+#endif
So you are saving everything in the top memory context. I am fine to
give the last word to a committer here but I would just go with
calloc/free to simplify those hunks.

+#ifdef ENABLE_GSS
+       if (conn->gss->buf.data)
+           pfree(conn->gss->buf.data);
+       if (conn->gss->writebuf.data)
+           pfree(conn->gss->writebuf.data);
+#endif
This should happen in its own memory context, no

> - Error cleanup.  I've been looking very hard at this code in order to
>   try to fix the assert, and I've fixed up a couple error paths that
>   hadn't been taken.  This involves replacing the double-send with a
>   buffer-and-then-send, which turns out to be not only shorter but
>   easier for me to reason about.

@@ -775,6 +775,7 @@ infodirdocdiroldincludedirincludedir
+runstatedirlocalstatedirsharedstatedirsysconfdir
@@ -896,6 +897,7 @@
datadir='${datarootdir}'sysconfdir='${prefix}/etc'sharedstatedir='${prefix}/com'localstatedir='${prefix}/var'
+runstatedir='${localstatedir}/run'
What's that? I would recommend re-running autoconf to remove this
portion (committers do it at the end btw, so that's actually no bug
deal).


-#if defined(WIN32) && !defined(WIN32_ONLY_COMPILER)
-/*
- * MIT Kerberos GSSAPI DLL doesn't properly export the symbols for MingW
- * that contain the OIDs required. Redefine here, values copied
- * from src/athena/auth/krb5/src/lib/gssapi/generic/gssapi_generic.c
- */
-static const gss_OID_desc GSS_C_NT_USER_NAME_desc =
-{10, (void *) "\x2a\x86\x48\x86\xf7\x12\x01\x02\x01\x02"};
-static GSS_DLLIMP gss_OID GSS_C_NT_USER_NAME = &GSS_C_NT_USER_NAME_desc;
-#endif
Regarding patch 0003 it may be fine to remove that... Robbie, do you
know how long ago this has been fixed upstream? I'd rather not have
this bit removed if this could impact some users.

On 0003, +1 for reading the whole stack of messages. That's definitely
worth a separate patch.

Btw, those seem like small things to me, and my comments have been
addressed, so I have switched the patch as ready for committer. I
guess that Stephen would be the one to look at it.
-- 
Michael