[pgpool-hackers: 4475] Re: Segmentation fault on error at parsing config file

[Bo Peng]

Hi,

Sorry for the delay in reviewing your patch.

Another developer, Ishii-san had reviewd your patch
and modified your patch.

Below are some comments.
The following part is required to avoid memory leak.

--------------------------
@@ -458,8 +460,6 @@ ParseConfigFile(const char *config_file, const char *calling_file,
 
 parse_error:
 
-	if (key)
-		pfree(key);
--------------------------

I think it would be simpler to just set "key = NULL" at the beginning of the for loop.

--------------------------
 	for(;;)
 	{
+		key = NULL;
+
 		token = yylex();
--------------------------

I have committed the fix:

https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=c1b17275fc0db9991fb6c644d098bf10ca00cc66

Thank you for the issue reporting and the patch.

On Wed, 29 May 2024 17:05:06 -0500
Carlos Chapi <cchapi at systemguards.com.ec> wrote:

> If there's a syntax error in pgpool.conf like:
> 
> listen_addresses = '*
> 
> (without closing single quotes) trying to start pgpool results in a
> Segmentation Fault instead of a parsing error. Here's the backtrace:
> 
> Core was generated by `/usr/bin/pgpool -f /etc/pgpool-II/pgpool.conf -n'.
> Program terminated with signal SIGSEGV, Segmentation fault.
> #0  pfree (pointer=0x7f524ce70bc0) at parser/../../src/utils/mmgr/mcxt.c:956
> 956             (*context->methods->free_p) (context, pointer);
> (gdb) bt full
> #0  pfree (pointer=0x7f524ce70bc0) at parser/../../src/utils/mmgr/mcxt.c:956
>         context = 0x0
> #1  FreeConfigVariable (item=0x7f524ce70c08) at config/pool_config.l:287
> No locals.
> #2  FreeConfigVariables (list=<optimized out>) at config/pool_config.l:305
>         next = 0x0
>         item = 0x7f524ce70c08
> #3  ParseConfigFile (config_file=<optimized out>,
> calling_file=calling_file at entry=0x0, depth=depth at entry=0,
> elevel=elevel at entry=21, head_p=head_p at entry=0x7ffc7a650d50,
> tail_p=tail_p at entry=0x7ffc7a650d48) at config/pool_config.l:465
>         fd = 0x560db343af50
>         lex_buffer = 0x560db343b130
>         token = 8
>         key = <optimized out>
>         val = <optimized out>
>         item = <optimized out>
>         buf = "/etc/pgpool-II\000pgpool.conf", '\000' <repeats 5998
> times>...
>         config_filepath = 0x7f524ce70b00 "/etc/pgpool-II/pgpool.conf"
> #4  0x0000560db2501e2a in pool_get_config (config_file=<optimized out>,
> context=CFGCXT_INIT) at config/pool_config.l:485
>         head_p = 0x7f524ce70b90
>         tail_p = 0x7f524ce70c08
>         res = <optimized out>
>         elevel = 21
> #5  0x0000560db24f1417 in main (argc=<optimized out>, argv=<optimized out>)
> at main/main.c:231
>         opt = <optimized out>
>         debug_level = 0
>         optindex = 0
>         discard_status = 0 '\000'
>         clear_memcache_oidmaps = 0 '\000'
>         pcp_conf_file_path = "/etc/pgpool-II/pcp.conf", '\000' <repeats
> 8169 times>
>         conf_file_path = "/etc/pgpool-II/pgpool.conf", '\000' <repeats 8166
> times>
>         hba_file_path = "/etc/pgpool-II/pool_hba.conf", '\000' <repeats
> 8164 times>
>         pool_passwd_key_file_path = '\000' <repeats 2856 times>...
>         long_options = {{name = 0x560db25ea865 "hba-file", has_arg = 1,
> flag = 0x0, val = 97}, {name = 0x560db25e98d3 "debug", has_arg = 0, flag =
> 0x0, val = 100}, {name = 0x560db25ea86e "config-file", has_arg = 1, flag =
> 0x0, val = 102}, {
>             name = 0x560db25ea87a "key-file", has_arg = 1, flag = 0x0, val
> = 107}, {name = 0x560db25ea883 "pcp-file", has_arg = 1, flag = 0x0, val =
> 70}, {name = 0x560db25ea88c "help", has_arg = 0, flag = 0x0, val = 104},
> {name = 0x560db25ea606 "mode", has_arg = 1,
>             flag = 0x0, val = 109}, {name = 0x560db25ea891 "dont-detach",
> has_arg = 0, flag = 0x0, val = 110}, {name = 0x560db25ea89d
> "discard-status", has_arg = 0, flag = 0x0, val = 68}, {name =
> 0x560db25ea8ac "clear-oidmaps", has_arg = 0, flag = 0x0, val = 67}, {
>             name = 0x560db25ea8ba "debug-assertions", has_arg = 0, flag =
> 0x0, val = 120}, {name = 0x560db25fbb8b "version", has_arg = 0, flag = 0x0,
> val = 118}, {name = 0x0, has_arg = 0, flag = 0x0, val = 0}}
> 
> It seems this was introduced on this commit:
> 
> https://git.postgresql.org/gitweb/?p=pgpool2.git;a=commit;h=25d05e55e6ebf334e768a9b454ecdb84a8a4054d
> 
> I have attached a patch to mitigate this, but I'm not entirely sure if it's
> the right approach to avoiding the (possible?) memory leak with the "key"
> pointer.
> 
> I'll appreciate it if anyone can take a look at the patch or the issue
> overall.
> 
> Thanks,
> 
> 
> --
> Carlos Chapi


-- 
Bo Peng <pengbo at sraoss.co.jp>
SRA OSS LLC
TEL: 03-5979-2701 FAX: 03-5979-2702
URL: https://www.sraoss.co.jp/