Skip to content

Implement mysqlnd_set_persistent_string #7371

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Aug 20, 2021
Merged

Implement mysqlnd_set_persistent_string #7371

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6101506
Implement mysqlnd_set_persistent_string
kamil-tekiela Aug 13, 2021
ada3c56
memcpy to new function
kamil-tekiela Aug 17, 2021
f2f31c7
Add assert
kamil-tekiela Aug 17, 2021
20c0f4e
Revert "memcpy to new function"
kamil-tekiela Aug 18, 2021
49841b6
password
kamil-tekiela Aug 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions ext/mysqlnd/mysqlnd_alloc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,4 +83,16 @@ static inline void mysqlnd_set_string(MYSQLND_STRING *buf, const char *string, s
}
}

static inline void mysqlnd_set_persistent_string(MYSQLND_STRING *buf, const char *string, size_t len, bool persistent) {
if (buf->s) {
mnd_pefree(buf->s, persistent);
buf->s = NULL;
buf->l = 0;
}
if (string) {
buf->s = mnd_pestrndup(string, len, persistent);
buf->l = len;
}
}

#endif /* MYSQLND_ALLOC_H */
21 changes: 5 additions & 16 deletions ext/mysqlnd/mysqlnd_auth.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,10 +99,7 @@ mysqlnd_run_authentication(
switch_to_auth_protocol = NULL;
switch_to_auth_protocol_len = 0;

if (conn->authentication_plugin_data.s) {
mnd_pefree(conn->authentication_plugin_data.s, conn->persistent);
conn->authentication_plugin_data.s = NULL;
}
mysqlnd_set_persistent_string(&conn->authentication_plugin_data, NULL, 0, conn->persistent);
conn->authentication_plugin_data.l = plugin_data_len;
conn->authentication_plugin_data.s = mnd_pemalloc(conn->authentication_plugin_data.l, conn->persistent);
memcpy(conn->authentication_plugin_data.s, plugin_data, plugin_data_len);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure If I can change this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pemalloc+memcpy is the same as pestrndup, so changing this should be fine.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I tried it but I get warnings because it is zend_uchar. I am not sure what is going on with this assignment so I will roll back this one change.

Expand Down Expand Up @@ -490,24 +487,16 @@ mysqlnd_auth_change_user(MYSQLND_CONN_DATA * const conn,
}
}
if (ret == PASS) {
char * tmp = NULL;
/* if we get conn->username as parameter and then we first free it, then estrndup it, we will crash */
tmp = mnd_pestrndup(user, user_len, conn->persistent);
if (conn->username.s) {
mnd_pefree(conn->username.s, conn->persistent);
}
conn->username.s = tmp;
ZEND_ASSERT(conn->username.s != user && conn->password.s != passwd);
mysqlnd_set_persistent_string(&conn->username, user, user_len, conn->persistent);

tmp = mnd_pestrdup(passwd, conn->persistent);
char * tmp = mnd_pestrdup(passwd, conn->persistent);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the conn->password assignment be changed as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the non-N version. I wasn't sure if I can touch this given that it is a variable-length assignment and the previous code didn't set the .l property anywhere. Is it safe to use strlen() here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pestrdup = pestrndup + strlen basically. You make a good point on this not assigning .l though (also applies to username assignment above), which seems suspect to me. I assume the length just never gets used?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed there is passwd_len in the signature, so I used it. Is this ok?

I grepped for conn->password but I can't see any usage of this property. To be frank, I don't understand what is the purpose of these properties on the conn object. The connection seems to happen before they are saved. The change_user method also uses the values from the signature and then copies the strings.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Let's land this patch first and then remove them. I could see how they might be useful for a hypothetical "automatic reconnect" functionality, but given how such a functionality doesn't exist...

if (conn->password.s) {
mnd_pefree(conn->password.s, conn->persistent);
}
conn->password.s = tmp;

if (conn->last_message.s) {
mnd_efree(conn->last_message.s);
conn->last_message.s = NULL;
}
mysqlnd_set_string(&conn->last_message, NULL, 0);
UPSERT_STATUS_RESET(conn->upsert_status);
/* set charset for old servers */
if (conn->m->get_server_version(conn) < 50123) {
Expand Down
6 changes: 1 addition & 5 deletions ext/mysqlnd/mysqlnd_commands.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,11 +107,7 @@ MYSQLND_METHOD(mysqlnd_command, init_db)(MYSQLND_CONN_DATA * const conn, const M
*/
UPSERT_STATUS_SET_AFFECTED_ROWS_TO_ERROR(conn->upsert_status);
if (ret == PASS) {
if (conn->connect_or_select_db.s) {
mnd_pefree(conn->connect_or_select_db.s, conn->persistent);
}
conn->connect_or_select_db.s = mnd_pestrndup(db.s, db.l, conn->persistent);
conn->connect_or_select_db.l = db.l;
mysqlnd_set_persistent_string(&conn->connect_or_select_db, db.s, db.l, conn->persistent);
}

DBG_RETURN(ret);
Expand Down
61 changes: 15 additions & 46 deletions ext/mysqlnd/mysqlnd_connection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,31 +282,14 @@ MYSQLND_METHOD(mysqlnd_conn_data, free_contents)(MYSQLND_CONN_DATA * conn)

DBG_INF("Freeing memory of members");

if (conn->hostname.s) {
mnd_pefree(conn->hostname.s, pers);
conn->hostname.s = NULL;
}
if (conn->username.s) {
mnd_pefree(conn->username.s, pers);
conn->username.s = NULL;
}
if (conn->password.s) {
mnd_pefree(conn->password.s, pers);
conn->password.s = NULL;
}
if (conn->connect_or_select_db.s) {
mnd_pefree(conn->connect_or_select_db.s, pers);
conn->connect_or_select_db.s = NULL;
}
if (conn->unix_socket.s) {
mnd_pefree(conn->unix_socket.s, pers);
conn->unix_socket.s = NULL;
}
mysqlnd_set_persistent_string(&conn->hostname, NULL, 0, pers);
mysqlnd_set_persistent_string(&conn->username, NULL, 0, pers);
mysqlnd_set_persistent_string(&conn->password, NULL, 0, pers);
mysqlnd_set_persistent_string(&conn->connect_or_select_db, NULL, 0, pers);
mysqlnd_set_persistent_string(&conn->unix_socket, NULL, 0, pers);
DBG_INF_FMT("scheme=%s", conn->scheme.s);
if (conn->scheme.s) {
mnd_pefree(conn->scheme.s, pers);
conn->scheme.s = NULL;
}
mysqlnd_set_persistent_string(&conn->scheme, NULL, 0, pers);

if (conn->server_version) {
mnd_pefree(conn->server_version, pers);
conn->server_version = NULL;
Expand All @@ -315,14 +298,8 @@ MYSQLND_METHOD(mysqlnd_conn_data, free_contents)(MYSQLND_CONN_DATA * conn)
mnd_pefree(conn->host_info, pers);
conn->host_info = NULL;
}
if (conn->authentication_plugin_data.s) {
mnd_pefree(conn->authentication_plugin_data.s, pers);
conn->authentication_plugin_data.s = NULL;
}
if (conn->last_message.s) {
mnd_efree(conn->last_message.s);
conn->last_message.s = NULL;
}
mysqlnd_set_persistent_string(&conn->authentication_plugin_data, NULL, 0, pers);
mysqlnd_set_string(&conn->last_message, NULL, 0);

conn->charset = NULL;
conn->greet_charset = NULL;
Expand Down Expand Up @@ -406,10 +383,7 @@ MYSQLND_METHOD(mysqlnd_conn_data, end_psession)(MYSQLND_CONN_DATA * conn)
conn->current_result->m.free_result(conn->current_result, TRUE);
conn->current_result = NULL;
}
if (conn->last_message.s) {
mnd_efree(conn->last_message.s);
conn->last_message.s = NULL;
}
mysqlnd_set_string(&conn->last_message, NULL, 0);
conn->error_info = &conn->error_info_impl;
DBG_RETURN(PASS);
}
Expand Down Expand Up @@ -681,8 +655,7 @@ MYSQLND_METHOD(mysqlnd_conn_data, connect)(MYSQLND_CONN_DATA * conn,
pfc->data->compressed = mysql_flags & CLIENT_COMPRESS? TRUE:FALSE;


conn->scheme.s = mnd_pestrndup(transport.s, transport.l, conn->persistent);
conn->scheme.l = transport.l;
mysqlnd_set_persistent_string(&conn->scheme, transport.s, transport.l, conn->persistent);
if (transport.s) {
mnd_sprintf_free(transport.s);
transport.s = NULL;
Expand All @@ -692,17 +665,13 @@ MYSQLND_METHOD(mysqlnd_conn_data, connect)(MYSQLND_CONN_DATA * conn,
goto err; /* OOM */
}

conn->username.l = username.l;
conn->username.s = mnd_pestrndup(username.s, conn->username.l, conn->persistent);
conn->password.l = password.l;
conn->password.s = mnd_pestrndup(password.s, conn->password.l, conn->persistent);
mysqlnd_set_persistent_string(&conn->username, username.s, username.l, conn->persistent);
mysqlnd_set_persistent_string(&conn->password, username.s, password.l, conn->persistent);
conn->port = port;
conn->connect_or_select_db.l = database.l;
conn->connect_or_select_db.s = mnd_pestrndup(database.s, conn->connect_or_select_db.l, conn->persistent);
mysqlnd_set_persistent_string(&conn->connect_or_select_db, database.s, database.l, conn->persistent);

if (!unix_socket && !named_pipe) {
conn->hostname.s = mnd_pestrndup(hostname.s, hostname.l, conn->persistent);
conn->hostname.l = hostname.l;
mysqlnd_set_persistent_string(&conn->hostname, hostname.s, hostname.l, conn->persistent);
{
char *p;
mnd_sprintf(&p, 0, "%s via TCP/IP", conn->hostname.s);
Expand Down
10 changes: 2 additions & 8 deletions ext/mysqlnd/mysqlnd_wireprotocol.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1127,10 +1127,7 @@ void php_mysqlnd_rset_header_free_mem(void * _packet)
{
MYSQLND_PACKET_RSET_HEADER *p= (MYSQLND_PACKET_RSET_HEADER *) _packet;
DBG_ENTER("php_mysqlnd_rset_header_free_mem");
if (p->info_or_local_file.s) {
mnd_efree(p->info_or_local_file.s);
p->info_or_local_file.s = NULL;
}
mysqlnd_set_string(&p->info_or_local_file, NULL, 0);
DBG_VOID_RETURN;
}
/* }}} */
Expand Down Expand Up @@ -1764,10 +1761,7 @@ static
void php_mysqlnd_stats_free_mem(void * _packet)
{
MYSQLND_PACKET_STATS *p= (MYSQLND_PACKET_STATS *) _packet;
if (p->message.s) {
mnd_efree(p->message.s);
p->message.s = NULL;
}
mysqlnd_set_string(&p->message, NULL, 0);
}
/* }}} */

Expand Down