Re: [PATCH] Log details for client certificate failures

Hi,

On Tue, Sep 13, 2022 at 11:11 PM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
>
> On 09.09.22 00:32, Jacob Champion wrote:
> > On Thu, Jul 28, 2022 at 9:19 AM Jacob Champion <jchampion@timescale.com> wrote:
> >> On Thu, Jul 21, 2022 at 4:29 PM Jacob Champion <jchampion@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