Re: [PATCH] Log details for client certificate failures

[Masahiko Sawada]

Hi,

On Tue, Sep 13, 2022 at 11:11 PM Peter Eisentraut
<peter.eisentr...@enterprisedb.com> wrote:
>
> On 09.09.22 00:32, Jacob Champion wrote:
> > On Thu, Jul 28, 2022 at 9:19 AM Jacob Champion <jchamp...@timescale.com> 
> > wrote:
> >> On Thu, Jul 21, 2022 at 4:29 PM Jacob Champion <jchamp...@timescale.com> 
> >> wrote:
> >>> v4 attempts to fix this by letting the check hooks pass
> >>> MCXT_ALLOC_NO_OOM to pg_clean_ascii(). (It's ignored in the frontend,
> >>> which just mallocs.)
> >>
> >> Ping -- should I add an open item somewhere so this isn't lost?
> >
> > Trying again. Peter, is this approach acceptable? Should I try something 
> > else?
>
> This looks fine to me.  Committed.

While looking at the recent changes for check_cluster_name() I found
this thread. Regarding the following change made by the commit
45b1a67a0fc, there is possibly small memory leak:

 static bool
 check_cluster_name(char **newval, void **extra, GucSource source)
 {
+   char       *clean;
+
    /* Only allow clean ASCII chars in the cluster name */
-   pg_clean_ascii(*newval);
+   clean = pg_clean_ascii(*newval, MCXT_ALLOC_NO_OOM);
+   if (!clean)
+       return false;
+
+   clean = guc_strdup(WARNING, clean);
+   if (!clean)
+       return false;

+   *newval = clean;
    return true;
 }

pg_clean_ascii() does palloc_extended() to allocate memory in
Postmaster context for the new characters and the clean is then
replaced with the new memory allocated by guc_strdup(). No-one
references the memory allocated by pg_clean_ascii() and it lasts for
postmaster lifetime. Valgrind memcheck also shows:

 1 bytes in 1 blocks are definitely lost in loss record 4 of 70
    at 0xCD2A16: palloc_extended (mcxt.c:1239)
    by 0xD09437: pg_clean_ascii (string.c:99)
    by 0x7A5CF3: check_cluster_name (variable.c:1061)
    by 0xCAF7CD: call_string_check_hook (guc.c:6365)
    by 0xCAA724: InitializeOneGUCOption (guc.c:1439)
    by 0xCAA0ED: InitializeGUCOptions (guc.c:1268)
    by 0x99B245: PostmasterMain (postmaster.c:691)
    by 0x858896: main (main.c:197)

I think we can fix it by the attached patch but I'd like to discuss
whether it's worth fixing it.

Regards,

--
Masahiko Sawada
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c
index c795cb7a29..e555fb3150 100644
--- a/src/backend/commands/variable.c
+++ b/src/backend/commands/variable.c
@@ -1025,17 +1025,22 @@ bool
 check_application_name(char **newval, void **extra, GucSource source)
 {
 	char	   *clean;
+	char	   *ret;
 
 	/* Only allow clean ASCII chars in the application name */
 	clean = pg_clean_ascii(*newval, MCXT_ALLOC_NO_OOM);
 	if (!clean)
 		return false;
 
-	clean = guc_strdup(WARNING, clean);
-	if (!clean)
+	ret = guc_strdup(WARNING, clean);
+	if (!ret)
+	{
+		pfree(clean);
 		return false;
+	}
 
-	*newval = clean;
+	pfree(clean);
+	*newval = ret;
 	return true;
 }
 
@@ -1056,17 +1061,22 @@ bool
 check_cluster_name(char **newval, void **extra, GucSource source)
 {
 	char	   *clean;
+	char	   *ret;
 
 	/* Only allow clean ASCII chars in the cluster name */
 	clean = pg_clean_ascii(*newval, MCXT_ALLOC_NO_OOM);
 	if (!clean)
 		return false;
 
-	clean = guc_strdup(WARNING, clean);
-	if (!clean)
+	ret = guc_strdup(WARNING, clean);
+	if (!ret)
+	{
+		pfree(clean);
 		return false;
+	}
 
-	*newval = clean;
+	pfree(clean);
+	*newval = ret;
 	return true;
 }