From 7bd2e1a052a2cde019b73af1ed59c04e68a68a87 Mon Sep 17 00:00:00 2001 From: Artem Gavrilov Date: Tue, 29 Apr 2025 18:56:46 +0200 Subject: [PATCH 1/3] PG-1465 Add CI workfolw with sanitizers Add CI workflow with address and undefined behaviour santitzers. --- .github/workflows/coverage.yml | 2 +- .github/workflows/psp-reusable.yml | 2 +- .github/workflows/sanitizers.yml | 69 +++++++++++++++++++ ci_scripts/make-build.sh | 35 +++++++--- ci_scripts/make-test.sh | 44 +++++++----- ci_scripts/meson-build.sh | 28 +++++++- ci_scripts/meson-test.sh | 39 ++++++----- ci_scripts/suppressions/lsan.supp | 15 ++++ contrib/pg_tde/documentation/CONTRIBUTING.md | 2 +- .../pg_tde/documentation/docs/contribute.md | 2 +- contrib/pg_tde/t/013_crash_recovery.pl | 4 ++ src/test/recovery/t/012_subtransactions.pl | 4 ++ 12 files changed, 199 insertions(+), 47 deletions(-) create mode 100644 .github/workflows/sanitizers.yml create mode 100644 ci_scripts/suppressions/lsan.supp diff --git a/.github/workflows/coverage.yml b/.github/workflows/coverage.yml index 10b58784bdc7e..28e2369bf12e4 100644 --- a/.github/workflows/coverage.yml +++ b/.github/workflows/coverage.yml @@ -28,7 +28,7 @@ jobs: run: ci_scripts/setup-keyring-servers.sh - name: Test postgres with TDE to generate coverage - run: ci_scripts/make-test.sh --tde-only + run: ci_scripts/make-test.sh tde - name: Collect coverage data run: find . -type f -name "*.c" ! -path '*libkmip*' | xargs -t gcov -abcfu diff --git a/.github/workflows/psp-reusable.yml b/.github/workflows/psp-reusable.yml index e72169af6353a..c363deb437221 100644 --- a/.github/workflows/psp-reusable.yml +++ b/.github/workflows/psp-reusable.yml @@ -71,7 +71,7 @@ jobs: run: src/ci_scripts/setup-keyring-servers.sh - name: Test postgres - run: src/ci_scripts/${{ inputs.build_script }}-test.sh + run: src/ci_scripts/${{ inputs.build_script }}-test.sh all - name: Report on test fail uses: actions/upload-artifact@v4 diff --git a/.github/workflows/sanitizers.yml b/.github/workflows/sanitizers.yml new file mode 100644 index 0000000000000..540b32e67d3ea --- /dev/null +++ b/.github/workflows/sanitizers.yml @@ -0,0 +1,69 @@ +name: Sanitizers +on: + pull_request: + push: + branches: + - TDE_REL_17_STABLE + +env: + CC: clang + LD: clang + UBSAN_OPTIONS: log_path=${{ github.workspace }}/sanitize.log print_suppressions=0 print_stacktrace=1 print_summary=1 halt_on_error=1 + ASAN_OPTIONS: log_path=${{ github.workspace }}/sanitize.log print_suppressions=0 abort_on_error=1 + LSAN_OPTIONS: log_path=${{ github.workspace }}/sanitize.log print_suppressions=0 suppressions=${{ github.workspace }}/ci_scripts/suppressions/lsan.supp + ASAN_SYMBOLIZER_PATH: /usr/bin/llvm-symbolizer-14 + EXTRA_REGRESS_OPTS: "--temp-config=${{ github.workspace }}/test_postgresql.conf" + PGCTLTIMEOUT: 120 + +jobs: + run: + name: Run + runs-on: ubuntu-22.04 + steps: + - name: Clone repository + uses: actions/checkout@v4 + with: + submodules: recursive + + - name: Install dependencies + run: ci_scripts/ubuntu-deps.sh + + - name: Build postgres + run: ci_scripts/make-build.sh sanitize + + - name: Setup kmip and vault + run: ci_scripts/setup-keyring-servers.sh + + - name: Configure test postgres + run: echo 'max_stack_depth=8MB' > test_postgresql.conf + + - name: Run tests pg_tde tests + run: ci_scripts/make-test.sh tde + + - name: Run PG regression tests + run: ci_scripts/make-test.sh server + + - name: Print sanitize logs + if: ${{ !cancelled() }} + run: cat sanitize.log.* + + - name: Report on test fail + uses: actions/upload-artifact@v4 + if: ${{ failure() }} + with: + name: sanitizers-testlog + path: | + build/testrun + contrib/**/log + contrib/**/regression.diffs + contrib/**/regression.out + contrib/**/results + contrib/**/tmp_check + contrib/**/t/results + src/test/**/log + src/test/**/regression.diffs + src/test/**/regression.out + src/test/**/results + src/test/**/tmp_check + sanitize.log.* + retention-days: 3 diff --git a/ci_scripts/make-build.sh b/ci_scripts/make-build.sh index c99e77a051bb1..63b139e805957 100755 --- a/ci_scripts/make-build.sh +++ b/ci_scripts/make-build.sh @@ -1,13 +1,13 @@ #!/bin/bash -ENABLE_COVERAGE= +ARGS= for arg in "$@" do case "$arg" in --enable-coverage) - ENABLE_COVERAGE="--enable-coverage" - shift;; + ARGS +=" --enable-coverage" + ;; esac done @@ -17,10 +17,29 @@ source "$SCRIPT_DIR/env.sh" cd "$SCRIPT_DIR/.." -if [ "$1" = "debugoptimized" ]; then - export CFLAGS="-O2" - export CXXFLAGS="-O2" -fi +case "$1" in + debug) + echo "Building with debug option" + ARGS+=" --enable-cassert" + ;; -./configure --prefix="$INSTALL_DIR" --enable-debug --enable-cassert --enable-tap-tests $ENABLE_COVERAGE + debugoptimized) + echo "Building with debugoptimized option" + export CFLAGS="-O2" + ARGS+=" --enable-cassert" + ;; + + sanitize) + echo "Building with sanitize option" + export CFLAGS="-fsanitize=address -fsanitize=undefined -fno-omit-frame-pointer -fno-inline-functions" + ;; + + *) + echo "Unknown build type: $1" + echo "Please use one of the following: debug, debugoptimized, sanitize" + exit 1 + ;; +esac + +./configure --prefix="$INSTALL_DIR" --enable-debug --enable-tap-tests $ARGS make install-world -j diff --git a/ci_scripts/make-test.sh b/ci_scripts/make-test.sh index 6ce136dd28bca..2cf269fccaa64 100755 --- a/ci_scripts/make-test.sh +++ b/ci_scripts/make-test.sh @@ -1,25 +1,33 @@ #!/bin/bash set -e -TDE_ONLY=0 - -for arg in "$@" -do - case "$arg" in - --tde-only) - TDE_ONLY=1 - shift;; - esac -done SCRIPT_DIR="$(cd -- "$(dirname "$0")" >/dev/null 2>&1; pwd -P)" source "$SCRIPT_DIR/env.sh" -if [ "$TDE_ONLY" -eq 1 ]; -then - cd "$SCRIPT_DIR/../contrib/pg_tde" - make -s check -else - cd "$SCRIPT_DIR/.." - make -s check-world -fi + +case "$1" in + server) + echo "Run server regression tests" + cd "$SCRIPT_DIR/.." + make -s check + ;; + + tde) + echo "Run tde tests" + cd "$SCRIPT_DIR/../contrib/pg_tde" + make -s check + ;; + + all) + echo "Run all tests" + cd "$SCRIPT_DIR/.." + make -s check-world + ;; + + *) + echo "Unknown test suite: $1" + echo "Please use one of the following: server, tde, all" + exit 1 + ;; +esac diff --git a/ci_scripts/meson-build.sh b/ci_scripts/meson-build.sh index e954b04619c09..1cd3145a04b93 100755 --- a/ci_scripts/meson-build.sh +++ b/ci_scripts/meson-build.sh @@ -17,5 +17,31 @@ source "$SCRIPT_DIR/env.sh" cd "$SCRIPT_DIR/.." -meson setup build --prefix "$INSTALL_DIR" --buildtype="$1" -Dcassert=true -Dtap_tests=enabled $ENABLE_COVERAGE +BUILD_TYPE= + +case "$1" in + debug) + echo "Building with debug option" + BUILD_TYPE=$1 + ;; + + debugoptimized) + echo "Building with debugoptimized option" + BUILD_TYPE=$1 + ;; + + sanitize) + echo "Building with sanitize option" + export CFLAGS="-fsanitize=address -fsanitize=undefined -fno-omit-frame-pointer -fno-inline-functions" + BUILD_TYPE=debug + ;; + + *) + echo "Unknown build type: $1" + echo "Please use one of the following: debug, debugoptimized, sanitize" + exit 1 + ;; +esac + +meson setup build --prefix "$INSTALL_DIR" --buildtype="$BUILD_TYPE" -Dcassert=true -Dtap_tests=enabled $ENABLE_COVERAGE cd build && ninja && ninja install diff --git a/ci_scripts/meson-test.sh b/ci_scripts/meson-test.sh index f3f888d31c23f..c9de99dca05f4 100755 --- a/ci_scripts/meson-test.sh +++ b/ci_scripts/meson-test.sh @@ -1,25 +1,32 @@ #!/bin/bash set -e -TDE_ONLY=0 - -for arg in "$@" -do - case "$arg" in - --tde-only) - TDE_ONLY=1 - shift;; - esac -done SCRIPT_DIR="$(cd -- "$(dirname "$0")" >/dev/null 2>&1; pwd -P)" source "$SCRIPT_DIR/env.sh" cd "$SCRIPT_DIR/../build" -if [ "$TDE_ONLY" -eq 1 ]; -then - meson test --suite setup --suite pg_tde -else - meson test -fi + +case "$1" in + server) + echo "Run server regression tests" + meson test --suite setup --suite regress + ;; + + tde) + echo "Run tde tests" + meson test --suite setup --suite pg_tde + ;; + + all) + echo "Run all tests" + meson test + ;; + + *) + echo "Unknown test suite: $1" + echo "Please use one of the following: server, tde, all" + exit 1 + ;; +esac diff --git a/ci_scripts/suppressions/lsan.supp b/ci_scripts/suppressions/lsan.supp new file mode 100644 index 0000000000000..00ab2f110a22c --- /dev/null +++ b/ci_scripts/suppressions/lsan.supp @@ -0,0 +1,15 @@ +leak:save_ps_display_args +leak:initdb.c +leak:fe_memutils.c +leak:fe-exec.c +leak:fe-connect.c +leak:pqexpbuffer.c +leak:strdup +leak:preproc.y +leak:pgtypeslib/common.c +leak:ecpglib/memory.c +leak:kmip.c + +#pg_dump +leak:getSchemaData +leak:dumpDumpableObject diff --git a/contrib/pg_tde/documentation/CONTRIBUTING.md b/contrib/pg_tde/documentation/CONTRIBUTING.md index 9de67c5babf4b..5708d979dee9f 100644 --- a/contrib/pg_tde/documentation/CONTRIBUTING.md +++ b/contrib/pg_tde/documentation/CONTRIBUTING.md @@ -56,7 +56,7 @@ To run the tests, use the following command: ``` source ci_scripts/setup-keyring-servers.sh -ci_scripts/make-test.sh +ci_scripts/make-test.sh all ``` You can run tests on your local machine with whatever operating system you have. After you submit the pull request, we will check your patch on multiple operating systems. diff --git a/contrib/pg_tde/documentation/docs/contribute.md b/contrib/pg_tde/documentation/docs/contribute.md index c728f333f49d0..88c019bf66e40 100644 --- a/contrib/pg_tde/documentation/docs/contribute.md +++ b/contrib/pg_tde/documentation/docs/contribute.md @@ -56,7 +56,7 @@ To run the tests, use the following command: ``` source ci_scripts/setup-keyring-servers.sh -ci_scripts/make-test.sh +ci_scripts/make-test.sh all ``` You can run tests on your local machine with whatever operating system you have. After you submit the pull request, we will check your patch on multiple operating systems. diff --git a/contrib/pg_tde/t/013_crash_recovery.pl b/contrib/pg_tde/t/013_crash_recovery.pl index ffdbf2ab062e6..2048975dd44ba 100644 --- a/contrib/pg_tde/t/013_crash_recovery.pl +++ b/contrib/pg_tde/t/013_crash_recovery.pl @@ -18,6 +18,10 @@ 'postgresql.conf', q{ checkpoint_timeout = 1h shared_preload_libraries = 'pg_tde' + +## Workaround to make tests pass with enabled UBSAN +## https://www.postgresql.org/message-id/flat/1775221.1658699459%40sss.pgh.pa.us#18ac2074d2383f232a20bf8b7b32c526 +max_stack_depth = 8MB }); $node->start; diff --git a/src/test/recovery/t/012_subtransactions.pl b/src/test/recovery/t/012_subtransactions.pl index 2f74bfc9a5af5..8d83c0687c838 100644 --- a/src/test/recovery/t/012_subtransactions.pl +++ b/src/test/recovery/t/012_subtransactions.pl @@ -16,6 +16,10 @@ 'postgresql.conf', qq( max_prepared_transactions = 10 log_checkpoints = true + + ## Workaround to make tests pass with enabled UBSAN + ## https://www.postgresql.org/message-id/flat/1775221.1658699459%40sss.pgh.pa.us#18ac2074d2383f232a20bf8b7b32c526 + max_stack_depth = 8MB )); $node_primary->start; $node_primary->backup('primary_backup'); From 81b687525d3e48962a1f3306d30c2f281d1b0813 Mon Sep 17 00:00:00 2001 From: Artem Gavrilov Date: Tue, 6 May 2025 19:19:57 +0200 Subject: [PATCH 2/3] PG-1465 Give server some time to die in TAP tests Sanitizers increase time needed by server to shut down. That's not a problem itself, but pg_ctl has problem that made this tests flacky. If some child processes still alive at the moment when test tries to start new instance whole test may stuck until timeout will be reached. https://www.postgresql.org/message-id/1709793.1644523137%40sss.pgh.pa.us --- contrib/pg_tde/t/011_unlogged_tables.pl | 1 + contrib/pg_tde/t/012_replication.pl | 1 + contrib/pg_tde/t/013_crash_recovery.pl | 11 +++++++---- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/contrib/pg_tde/t/011_unlogged_tables.pl b/contrib/pg_tde/t/011_unlogged_tables.pl index cdf898c54f845..14b03ce31388c 100644 --- a/contrib/pg_tde/t/011_unlogged_tables.pl +++ b/contrib/pg_tde/t/011_unlogged_tables.pl @@ -31,6 +31,7 @@ PGTDE::append_to_result_file("-- kill -9"); $node->kill9(); +sleep(3); # sanitizers slow down the kill, let server die in peace PGTDE::append_to_result_file("-- server start"); $node->start; diff --git a/contrib/pg_tde/t/012_replication.pl b/contrib/pg_tde/t/012_replication.pl index 82ebf7c0ab3d6..36e3121e894e9 100644 --- a/contrib/pg_tde/t/012_replication.pl +++ b/contrib/pg_tde/t/012_replication.pl @@ -75,6 +75,7 @@ PGTDE::psql($primary, 'postgres', "ALTER SYSTEM SET pg_tde.wal_encrypt = 'on';"); $primary->kill9; +sleep(3); # sanitizers slow down the kill, let server die in peace PGTDE::append_to_result_file("-- primary start"); $primary->start; diff --git a/contrib/pg_tde/t/013_crash_recovery.pl b/contrib/pg_tde/t/013_crash_recovery.pl index 2048975dd44ba..527aa7d369aa7 100644 --- a/contrib/pg_tde/t/013_crash_recovery.pl +++ b/contrib/pg_tde/t/013_crash_recovery.pl @@ -18,10 +18,6 @@ 'postgresql.conf', q{ checkpoint_timeout = 1h shared_preload_libraries = 'pg_tde' - -## Workaround to make tests pass with enabled UBSAN -## https://www.postgresql.org/message-id/flat/1775221.1658699459%40sss.pgh.pa.us#18ac2074d2383f232a20bf8b7b32c526 -max_stack_depth = 8MB }); $node->start; @@ -51,6 +47,7 @@ PGTDE::append_to_result_file("-- kill -9"); $node->kill9; +sleep(3); # sanitizers slow down the kill, let server die in peace PGTDE::append_to_result_file("-- server start"); $node->start; @@ -65,6 +62,8 @@ PGTDE::psql($node, 'postgres', "INSERT INTO test_enc (x) VALUES (3), (4);"); PGTDE::append_to_result_file("-- kill -9"); $node->kill9; +sleep(3); # sanitizers slow down the kill, let server die in peace + PGTDE::append_to_result_file("-- server start"); PGTDE::append_to_result_file( "-- check that pg_tde_save_principal_key_redo hasn't destroyed a WAL key created during the server start" @@ -81,6 +80,8 @@ PGTDE::psql($node, 'postgres', "INSERT INTO test_enc (x) VALUES (5), (6);"); PGTDE::append_to_result_file("-- kill -9"); $node->kill9; +sleep(3); # sanitizers slow down the kill, let server die in peace + PGTDE::append_to_result_file("-- server start"); PGTDE::append_to_result_file( "-- check that the key rotation hasn't destroyed a WAL key created during the server start" @@ -93,6 +94,8 @@ "CREATE TABLE test_enc2 (x int PRIMARY KEY) USING tde_heap;"); PGTDE::append_to_result_file("-- kill -9"); $node->kill9; +sleep(3); # sanitizers slow down the kill, let server die in peace + PGTDE::append_to_result_file("-- server start"); PGTDE::append_to_result_file( "-- check redo of the smgr internal key creation when the key is on disk" From 2fae6b396ee8a9c1b65077b87e2e19ff054c97e0 Mon Sep 17 00:00:00 2001 From: Artem Gavrilov Date: Mon, 5 May 2025 23:55:33 +0200 Subject: [PATCH 3/3] Fix uninitialized pointers Use palloc0_object to initialize tail and head pointers to zero value. --- contrib/pg_tde/src/catalog/tde_keyring.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/pg_tde/src/catalog/tde_keyring.c b/contrib/pg_tde/src/catalog/tde_keyring.c index 6eff577e9af07..13a65e422cd74 100644 --- a/contrib/pg_tde/src/catalog/tde_keyring.c +++ b/contrib/pg_tde/src/catalog/tde_keyring.c @@ -781,7 +781,7 @@ scan_key_provider_file(ProviderScanType scanType, void *scanKey, Oid dbOid) providers_list = lappend(providers_list, keyring); #else if (providers_list == NULL) - providers_list = palloc_object(SimplePtrList); + providers_list = palloc0_object(SimplePtrList); simple_ptr_list_append(providers_list, keyring); #endif }