Avoid usage of deleted connection

[Gonlo2]

This PR fix the usage of a connection pointer to a deleted object.

A clear description of the issue

The handler_minus1_HandleErrorCodes function sometimes calls destroy_MySQL_Connection_From_Pool, causing the pointer of the variable myconn to become invalid.

ProxySQL version

v3.0.2

The steps to reproduce the issue

Call the function that removes the connection. This has been achieved by repeatedly restarting MySQL.

[gemini-code-assist]

Summary of Changes

Hello @Gonlo2, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical bug in the MySQL session handling logic that could lead to crashes or undefined behavior. It specifically targets a scenario where a connection object might be destroyed and recreated, but the local pointer to it is not updated, resulting in attempts to use a stale or invalid memory address. The fix ensures that the connection pointer is always valid before further operations.

Highlights

• Critical Bug Fix: Resolves a use-after-free bug in MySQL_Session::handler() where the myconn pointer could become invalid if destroy_MySQL_Connection_From_Pool was called, but the pointer was subsequently used.
• Connection Pointer Management: Ensures the myconn local variable is updated with the current valid connection from myds->myconn after error handling, preventing operations on a potentially deleted connection object.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request correctly addresses a use-after-free bug by ensuring the myconn pointer is updated after a potential deallocation. The fix is minimal and effective. I've added one suggestion for a potential refactoring that could make the code more robust against similar issues in the future by improving the data flow in the error handling functions. Overall, this is a good fix for a critical issue.

[gemini-code-assist]

Your fix is correct and effectively prevents the use-after-free bug.

However, to make the code more robust and prevent similar issues in the future, we could consider a small refactoring. The functions handler_minus1_GenerateErrorMessage and handler_minus1_HandleBackendConnection are passed a myconn pointer that might be stale.

Instead of passing myconn, these functions could retrieve the connection pointer directly from the MySQL_Data_Stream (myds) argument they already receive. This would ensure they always work with the most up-to-date connection state.

For example, handler_minus1_GenerateErrorMessage could be changed like this:

// from
void MySQL_Session::handler_minus1_GenerateErrorMessage(MySQL_Data_Stream *myds, MySQL_Connection *myconn, bool& wrong_pass) {
    // ...
}

// to
void MySQL_Session::handler_minus1_GenerateErrorMessage(MySQL_Data_Stream *myds, bool& wrong_pass) {
    MySQL_Connection *myconn = myds->myconn;
    // ...
}

A similar change could be applied to handler_minus1_HandleBackendConnection.

This would eliminate the need for the myconn = myds->myconn; line here and make the data flow clearer. What do you think?

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[renecannao]

Can one of the admins verify this patch?