Skip to content

Do not require an open connection for MySQL string escaping #1

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

Open
wants to merge 1 commit into
base: use-vio-is-connected
Choose a base branch
from
Open

Do not require an open connection for MySQL string escaping #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
17 changes: 15 additions & 2 deletions ext/mysql2/client.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,12 @@ static ID intern_brackets, intern_merge, intern_merge_bang, intern_new_with_args
#define CONNECTED(wrapper) (wrapper->client->net.pvio != NULL && wrapper->client->net.fd != -1 && VIO_IS_CONNECTED(wrapper))
#endif

#define MYSQL_CLIENT_NOT_CONNECTED_STR "MySQL client is not connected"

#define REQUIRE_CONNECTED(wrapper) \
REQUIRE_INITIALIZED(wrapper) \
if (!CONNECTED(wrapper) && !wrapper->reconnect_enabled) { \
rb_raise(cMysql2Error, "MySQL client is not connected"); \
rb_raise(cMysql2Error, MYSQL_CLIENT_NOT_CONNECTED_STR); \
}

#define REQUIRE_NOT_CONNECTED(wrapper) \
Expand All @@ -50,6 +52,17 @@ static ID intern_brackets, intern_merge, intern_merge_bang, intern_new_with_args
rb_raise(cMysql2Error, "MySQL connection is already open"); \
}

/*
* assert that we've connected at least once by using
* `client->server_version`, which is a string that is initialized to the char*
* server name once the client has connected
*/
#define REQUIRE_CONNECTED_ONCE(wrapper) \
REQUIRE_INITIALIZED(wrapper) \
if (!wrapper->client->server_version) { \
Copy link

Choose a reason for hiding this comment

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

Some others places in client.c use wrapper->server_version, is there a difference with wrapper->client->server_version?

Copy link
Owner Author

Choose a reason for hiding this comment

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

From my precursory look, it appears that wrapper->server_version is used to cache the response of mysql_get_server_version, which is an integer. wrapper->client->server_version should be a string representation of the server version that is set by the mysql client library upon connection.

rb_raise(cMysql2Error, MYSQL_CLIENT_NOT_CONNECTED_STR); \
}

/*
* compatability with mysql-connector-c, where LIBMYSQL_VERSION is the correct
* variable to use, but MYSQL_SERVER_VERSION gives the correct numbers when
Expand Down Expand Up @@ -819,7 +832,7 @@ static VALUE rb_mysql_client_real_escape(VALUE self, VALUE str) {
rb_encoding *conn_enc;
GET_CLIENT(self);

REQUIRE_CONNECTED(wrapper);
REQUIRE_CONNECTED_ONCE(wrapper);
Check_Type(str, T_STRING);
default_internal_enc = rb_default_internal_encoding();
conn_enc = rb_to_encoding(wrapper->encoding);
Expand Down
4 changes: 2 additions & 2 deletions spec/mysql2/client_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -887,11 +887,11 @@ def run_gc
end.not_to raise_error
end

it "should require an open connection" do
it "should not require an open connection" do
Copy link

Choose a reason for hiding this comment

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

Should we change this to allows to escape on a closed connection?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I feel like it's basically the same thing. It's running in the #escape context.

@client.close
expect do
@client.escape ""
end.to raise_error(Mysql2::Error)
end.not_to raise_error
end

context 'when mysql encoding is not utf8' do
Expand Down