Fix int overflow if the first server is invalid

[Gonlo2]

This PR solves the problem of integer underflow.

A clear description of the issue

If the first connection is removed, this decreases the variable j by one unit, but since it is an unsigned integer, it causes it to acquire an indeterminate value and thus may cause errors if the code want to access that element.

ProxySQL version

v3.0.2

The steps to reproduce the issue

This error was detected by a manual inspection of the code, so I don't know what could be causing it.

[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 related to integer underflow within the server management logic of ProxySQL. By modifying a loop counter's data type, it ensures robust handling of server removal operations, preventing potential crashes or incorrect behavior that could arise from invalid memory access.

Highlights

• Integer Underflow Fix: Corrected a potential integer underflow issue in the purge_mysql_servers_table function within lib/MySQL_HostGroups_Manager.cpp. The loop counter j was changed from unsigned int to int to prevent it from acquiring an indeterminate value when decremented below zero, which could lead to out-of-bounds access.

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. ↩

[renecannao]

Can one of the admins verify this patch?

[gemini-code-assist]

Code Review

This pull request aims to fix an integer underflow bug in the purge_mysql_servers_table function. The loop counter j was an unsigned int and could underflow when decremented, leading to incorrect behavior. The proposed fix of changing j to a signed int is a step in the right direction, but it introduces a subtle bug related to signed/unsigned integer comparison in the loop condition. My review provides a corrected version of the fix to ensure the loop behaves as expected, and also suggests an alternative, more robust implementation pattern for this kind of loop.

[gemini-code-assist]

This change correctly identifies the integer underflow issue with j, but the fix introduces a new potential bug. When comparing a signed int (j) with an unsigned int (myhgc->mysrvs->servers->len), the signed integer is promoted to unsigned. If j becomes -1 (after j-- when j was 0), it will be converted to UINT_MAX. The loop condition UINT_MAX < len will then be false, causing the loop to terminate prematurely.

A safer fix is to cast the length to a signed integer to ensure correct comparison. This avoids the signed/unsigned comparison issue.

Alternatively, a more robust and common pattern for removing items from a collection while iterating is to loop backwards. This avoids the need to manually adjust the loop counter (j--). For example:

for (int j = (int)myhgc->mysrvs->servers->len - 1; j >= 0; j--) {
    mysrvc=myhgc->mysrvs->idx(j);
    if (mysrvc->get_status() == MYSQL_SERVER_STATUS_OFFLINE_HARD) {
        if (mysrvc->ConnectionsUsed->conns_length()==0 && mysrvc->ConnectionsFree->conns_length()==0) {
            // no more connections for OFFLINE_HARD server, removing it
            mysrvc=(MySrvC *)myhgc->mysrvs->servers->remove_index_fast(j);
            delete mysrvc;
        }
    }
}

This would require changing the loop body to remove the j-- statement.

for (int j=0; j < (int)myhgc->mysrvs->servers->len; j++) {

[Gonlo2]

No underflow bug occurs because if the variable is decremented, it is incremented before the next comparison, so it has a safe value for an unsigned int before it is promoted in the comparison. It's not my favorite way of doing this, but decrementing the variable in order to increment it later is a pattern that is repeated in several places in the existing code.

In any case, traversing the array in reverse order simplifies the code, so I think it's fine.

[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