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

Started by Ranier Vilelaover 5 years ago3 messages

ranier.vf@gmail.com

over 5 years ago

1 attachment(s)

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)

Attachments:

fix_resource_leak_dba.patchapplication/octet-stream; name=fix_resource_leak_dba.patchDownload

diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
index 9d63830553..6cad0c78a2 100644
--- a/src/backend/libpq/hba.c
+++ b/src/backend/libpq/hba.c
@@ -1938,6 +1938,7 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
 							val),
 					 errcontext("line %d of configuration file \"%s\"",
 								line_num, HbaFileName)));
+			list_free(parsed_servers);
 			return false;
 		}
 
@@ -1987,6 +1988,7 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
 					 errcontext("line %d of configuration file \"%s\"",
 								line_num, HbaFileName)));
 			*err_msg = psprintf("invalid RADIUS port number: \"%s\"", val);
+			list_free(parsed_ports);
 			return false;
 		}
 
@@ -2000,6 +2002,7 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
 						 errcontext("line %d of configuration file \"%s\"",
 									line_num, HbaFileName)));
 
+			    list_free(parsed_ports);
 				return false;
 			}
 		}
@@ -2022,6 +2025,7 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
 							val),
 					 errcontext("line %d of configuration file \"%s\"",
 								line_num, HbaFileName)));
+			list_free(parsed_secrets);
 			return false;
 		}
 
@@ -2044,6 +2048,7 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline,
 							val),
 					 errcontext("line %d of configuration file \"%s\"",
 								line_num, HbaFileName)));
+			list_free(parsed_identifiers);
 			return false;
 		}

Kyotaro Horiguchi

horikyota.ntt@gmail.com

over 5 years ago

In reply to: Ranier Vilela (#1)

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

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

Ranier Vilela

ranier.vf@gmail.com

over 5 years ago

In reply to: Kyotaro Horiguchi (#2)

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

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