-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Consistently check connection validity in AsyncMysqlConnection #9621
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: master
Are you sure you want to change the base?
Conversation
@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.) |
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. |
Thanks! Happy to add tests for this if the CI has a MySQL instance available—I see |
0ca9e6e
to
18c4f3d
Compare
@mszabo-wikia has updated the pull request. You must reimport the pull request before landing. |
@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.) |
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 facebook#8678: ```hack 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'); $async_conn->close(); var_dump($async_conn->getSslCertCn()); } async function func_async(AsyncMysqlConnection $asyncMysql, SQL\Query $query): Awaitable<void> { $query->toString__FOR_DEBUGGING_ONLY($asyncMysql); var_dump($asyncMysql->getSslCertCn()); await $asyncMysql->queryf('SELECT %s', 'something'); } ``` and for `(get|is)Ssl.*`: ```hack 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 facebook#8678
18c4f3d
to
bb4386b
Compare
@mszabo-wikia has updated the pull request. You must reimport the pull request before landing. |
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:
and for
(get|is)Ssl.*
:Call verifyValidConnection() in all these cases, as raising an appropriate exception (e.g. closed/busy connection) seems appropriate here.
Fixes #8678