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)
Attachments:
v1-0001-Patch-two-memory-leaks-in-ecpg.addons.patchtext/x-patch; charset=utf-8; name=v1-0001-Patch-two-memory-leaks-in-ecpg.addons.patchDownload
From af69e1711e87ae96d75814fc83a8588a4bdf7cac Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Wed, 8 Nov 2023 10:53:02 -0600
Subject: [PATCH v1] Patch two memory leaks in ecpg.addons
make_str() allocates memory, but we were never releasing it. There are
more issues in this code, but these were just two easy ones to fix.
---
src/interfaces/ecpg/preproc/ecpg.addons | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/interfaces/ecpg/preproc/ecpg.addons b/src/interfaces/ecpg/preproc/ecpg.addons
index e94da2a3f8..dd1ea39c6e 100644
--- a/src/interfaces/ecpg/preproc/ecpg.addons
+++ b/src/interfaces/ecpg/preproc/ecpg.addons
@@ -51,6 +51,8 @@ ECPG: stmtExecuteStmt block
str[strlen(str) - 1] = '\0';
sprintf(length, "%zu", strlen(str));
add_variable_to_tail(&argsinsert, new_variable(str, ECPGmake_simple_type(ECPGt_const, length, 0), 0), &no_indicator);
+
+ free(str);
}
output_statement(cat_str(3, mm_strdup("execute"), mm_strdup("$0"), $1.type), 0, ECPGst_exec_with_exprlist);
}
@@ -79,6 +81,8 @@ ECPG: stmtPrepareStmt block
str[strlen(str) - 1] = '\0';
sprintf(length, "%zu", strlen(str));
add_variable_to_tail(&argsinsert, new_variable(str, ECPGmake_simple_type(ECPGt_const, length, 0), 0), &no_indicator);
+
+ free(str);
}
output_statement(cat_str(5, mm_strdup("prepare"), mm_strdup("$0"), $1.type, mm_strdup("as"), $1.stmt), 0, ECPGst_prepare);
}
--
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)
Attachments:
v1-0001-Silence-AddressSanitizer-when-creating-custom-tar.patchtext/x-patch; charset=utf-8; name=v1-0001-Silence-AddressSanitizer-when-creating-custom-tar.patchDownload
From da1bce1d68a55d8f00093a1aa1b2dfe913bd4549 Mon Sep 17 00:00:00 2001
From: Tristan Partin <tristan@neon.tech>
Date: Wed, 8 Nov 2023 11:35:48 -0600
Subject: [PATCH v1] Silence AddressSanitizer when creating custom targets with
ecpg
ecpg leaks like no tomorrow and will cause builds to fail.
---
src/interfaces/ecpg/test/compat_oracle/meson.build | 12 ++++++++++++
src/interfaces/ecpg/test/meson.build | 9 +++++++++
2 files changed, 21 insertions(+)
diff --git a/src/interfaces/ecpg/test/compat_oracle/meson.build b/src/interfaces/ecpg/test/compat_oracle/meson.build
index f07d9859d1..3aff426fec 100644
--- a/src/interfaces/ecpg/test/compat_oracle/meson.build
+++ b/src/interfaces/ecpg/test/compat_oracle/meson.build
@@ -4,6 +4,17 @@ pgc_files = [
'char_array',
]
+exe_preproc_kw = {}
+
+# ecpg leaks memory like no tomorrow; silence AddressSanitizer
+if get_option('b_sanitize').contains('address')
+ exe_preproc_kw += {
+ 'env': {
+ 'ASAN_OPTIONS': 'detect_leaks=0'
+ }
+ }
+endif
+
foreach pgc_file : pgc_files
exe_input = custom_target('@0@.c'.format(pgc_file),
input: '@0@.pgc'.format(pgc_file),
@@ -13,6 +24,7 @@ foreach pgc_file : pgc_files
ecpg_preproc_test_command_end,
install: false,
build_by_default: false,
+ kwargs: exe_preproc_kw,
)
ecpg_test_dependencies += executable(pgc_file,
diff --git a/src/interfaces/ecpg/test/meson.build b/src/interfaces/ecpg/test/meson.build
index b7a3fb4e0e..621e76bd88 100644
--- a/src/interfaces/ecpg/test/meson.build
+++ b/src/interfaces/ecpg/test/meson.build
@@ -40,6 +40,15 @@ ecpg_preproc_kw = {
'build_by_default': false,
}
+# ecpg leaks memory like no tomorrow; silence AddressSanitizer
+if get_option('b_sanitize').contains('address')
+ ecpg_preproc_kw += {
+ 'env': {
+ 'ASAN_OPTIONS': 'detect_leaks=0'
+ }
+ }
+endif
+
ecpg_preproc_test_command_start = [
ecpg_exe,
'--regression',
--
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]/messages/by-id/dbf77bf7-6e54-ed8a-c4ae-d196eeb664ce@gmail.com).
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]: /messages/by-id/dbf77bf7-6e54-ed8a-c4ae-d196eeb664ce@gmail.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, butI 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 ecpgpreprocessor 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