Thread: [PATCH] Resource leaks (src/backend/libpq/hba.c)

[PATCH] Resource leaks (src/backend/libpq/hba.c)

From
Ranier Vilela
Date:
Hi Tom,

Per Coverity.

The function parse_hba_auth_op at (src/backend/libpq/hba.c) allows resource leaks when called
by the function parse_hba_line, with parameters LOG and DEBUG3 levels.

The function SplitGUCList (src/bin/pg_dump/dumputils.c) allows even returning FALSE,
that namelist list is not empty and as memory allocated by pg_malloc.

The simplest solution is free namelist, even when calling ereport, why the level can be
LOG or DEBUG3.

regards,
Ranier Vilela

PS. Are two SplitGUCList in codebase.
1. SplitGUCList (src/bin/pg_dump/dumputils.c)
2. SplitGUCList (src/backend/utils/adt/varlena.c)
Attachment

Re: [PATCH] Resource leaks (src/backend/libpq/hba.c)

From
Kyotaro Horiguchi
Date:
At Tue, 25 Aug 2020 10:20:07 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in 
> Hi Tom,
> 
> Per Coverity.
> 
> The function parse_hba_auth_op at (src/backend/libpq/hba.c) allows resource
> leaks when called
> by the function parse_hba_line, with parameters LOG and DEBUG3 levels.
> The function SplitGUCList (src/bin/pg_dump/dumputils.c) allows even
> returning FALSE,
> that namelist list is not empty and as memory allocated by pg_malloc.

As you know, there are two implementations of the function. One that
uses pg_malloc is used in pg_dump and the returned char *namelist is
always pg_free'd after use. The other that returns a pg_list, and the
returned list is reclaimed by MemoryContextDelete at callers
(concretely load_hba and fill_hba_view).  Indeed they share the same
name but have different signatures so the two are statically
distinguishable but Coverity seems failing to do so. You may need to
avoid feeding the whole source tree to Coverity at once.

Anyway this is a very common style in the PostgreSQL code so I
recommend to verify the outcome from such tools against the actual
code.

> The simplest solution is free namelist, even when calling ereport, why the
> level can be
> LOG or DEBUG3.

So we don't need to do anything there. Rather we can remove the
existing list_free(parsed_servers) in parse_hba_auth_opt.

> regards,
> Ranier Vilela
> 
> PS. Are two SplitGUCList in codebase.
> 1. SplitGUCList (src/bin/pg_dump/dumputils.c)
> 2. SplitGUCList (src/backend/utils/adt/varlena.c)

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center



Re: [PATCH] Resource leaks (src/backend/libpq/hba.c)

From
Ranier Vilela
Date:
Em ter., 25 de ago. de 2020 às 23:02, Kyotaro Horiguchi <horikyota.ntt@gmail.com> escreveu:
At Tue, 25 Aug 2020 10:20:07 -0300, Ranier Vilela <ranier.vf@gmail.com> wrote in
> Hi Tom,
>
> Per Coverity.
>
> The function parse_hba_auth_op at (src/backend/libpq/hba.c) allows resource
> leaks when called
> by the function parse_hba_line, with parameters LOG and DEBUG3 levels.
> The function SplitGUCList (src/bin/pg_dump/dumputils.c) allows even
> returning FALSE,
> that namelist list is not empty and as memory allocated by pg_malloc.

As you know, there are two implementations of the function. One that
uses pg_malloc is used in pg_dump and the returned char *namelist is
always pg_free'd after use. The other that returns a pg_list, and the
returned list is reclaimed by MemoryContextDelete at callers
(concretely load_hba and fill_hba_view).  Indeed they share the same
name but have different signatures so the two are statically
distinguishable but Coverity seems failing to do so. You may need to
avoid feeding the whole source tree to Coverity at once.
Yes, thanks for the hit.

Anyway this is a very common style in the PostgreSQL code so I
recommend to verify the outcome from such tools against the actual
code.
 Ok.


> The simplest solution is free namelist, even when calling ereport, why the
> level can be
> LOG or DEBUG3.

So we don't need to do anything there. Rather we can remove the
existing list_free(parsed_servers) in parse_hba_auth_opt.
It would be good, the call helped to confuse.
 
Very thanks, for the explanation.

Ranier Vilela