-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
MDEV-38126 Dead code, race conditions and contentious lock_operations in audit plugin #4449
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
base: main
Are you sure you want to change the base?
Conversation
The release of MySQL 5.7.44 (the last one in the 5.7 series) took place in August 2023. Also, the release of MariaDB 5.5.68 (the last one in the 5.5 series) took place in May 2020. Let us remove some dead code that is related to these, as well as attempts to detect the server version by symbol lookup. The audit plugin will typically be built and distributed together with the current MariaDB server version.
To be able to improve performance, let us move from C90 to C++11, which introduced std::atomic.
Starting with ISO/IEC 14882:1998 (C++98), unused function parameters may be identified simply by omitting their names.
internal_stop_logging: Declared as Atomic_counter<int>
|
|
|
I spotted some more errors: It looks like MDEV-34348 and |
Replace the contention prone mysql_prlock_t with a simpler rw-lock wrapper from InnoDB, and fix several potential race conditions by making consistent use of lock_operations. This will also omit any PERFORMANCE_SCHEMA instrumentation.
|
I could not figure out a solution to the diff --git a/include/mysql/service_logger.h b/include/mysql/service_logger.h
index d11dd4291ae..aa6c34fd755 100644
--- a/include/mysql/service_logger.h
+++ b/include/mysql/service_logger.h
@@ -61,7 +61,7 @@ extern "C" {
typedef struct logger_handle_st LOGGER_HANDLE;
extern struct logger_service_st {
- void (*logger_init_mutexes)();
+ void (*logger_init_mutexes)(void);
LOGGER_HANDLE* (*open)(const char *path,
unsigned long long size_limit,
unsigned int rotations, size_t buffer_size);
diff --git a/plugin/server_audit/server_audit.cc b/plugin/server_audit/server_audit.cc
index b1cc1c08ce2..5972b09319e 100644
--- a/plugin/server_audit/server_audit.cc
+++ b/plugin/server_audit/server_audit.cc
@@ -74,11 +74,13 @@ static void closelog() {}
#include <my_global.h>
#include <my_base.h>
#include <typelib.h>
+#include <string.h>
+extern "C" {
#include <mysql/plugin.h>
#include <mysql/plugin_audit.h>
-#include <string.h>
#include <mysql/service_logger.h>
#include "../../mysys/mysys_priv.h"
+}
#ifndef RTLD_DEFAULT
#define RTLD_DEFAULT NULL
#endifIn any case, I think that the |
logger_init_mutexes(void): Define the function as taking no arguments, to avoid function pointer type mismatch. my_b_flush_io_cache(): runtime error: addition of unsigned offset ... overflowed. Cast to a signed offset.
|
I hope that 5676ecf will not introduce function pointer type mismatch elsewhere. |
Description
The implementation of the audit plugin apparently follows obsolete 1990 version of ISO/IEC 9899, a.k.a. C90. Atomic memory operations were not introduced to the language until 2011. Therefore, a combination of mutexes and volatile variables is being used. This is prone to race conditions that could explain some failures such as MDEV-34074, because no well-defined consistency model is being followed. Elsewhere in the code base, we are making use of features that were introduced in ISO/IEC 14882:2011, such as
std::atomic.Release Notes
The performance of the
server_auditplugin was improved andperformance_schemainstrumentation forSERVER_AUDIT_plugin::lock_operationsremoved.How can this PR be tested?
HammerDB with auditing enabled
Basing the PR against the correct MariaDB version
mainbranch.This depends on some
write_log()code removal in 7251cbc. It should be technically possible to port some of this to an earlier major version (I tried it on 10.11), but that would have to be tested and reviewed separately.PR quality check