hhvm icon indicating copy to clipboard operation
hhvm copied to clipboard

Consistently check connection validity in AsyncMysqlConnection

Open mszabo-wikia opened this issue 5 months ago • 6 comments

The Squangle connection pointer wrapped by AsyncMysqlConnection may be nullptr if the connection was closed or is currently busy waiting for the result of an async query. Most code paths already call either verifyValidConnection() to raise an appropriate Hack exception or explicitly check for and handle a null backing connection, but Query::toString__FOR_DEBUGGING_ONLY() and the SSL-related getters from D33663743 do not, which can lead to segfaults.

Slightly simplified reproducer from #8678:

use namespace HH\Lib\SQL;

<<__EntryPoint>>
async function main_async(): Awaitable<void> {
    // connection parameters as needed
    $async_conn = await AsyncMysqlClient::connect('127.0.0.1', 3306, 'foo', 'root', 'pass');
    concurrent {
        await func_async($async_conn, new SQL\Query('SELECT %s', 'something'));
        await func_async($async_conn, new SQL\Query('SELECT %s', 'something'));
    }
}

async function func_async(AsyncMysqlConnection $asyncMysql, SQL\Query $query): Awaitable<void> {
    $query->toString__FOR_DEBUGGING_ONLY($asyncMysql);
    await $asyncMysql->queryf('SELECT %s', 'something');
}

and for (get|is)Ssl.*:

use namespace HH\Lib\SQL;

<<__EntryPoint>>
async function main_async(): Awaitable<void> {
    $async_conn = await AsyncMysqlClient::connect('127.0.0.1', 3306, 'foo', 'root', 'wikia123456');
    $async_conn->close();

    var_dump($async_conn->getSslCertCn());
}

Call verifyValidConnection() in all these cases, as raising an appropriate exception (e.g. closed/busy connection) seems appropriate here.

Fixes #8678

mszabo-wikia avatar Jun 25 '25 00:06 mszabo-wikia

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. (Because this pull request was imported automatically, there will not be any future comments.)

facebook-github-bot avatar Jun 25 '25 00:06 facebook-github-bot

This code looks sensible to me, but I'm slightly nervous about changing C++ in core DB code when I have less context on the C++ components. I'm trying to find a reviewer.

Wilfred avatar Aug 05 '25 09:08 Wilfred

Thanks! Happy to add tests for this if the CI has a MySQL instance available—I see ext_mysql has tests that look for connection parameters in MYSQL_TEST_* environment variables.

mszabo-wikia avatar Aug 05 '25 10:08 mszabo-wikia

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot avatar Oct 06 '25 10:10 facebook-github-bot

@facebook-github-bot has imported this pull request. If you are a Meta employee, you can view this in D77272245. (Because this pull request was imported automatically, there will not be any future comments.)

meta-codesync[bot] avatar Oct 06 '25 11:10 meta-codesync[bot]

@mszabo-wikia has updated the pull request. You must reimport the pull request before landing.

facebook-github-bot avatar Oct 06 '25 11:10 facebook-github-bot