diff --git a/CHANGELOG.md b/CHANGELOG.md index b8166f1f..7daafe85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,7 @@ ## (unreleased) +* Raise a login error after `dbopen` returns, not from inside FreeTDS. A failed connection attempt no longer leaks the half-open connection (70 KB to 150 KB of native memory each time). Close the connection before the error when the server negotiates a TDS version older than 7.3. + ## 3.4.1 * Register the C globals that hold Ruby classes and modules with the GC, so raising `TinyTds::Error` after GC compaction no longer crashes with a segmentation fault. Fixes #608. diff --git a/ext/tiny_tds/client.c b/ext/tiny_tds/client.c index 4e84efc1..f857d576 100644 --- a/ext/tiny_tds/client.c +++ b/ext/tiny_tds/client.c @@ -66,19 +66,9 @@ static const rb_data_type_t tinytds_client_wrapper_type = { // Lib Backend (Helpers) -VALUE rb_tinytds_raise_error(DBPROCESS *dbproc, tinytds_errordata error) +static VALUE rb_tinytds_new_error(tinytds_errordata error) { - VALUE e; - GET_CLIENT_USERDATA(dbproc); - - if (error.cancel && !dbdead(dbproc) && userdata && !userdata->closed) { - userdata->dbsqlok_sent = 1; - dbsqlok(dbproc); - userdata->dbcancel_sent = 1; - dbcancel(dbproc); - } - - e = rb_exc_new2(cTinyTdsError, error.error); + VALUE e = rb_exc_new2(cTinyTdsError, error.error); rb_funcall(e, intern_source_eql, 1, rb_str_new2(error.source)); if (error.severity) { @@ -93,6 +83,23 @@ VALUE rb_tinytds_raise_error(DBPROCESS *dbproc, tinytds_errordata error) rb_funcall(e, intern_os_error_number_eql, 1, INT2FIX(error.oserr)); } + return e; +} + +VALUE rb_tinytds_raise_error(DBPROCESS *dbproc, tinytds_errordata error) +{ + VALUE e; + GET_CLIENT_USERDATA(dbproc); + + if (error.cancel && !dbdead(dbproc) && userdata && !userdata->closed) { + userdata->dbsqlok_sent = 1; + dbsqlok(dbproc); + userdata->dbcancel_sent = 1; + dbcancel(dbproc); + } + + e = rb_tinytds_new_error(error); + if (error.severity <= 10 && error.is_message) { VALUE message_handler = userdata && userdata->message_handler ? userdata->message_handler : Qnil; @@ -121,6 +128,34 @@ static void push_userdata_error(tinytds_client_userdata *userdata, tinytds_error userdata->nonblocking_errors_length++; } +/* +dbopen() calls the handlers during the login, before the DBPROCESS has its userdata. +A raise at that point longjmps out of FreeTDS and leaks the half-open connection. +Thus the handlers keep the first error here, and rb_tinytds_connect() raises it after dbopen(). +dbopen() runs with the GVL held, so only one login is in progress at a time. +*/ +typedef struct { + int occurred; + tinytds_errordata error; +} tinytds_login_error; + +static tinytds_login_error *pending_login_error = NULL; + +static int defer_login_error(tinytds_client_userdata *userdata, tinytds_errordata error) +{ + if (userdata || !pending_login_error) { + return 0; + } + + // Ignore info messages: the login does not pass them to the message handler. + if (!error.is_message && !pending_login_error->occurred) { + pending_login_error->error = error; + pending_login_error->occurred = 1; + } + + return 1; +} + int tinytds_err_handler(DBPROCESS *dbproc, int severity, int dberr, int oserr, char *dberrstr, char *oserrstr) { static const char *source = "error"; @@ -185,6 +220,10 @@ int tinytds_err_handler(DBPROCESS *dbproc, int severity, int dberr, int oserr, c strncpy(error_data.error, dberrstr, ERROR_MSG_SIZE); strncpy(error_data.source, source, ERROR_MSG_SIZE); + if (defer_login_error(userdata, error_data)) { + return return_value; + } + /* When in non-blocking mode we need to store the exception data to throw it once the blocking call returns, otherwise we will segfault ruby since part @@ -222,6 +261,10 @@ int tinytds_msg_handler(DBPROCESS *dbproc, DBINT msgno, int msgstate, int severi strncpy(error_data.error, msgtext, ERROR_MSG_SIZE); strncpy(error_data.source, source, ERROR_MSG_SIZE); + if (defer_login_error(userdata, error_data)) { + return 0; + } + // See tinytds_err_handler() for info about why we do this if (userdata && userdata->nonblocking) { /* @@ -495,10 +538,25 @@ static VALUE rb_tinytds_connect(VALUE self, VALUE opts) DBSETLUTF16(cwrap->login, 0); } - cwrap->client = dbopen(cwrap->login, StringValueCStr(dataserver)); + char *dataserver_name = StringValueCStr(dataserver); + tinytds_login_error login_error = { .occurred = 0 }; + pending_login_error = &login_error; + cwrap->client = dbopen(cwrap->login, dataserver_name); + pending_login_error = NULL; + + if (login_error.occurred) { + if (cwrap->client) { + dbclose(cwrap->client); + cwrap->client = NULL; + } + + rb_exc_raise(rb_tinytds_new_error(login_error.error)); + } if (cwrap->client) { if (dbtds(cwrap->client) < 11) { + dbclose(cwrap->client); + cwrap->client = NULL; rb_raise(cTinyTdsError, "connecting with a TDS version older than 7.3!"); } diff --git a/test/client_test.rb b/test/client_test.rb index 4662f9c7..029bd998 100644 --- a/test/client_test.rb +++ b/test/client_test.rb @@ -186,6 +186,14 @@ class ClientTest < TinyTds::TestCase assert_new_connections_work end + it "raises TinyTds exception when the negotiated TDS version is older than 7.3" do + action = lambda { new_connection(tds_version: "7.1") } + assert_raise_tinytds_error(action) do |e| + assert_match %r{older than 7.3}i, e.message + end + assert_new_connections_work + end + it "raises TinyTds exception with wrong :username" do skip if ENV["CI"] && sqlserver_azure? # Some issue with db_error_number. options = connection_options username: "willnotwork"