Skip to content

Fix walreciever crash with SIGABRT - #1979

Open
HustonMmmavr wants to merge 2 commits into
apache:mainfrom
HustonMmmavr:issue_1978
Open

HustonMmmavr wants to merge 2 commits into
apache:mainfrom
HustonMmmavr:issue_1978

Conversation

@HustonMmmavr

Copy link
Copy Markdown
Contributor

Freeing of palloc'd memory leads to SIGABRT of walreciever process. The patch fixes the problem

Closes #1978

Type of Change

  • Bug fix (non-breaking change)
  • New feature (non-breaking change)
  • Breaking change (fix or feature with breaking changes)
  • Documentation update

Test Plan

  • Unit tests added/updated
  • Integration tests added/updated
  • Passed make installcheck
  • Passed make -C src/test installcheck-cbdb-parallel

Impact

Performance:

User-facing changes:

Dependencies:

Checklist

Additional Context

CI Skip Instructions


Freeing of palloc'd memory leads to SIGABRT of walreciever process.
The patch fixes the problem

Closes apache#1978

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @HustonMmmavr welcome!🎊 Thanks for taking the effort to make our project better! 🙌 Keep making such awesome contributions!

@leborchuk

Copy link
Copy Markdown
Contributor

Hi! Thank you for your contribution!

Yep, the issue is that we compiled frontend code to backend one.

But I have objections with the implementation.

The main question is why to compile frontend to backend? Here we try to fix the consequences of this. But changed only one place (in a code), while the other places still have the same issue. And next time we generate coredump with other line from scram_free. So I'd propose:
try to change src/backend/replication/Makefile:33 and build libpqwalreceiver as a shared module linking -lpq, as in upstream (https://github.com/postgres/postgres/tree/master) does. It looks like a clean revert — libpqwalreceiver.c:26 includes only libpq-fe.h, no internals, and the Makefile still carries SHLIB_LINK_INTERNAL = $(libpq) and SHLIB_PREREQS = submake-libpq. That removes the entire hazard class from the replication path instead of one instance of it.

A copy of libpq compiled into the backend needed to support QD-QE communication. But walreceiver is not part of that communication. So it all looks like a bad decision somewhere in the past.

#ifndef FRONTEND
#include "utils/palloc.h"
#endif
/* Cloudberry end */

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but I suggest to remove Cloudberry, Cloudberry end.

Comment on lines +166 to +169
#ifndef FRONTEND
state->password = strdup(prep_password);
pfree(prep_password);
#endif

@Vlasdislav Vlasdislav Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#ifndef FRONTEND
state->password = strdup(prep_password);
pfree(prep_password);
#endif
#ifndef FRONTEND
state->password = strdup(prep_password);
pfree(prep_password);
#else
state->password = prep_password;
#endif

@@ -147,6 +154,21 @@ scram_init(PGconn *conn,
}
state->password = prep_password;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HustonMmmavr

Copy link
Copy Markdown
Contributor Author

Hi! Thank you for your contribution!

Yep, the issue is that we compiled frontend code to backend one.

But I have objections with the implementation.

The main question is why to compile frontend to backend? Here we try to fix the consequences of this. But changed only one place (in a code), while the other places still have the same issue. And next time we generate coredump with other line from scram_free. So I'd propose: try to change src/backend/replication/Makefile:33 and build libpqwalreceiver as a shared module linking -lpq, as in upstream (https://github.com/postgres/postgres/tree/master) does. It looks like a clean revert — libpqwalreceiver.c:26 includes only libpq-fe.h, no internals, and the Makefile still carries SHLIB_LINK_INTERNAL = $(libpq) and SHLIB_PREREQS = submake-libpq. That removes the entire hazard class from the replication path instead of one instance of it.

A copy of libpq compiled into the backend needed to support QD-QE communication. But walreceiver is not part of that communication. So it all looks like a bad decision somewhere in the past.

Hello! @leborchuk Thank you for the proposal!
We've met the problem with SIGABRT after incorrect freeing of palloc'd password memory during PolarDB 11 testing,
Polar11 like postgres compiles libpqwalreceiver as standalone shared object. At polar11 the problem goes from symbol resolution - libpqwalreceiver used symbols from postgres binary instead of linked libpq. After some research I've found this commit at cloudberry, which solves problem with symbol resolution in shared objects, by making libpq symbols inside postgres binary unexported. So compiling libpqwalreceiver as shared object seems a better way to solve the problem. Locally I've built cloudberry with shared libpqwalreceiver.so (it requires additional changes like init libpqwalreceiver with load_file) and there is no SIGABRT. ASAP I'll cleanup the new version of patch, should I open new PR or perform a commit right here?

@leborchuk

Copy link
Copy Markdown
Contributor

Hi! Thank you for your contribution!
Yep, the issue is that we compiled frontend code to backend one.
But I have objections with the implementation.
The main question is why to compile frontend to backend? Here we try to fix the consequences of this. But changed only one place (in a code), while the other places still have the same issue. And next time we generate coredump with other line from scram_free. So I'd propose: try to change src/backend/replication/Makefile:33 and build libpqwalreceiver as a shared module linking -lpq, as in upstream (https://github.com/postgres/postgres/tree/master) does. It looks like a clean revert — libpqwalreceiver.c:26 includes only libpq-fe.h, no internals, and the Makefile still carries SHLIB_LINK_INTERNAL = $(libpq) and SHLIB_PREREQS = submake-libpq. That removes the entire hazard class from the replication path instead of one instance of it.
A copy of libpq compiled into the backend needed to support QD-QE communication. But walreceiver is not part of that communication. So it all looks like a bad decision somewhere in the past.

Hello! @leborchuk Thank you for the proposal! We've met the problem with SIGABRT after incorrect freeing of palloc'd password memory during PolarDB 11 testing, Polar11 like postgres compiles libpqwalreceiver as standalone shared object. At polar11 the problem goes from symbol resolution - libpqwalreceiver used symbols from postgres binary instead of linked libpq. After some research I've found this commit at cloudberry, which solves problem with symbol resolution in shared objects, by making libpq symbols inside postgres binary unexported. So compiling libpqwalreceiver as shared object seems a better way to solve the problem. Locally I've built cloudberry with shared libpqwalreceiver.so (it requires additional changes like init libpqwalreceiver with load_file) and there is no SIGABRT. ASAP I'll cleanup the new version of patch, should I open new PR or perform a commit right here?

Feel free to do whatever suits you.

Since you have existing project, you could cherry-pick original fix from your repo without changes (for example commit int this PR), and then add additional fix to improve it. (2 commits in 1 PR). After discussion we could merge both of them, and you could take newly created commit back to your project (by cherry-picking).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Walreciever crashes with SIGABRT for user with scram-sha-256 password

4 participants