Обсуждение: Fix some memory leaks in ecpg.addons

clang and gcc both now support -fsanitize=address,undefined. These are
really useful to me personally when trying to debug issues.
Unfortunately ecpg code has a ton of memory leaks, which makes builds
really painful. It would be great to fix all of them, but I don't have
the patience to try to read flex/bison code. Here are two memory leak
fixes in any case.

--
Tristan Partin
Neon (https://neon.tech)

"Tristan Partin" <tristan@neon.tech> writes:
> clang and gcc both now support -fsanitize=address,undefined. These are 
> really useful to me personally when trying to debug issues. 
> Unfortunately ecpg code has a ton of memory leaks, which makes builds 
> really painful. It would be great to fix all of them, but I don't have 
> the patience to try to read flex/bison code. Here are two memory leak 
> fixes in any case.

I'm kind of failing to see the point.  As you say, the ecpg
preprocessor leaks memory like there's no tomorrow.  But given its
usage (process one source file and exit) I'm not sure that is worth
much effort to fix.  And what does it buy to fix just two spots?

            regards, tom lane

Am Mittwoch, dem 08.11.2023 um 12:07 -0500 schrieb Tom Lane:
> "Tristan Partin" <tristan@neon.tech> writes:
> > clang and gcc both now support -fsanitize=address,undefined. These
> > are
> > really useful to me personally when trying to debug issues.
> > Unfortunately ecpg code has a ton of memory leaks, which makes
> > builds
> > really painful. It would be great to fix all of them, but I don't
> > have
> > the patience to try to read flex/bison code. Here are two memory
> > leak
> > fixes in any case.
>
> I'm kind of failing to see the point.  As you say, the ecpg
> preprocessor leaks memory like there's no tomorrow.  But given its
> usage (process one source file and exit) I'm not sure that is worth
> much effort to fix.  And what does it buy to fix just two spots?

Agreed, it's not exactly uncommon for tools like ecpg to not worry
about memory. After all it gets freed when the program ends.

Michael
--
Michael Meskes
Michael at Fam-Meskes dot De
Michael at Meskes dot (De|Com|Net|Org)
Meskes at (Debian|Postgresql) dot Org

On Wed Nov 8, 2023 at 11:18 AM CST, Michael Meskes wrote:
> Am Mittwoch, dem 08.11.2023 um 12:07 -0500 schrieb Tom Lane:
> > "Tristan Partin" <tristan@neon.tech> writes:
> > > clang and gcc both now support -fsanitize=address,undefined. These
> > > are
> > > really useful to me personally when trying to debug issues.
> > > Unfortunately ecpg code has a ton of memory leaks, which makes
> > > builds
> > > really painful. It would be great to fix all of them, but I don't
> > > have
> > > the patience to try to read flex/bison code. Here are two memory
> > > leak
> > > fixes in any case.
> >
> > I'm kind of failing to see the point.  As you say, the ecpg
> > preprocessor leaks memory like there's no tomorrow.  But given its
> > usage (process one source file and exit) I'm not sure that is worth
> > much effort to fix.  And what does it buy to fix just two spots?
>
> Agreed, it's not exactly uncommon for tools like ecpg to not worry
> about memory. After all it gets freed when the program ends.

In the default configuration of AddressSanitizer, I can't even complete
a full build of Postgres.

    meson setup build -Db_sanitize=address
    ninja -C build
    [1677/1855] Generating src/interfaces/ecpg/test/compat_informix/rfmtlong.c with a custom command
    FAILED: src/interfaces/ecpg/test/compat_informix/rfmtlong.c
    /home/tristan957/Projects/work/postgresql/build/src/interfaces/ecpg/preproc/ecpg --regression
-I../src/interfaces/ecpg/test/compat_informix-I../src/interfaces/ecpg/include/ -C INFORMIX -o
src/interfaces/ecpg/test/compat_informix/rfmtlong.c../src/interfaces/ecpg/test/compat_informix/rfmtlong.pgc 

    =================================================================
    ==114881==ERROR: LeakSanitizer: detected memory leaks

    Direct leak of 5 byte(s) in 1 object(s) allocated from:
        #0 0x7f88c34814a8 in strdup (/lib64/libasan.so.8+0x814a8) (BuildId: 6f17f87dc4c1aa9f9dde7c4856604c3a25ba4872)
        #1 0x4cfd93 in get_progname ../src/port/path.c:589
        #2 0x4b6dae in main ../src/interfaces/ecpg/preproc/ecpg.c:137
        #3 0x7f88c3246149 in __libc_start_call_main (/lib64/libc.so.6+0x28149) (BuildId:
651b2bed7ecaf18098a63b8f10299821749766e6)
        #4 0x7f88c324620a in __libc_start_main_impl (/lib64/libc.so.6+0x2820a) (BuildId:
