Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a workaround for broken MariaDB clients which overwrite SIGPIPE handler #196

Closed
wants to merge 1 commit into from

Conversation

pali
Copy link
Member

@pali pali commented Sep 6, 2023

MariaDB Connector/C 3.0.0 in function mysql_server_init() for non-Windows systems started setting SIGPIPE handler to SIG_IGN. This breaks Perl applications which installed its own SIGPIPE signal handler.

As a workaround use Perl rsignal_save() function to save existing SIGPIPE handler before calling mysql_server_init() and after that restore saved handler via Perl rsignal_restore() function.

Add a test which verifies that DBI->connect() which calls DBD::MariaDB's mariadb_dr_connect() function and which calls mysql_server_init(), does not overwrite $SIG{PIPE} handler set in Perl code.

Fixes: #170
See: http://jira.mariadb.org/browse/CONC-591

…andler

MariaDB Connector/C 3.0.0 in function mysql_server_init() for non-Windows
systems started setting SIGPIPE handler to SIG_IGN. This breaks Perl
applications which installed its own SIGPIPE signal handler.

As a workaround use Perl rsignal_save() function to save existing SIGPIPE
handler before calling mysql_server_init() and after that restore saved
handler via Perl rsignal_restore() function.

Add a test which verifies that DBI->connect() which calls DBD::MariaDB's
mariadb_dr_connect() function and which calls mysql_server_init(), does not
overwrite $SIG{PIPE} handler set in Perl code.

Fixes: perl5-dbi#170
See: http://jira.mariadb.org/browse/CONC-591
Comment on lines +1650 to +1651
* setting SIGPIPE handler to SIG_IGN. This breaks Perl applications which installed its own
* SIGPIPE signal handler. As a workaround use Perl rsignal_save() function to save existing
Copy link

Choose a reason for hiding this comment

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

This also breaks graceful termination of programs using the default signal behavior.

Copy link

@bugfood bugfood left a comment

Choose a reason for hiding this comment

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

Thank you for this. I tested the changes using my test program from #170

I verified that:

  1. Current master (0534b71) still had the same behavior as I reported.
  2. This PR fixes the problem

@pali
Copy link
Member Author

pali commented Sep 7, 2023

Ah, lot of tests are failing. It looks like that also older version of MySQL and MariaDB clients are overwriting SIGPIPE handler. And Perl functions rsignal_save() and rsignal_restore() are now marked as private via __attribute__visibility__("hidden") in Perl header file, so compilation under Perl 5.38 is also failing.

@pali
Copy link
Member Author

pali commented Sep 7, 2023

Ah, this issue is even worse and there is a very good reason why MariaDB and MySQL client libraries ignores SIGPIPE signal. Pity that commit which introduced SIG_IGN in MariaDB client library does not contain any information or reason. I'm going to describe it below.

By default when trying to write to stream-orientated socket (e.g. TCP) at the time when other side has closed the connection then POSIX-compatible system raise SIGPIPE signal to the process. It is somehow expected that POSIX socket based application install its own SIGPIPE handler for handling this socket event.

By default processes do not have SIGPIPE handler installed and so delivering it cause terminate of the process, for Perl it means to immediate terminate without running Perl destructors.

As MySQL and MariaDB libraries communicates over TCP, they by default set SIGPIPE handler to ignore to prevent any crashes and graceful handling of "connection lost" state. They detect connection closed by peer itself and propagate it to mysql_errno() function.

For preventing SIGPIPE signal on TCP write operations there are more options, every one is applicable for different system. On Linux it is needed to use send() function instead of write() with additional MSG_NOSIGNAL flag. On BSD it is needed to call setsockopt() with SO_NOSIGPIPE option. On Windows SIGPIPE is not delivered for TCP-write. And on other POSIX systems can be used pthread_sigmask() to temporarily block SIGPIPE for the thread which is going to send data over TCP and then sigwait() for consuming pending signal (after attempted to send data).

All this has to be implemented in the code which is sending data over TCP, so in the MySQL or MariaDB library. We cannot unblock SIGPIPE in DBI code globally otherwise we are risking crashing of Perl interpreter when connection with server is lost (e.g. network issue or timeout). This issue has to be really handled in the MariaDB library.

The good news is that it looks like that MariaDB has some kind of MSG_NOSIGNAL and SO_NOSIGPIPE support. The bad that SIG_IGN for SIGPIPE is still being installed globally via signal(), instead of thread-localizing SIG_BLOCK for SIGPIPE via pthread_sigmask() and consuming it via sigwait().

I'm going to close this pull request as in this state it does not work. Workaround at DBD::MariaDB layer would mean to save+restore SIGPIPE at beginning like in this code change and then to localize SIG_BLOCK for SIGPIPE via pthread_sigmask() for every mysql_*() function which sends data over MYSQL socket and consuming it via sigwait() after finishing function call.

If somebody is willing to implement that workaround at DBD::MariaDB layer then please contact me. I do not have power for it doing it alone.

@pali pali closed this Sep 7, 2023
@bugfood
Copy link

bugfood commented Sep 7, 2023

Ah, lot of tests are failing. It looks like that also older version of MySQL and MariaDB clients are overwriting SIGPIPE handler. And Perl functions rsignal_save() and rsignal_restore() are now marked as private via __attribute__visibility__("hidden") in Perl header file, so compilation under Perl 5.38 is also failing.

Oh, I wasn't looking at the tests.

My own workaround was to:

# Save the current SIGPIPE handler and restore it after connection.
# https://jira.mariadb.org/browse/CONC-591
# This leaves open a brief interval where SIGPIPE is ignored; we can't do
# anything about that here.
my $saved_sigpipe = $SIG{PIPE};
my $dbh = DBI->connect(...);
$SIG{PIPE} = $saved_sigpipe;

That has worked ok as far as I can tell, so I think using the same approach in lib/DBD/MariaDB.pm would work. There's always going to be a window where SIGPIPE is ignored, and having the signal save/restore happen in the C code ought to make that window a bit smaller, but I don't think the practical difference would be great.

I think it would be safe to safe/restore the signal handler regardless of whether the C connector is expected to change it.

@bugfood
Copy link

bugfood commented Sep 7, 2023

I see I was late in my reply. Disregard my previous comment, I guess.

Thank you for the detailed explanation.

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.

SIGPIPE ignored, resulting in: "Unable to flush stdout: Broken pipe"
2 participants