651b2bed7ecaf18098a63b8f10299821749766e6)
        #5 0x402664 in _start
(/home/tristan957/Projects/work/postgresql/build/src/interfaces/ecpg/preproc/ecpg+0x402664)(BuildId:
fab06f774e305cbe628e03cdc22d935f7bb70a76)

    SUMMARY: AddressSanitizer: 5 byte(s) leaked in 1 allocation(s).
    ninja: build stopped: subcommand failed.

Are people using some suppression file or setting ASAN_OPTIONS to
something?

Here is a patch with a better solution.

--
Tristan Partin
Neon (https://neon.tech)

"Tristan Partin" <tristan@neon.tech> writes:
> On Wed Nov 8, 2023 at 11:18 AM CST, Michael Meskes wrote:
>> Agreed, it's not exactly uncommon for tools like ecpg to not worry
>> about memory. After all it gets freed when the program ends.

> In the default configuration of AddressSanitizer, I can't even complete 
> a full build of Postgres.

Why is the meson stuff building ecpg test cases as part of the core build?
That seems wrong for a number of reasons, not only that we don't hold
that code to the same standards as the core server.

            regards, tom lane

Hello Tristan,

08.11.2023 20:37, Tristan Partin wrote:
> Are people using some suppression file or setting ASAN_OPTIONS to something?
>

I use the following:
ASAN_OPTIONS=detect_leaks=0:abort_on_error=1:print_stacktrace=1:\
disable_coredump=0:strict_string_checks=1:check_initialization_order=1:\
strict_init_order=1:detect_stack_use_after_return=0

(You'll need to add detect_stack_use_after_return=0 with a newer clang
(I use clang-18) to workaround an incompatibility of check_stack_depth()
with that sanitizer feature enabled by default.)

There is also another story with hwasan ([1]).
and yet another incompatibility of check_stack_depth() related to the
aarch64-specific address tagging (TBI).

So I would say that fixing ecpg won't make postgres sanitizer-friendly in
a whole.

[1] https://www.postgresql.org/message-id/dbf77bf7-6e54-ed8a-c4ae-d196eeb664ce%40gmail.com

Best regards,
Alexander

On Wed Nov 8, 2023 at 11:52 AM CST, Tom Lane wrote:
> "Tristan Partin" <tristan@neon.tech> writes:
> > On Wed Nov 8, 2023 at 11:18 AM CST, Michael Meskes wrote:
> >> Agreed, it's not exactly uncommon for tools like ecpg to not worry
> >> about memory. After all it gets freed when the program ends.
>
> > In the default configuration of AddressSanitizer, I can't even complete
> > a full build of Postgres.
>
> Why is the meson stuff building ecpg test cases as part of the core build?
> That seems wrong for a number of reasons, not only that we don't hold
> that code to the same standards as the core server.

After looking into this a tiny bit more, we are building the
dependencies of the ecpg tests.

> foreach pgc_file : pgc_files
>   exe_input = custom_target('@0@.c'.format(pgc_file),
>     input: '@0@.pgc'.format(pgc_file),
>     output: '@BASENAME@.c',
>     command: ecpg_preproc_test_command_start +
>       ['-C', 'ORACLE',] +
>       ecpg_preproc_test_command_end,
>     install: false,
>     build_by_default: false,
>     kwargs: exe_preproc_kw,
>   )
>
>   ecpg_test_dependencies += executable(pgc_file,
>     exe_input,
>     kwargs: ecpg_test_exec_kw,
>   )
> endforeach

This is the pattern that we have in all the ecpg/test/*/meson.build
files. That ecpg_test_dependencies variable is then used in the actual
ecpg tests:

> tests += {
>   'name': 'ecpg',
>   'sd': meson.current_source_dir(),
>   'bd': meson.current_build_dir(),
>   'ecpg': {
>     'expecteddir': meson.current_source_dir(),
>     'inputdir': meson.current_build_dir(),
>     'schedule': ecpg_test_files,
>     'sql': [
>       'sql/twophase',
>     ],
>     'test_kwargs': {
>       'depends': ecpg_test_dependencies,
>     },
>     'dbname': 'ecpg1_regression,ecpg2_regression',
>     'regress_args': ecpg_regress_args,
>   },
> }

So in my opinion there is nothing wrong here. The build is working as
intended. Does this make sense to you, Tom?

--
Tristan Partin
Neon (https://neon.tech)

Hi,

On 2023-11-08 11:37:46 -0600, Tristan Partin wrote:
> On Wed Nov 8, 2023 at 11:18 AM CST, Michael Meskes wrote:
> > Am Mittwoch, dem 08.11.2023 um 12:07 -0500 schrieb Tom Lane:
> > > "Tristan Partin" <tristan@neon.tech> writes:
> > > > clang and gcc both now support -fsanitize=address,undefined. These
> > > > are > > really useful to me personally when trying to debug issues.
> > > > Unfortunately ecpg code has a ton of memory leaks, which makes
> > > > builds > > really painful. It would be great to fix all of them, but
> > I don't
> > > > have > > the patience to try to read flex/bison code. Here are two
> > memory
> > > > leak > > fixes in any case.
> > > > I'm kind of failing to see the point.  As you say, the ecpg
> > > preprocessor leaks memory like there's no tomorrow.  But given its
> > > usage (process one source file and exit) I'm not sure that is worth
> > > much effort to fix.  And what does it buy to fix just two spots?
> > 
> > Agreed, it's not exactly uncommon for tools like ecpg to not worry
> > about memory. After all it gets freed when the program ends.
> 
> In the default configuration of AddressSanitizer, I can't even complete a
> full build of Postgres.

I don't find the leak checks very useful for the moment. Leaks that happen
once in the lifetime of the program aren't problematic, and often tracking
them would make code more complicated.  Perhaps we'll eventually change our
tune on this, but I don't think it's worth fighting this windmill at this
point. I think at the very least we'd first want to port the memory context
infrastructure to frontend programs.

> 
> Are people using some suppression file or setting ASAN_OPTIONS to something?

You pretty much have to. Locally I use this:

export
ASAN_OPTIONS='debug=1:print_stacktrace=1:disable_coredump=0:abort_on_error=1:detect_leaks=0:detect_stack_use_after_return=0'
UBSAN_OPTIONS='print_stacktrace=1:disable_coredump=0:abort_on_error=1'

CI uses something similar.

Greetings,

Andres Freund

Hi,

On 2023-11-08 22:00:00 +0300, Alexander Lakhin wrote:
> Hello Tristan,
>
> 08.11.2023 20:37, Tristan Partin wrote:
> > Are people using some suppression file or setting ASAN_OPTIONS to something?
> >
>
> I use the following:
> ASAN_OPTIONS=detect_leaks=0:abort_on_error=1:print_stacktrace=1:\
> disable_coredump=0:strict_string_checks=1:check_initialization_order=1:\
> strict_init_order=1:detect_stack_use_after_return=0

I wonder if we should add some of these options by default ourselves. We could
e.g. add something like the __ubsan_default_options() in
src/backend/main/main.c to src/port/... instead, and return a combination of
"our" options (like detect_leaks=0) and the ones from the environment.


> (You'll need to add detect_stack_use_after_return=0 with a newer clang
> (I use clang-18) to workaround an incompatibility of check_stack_depth()
> with that sanitizer feature enabled by default.)

I have been wondering if we should work on fixing that.  There are a few ways:

- We can add a compiler parameter explicitly disabling the use-after-return
  checks - however, the checks are quite useful, so that'd be somewhat of a
  shame.

- We could exempt the stack depth checking functions from being validated with
  asan, I think that should fix this issue. Looks like
  __attribute__((no_sanitize("address")))
  would work

- Asan has an interface for getting the real stack address. See
  https://github.com/llvm/llvm-project/blob/main/compiler-rt/include/sanitizer/asan_interface.h#L322


ISTM that, if it actually works as I theorize it should, using
__attribute__((no_sanitize("address"))) would be the easiest approach
here. Something like

#if defined(__has_feature) && __has_feature(address_sanitizer)
#define pg_attribute_no_asan __attribute__((no_sanitize("address")))
#else
#define pg_attribute_no_asan
#endif

or such should work.


> So I would say that fixing ecpg won't make postgres sanitizer-friendly in
> a whole.

One thing that's been holding me back on trying to do something around this is
the basically non-existing documentation around all of this. I haven't even
found documentation referencing the fact that there are headers like
sanitizer/asan_interface.h, you just have to figure that out yourself. Compare
that to something like valgrind, which has documented this at least somewhat.

Greetings,

Andres Freund