diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d534d0b8..bae5aba8 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -26,7 +26,7 @@ jobs: distribution: "debian:buster" ruby: "2.8" steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: docker login run: echo $GITHUB_TOKEN | docker login ghcr.io --username trilogy --password-stdin env: diff --git a/.github/workflows/macos.yml b/.github/workflows/macos.yml index 4788f14c..c56e5a62 100644 --- a/.github/workflows/macos.yml +++ b/.github/workflows/macos.yml @@ -15,7 +15,7 @@ jobs: matrix: mysql: ["5.7", "8.0"] steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Setup MySQL run: | brew install mysql@${{ matrix.mysql }} @@ -34,7 +34,7 @@ jobs: matrix: mysql: ["5.7", "8.0"] steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@v4 - name: Setup MySQL run: | brew install mysql@${{ matrix.mysql }} diff --git a/CHANGELOG.md b/CHANGELOG.md index f7a1f7ca..9ae4686d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,54 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). ## Unreleased +## 2.7.0 + +### Changed + + - `Trilogy::SyscallError::*` errors now use the standard `Module#===` implementation #143 + - `Trilogy::TimeoutError` no longer inherits from `Errno::ETIMEDOUT` #143 + - Deprecated `Trilogy::ConnectionRefusedError` and `Trilogy::ConnectionResetError`, + replaced by `Trilogy::SyscallError::ECONNREFUSED` and `Trilogy::SyscallError::ECONNRESET` #143 + +## 2.6.1 + +### Fixed + + - Report `EOFError: TRILOGY_CLOSED_CONNECTION` for `SSL_ERROR_ZERO_RETURN` + - `write_timeout` on connection now raises `Trilogy::TimeoutError` (previously it raised `EINPROGRESS`) + - Fix memory leak on failed connections + - Fix memory leak when connecting to unix socket + +## 2.6.0 + +### Changed + + - `TCP_NODELAY` is enabled on all TCP connections #122 + - `Trilogy::EOFError` is now raised for `TRILOGY_CLOSED_CONNECTION` instead + of the generic `Trilogy::QueryError` #118 + - `Trilogy::SyscallError` now inherits `Trilogy::ConnectionError` #118 + +## 2.5.0 + +### Fixed + - Fix build with LibreSSL #73 + - Fix build error on FreeBSD #82 + - Fix Trilogy.new with no arguments #94 + - Fix issues with OpenSSL #95 #112 + - Avoid closing connections that are not connected + - Always close socket on error + - Clear error queue after close + - Clear error queue before each operation to defend against other misbehaving libraries + - Close connection if interrupted by a Ruby timeout #110 + - Correctly cast time of 00:00:00 #97 + +### Added + - Add option to disable multi_result capability #77 + - Add option to validate max_allowed_packet #84 + - Add binary protocol/prepared statement support to the C library #3 + - Cast port option to integer #100 + - Add select_db as an alias for change_db #101 + ## 2.4.1 ### Fixed diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4dea470c..d2e4601d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,7 +1,7 @@ ## Contributing -[fork]: https://github.com/github/trilogy/fork -[pr]: https://github.com/github/trilogy/compare +[fork]: https://github.com/trilogy-libraries/trilogy/fork +[pr]: https://github.com/trilogy-libraries/trilogy/compare Hi there! We're thrilled that you'd like to contribute to this project. Your help is essential for keeping it great. diff --git a/Dockerfile b/Dockerfile index 84076ef0..c1d45ed2 100644 --- a/Dockerfile +++ b/Dockerfile @@ -3,7 +3,7 @@ ARG RUBY_VERSION=3.2 FROM ${DISTRIBUTION} LABEL maintainer="github@github.com" -RUN apt-get update -qq && DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y build-essential ca-certificates wget libssl-dev default-libmysqlclient-dev clang clang-tools valgrind netcat +RUN apt-get update -qq && DEBIAN_FRONTEND=noninteractive apt-get install --no-install-recommends -y build-essential ca-certificates wget libssl-dev default-libmysqlclient-dev clang clang-tools llvm valgrind netcat RUN update-ca-certificates diff --git a/contrib/ruby/Gemfile.lock b/contrib/ruby/Gemfile.lock index 835d5265..bfe51a89 100644 --- a/contrib/ruby/Gemfile.lock +++ b/contrib/ruby/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - trilogy (2.4.1) + trilogy (2.7.0) GEM remote: https://rubygems.org/ @@ -22,3 +22,6 @@ DEPENDENCIES mysql2 rake-compiler (~> 1.0) trilogy! + +BUNDLED WITH + 2.4.12 diff --git a/contrib/ruby/README.md b/contrib/ruby/README.md index 1c01136b..23340e1e 100644 --- a/contrib/ruby/README.md +++ b/contrib/ruby/README.md @@ -27,7 +27,6 @@ $ gem install trilogy ``` ruby client = Trilogy.new(host: "127.0.0.1", port: 3306, username: "root", read_timeout: 2) if client.ping - client.query_options[:database_timezone] = :utc client.change_db "mydb" result = client.query("SELECT id, created_at FROM users LIMIT 10") @@ -63,7 +62,7 @@ bundle exec rake build The official Ruby bindings are inside of the canonical trilogy repository itself. -1. Fork it ( https://github.com/github/trilogy/fork ) +1. Fork it ( https://github.com/trilogy-libraries/trilogy/fork ) 2. Create your feature branch (`git checkout -b my-new-feature`) 3. Commit your changes (`git commit -am 'Add some feature'`) 4. Push to the branch (`git push origin my-new-feature`) diff --git a/contrib/ruby/ext/trilogy-ruby/cext.c b/contrib/ruby/ext/trilogy-ruby/cext.c index 0e7940e7..d189762a 100644 --- a/contrib/ruby/ext/trilogy-ruby/cext.c +++ b/contrib/ruby/ext/trilogy-ruby/cext.c @@ -17,8 +17,8 @@ VALUE Trilogy_CastError; static VALUE Trilogy_BaseConnectionError, Trilogy_ProtocolError, Trilogy_SSLError, Trilogy_QueryError, - Trilogy_ConnectionClosedError, Trilogy_ConnectionRefusedError, Trilogy_ConnectionResetError, - Trilogy_TimeoutError, Trilogy_SyscallError, Trilogy_Result; + Trilogy_ConnectionClosedError, + Trilogy_TimeoutError, Trilogy_SyscallError, Trilogy_Result, Trilogy_EOFError; static ID id_socket, id_host, id_port, id_username, id_password, id_found_rows, id_connect_timeout, id_read_timeout, id_write_timeout, id_keepalive_enabled, id_keepalive_idle, id_keepalive_interval, id_keepalive_count, @@ -43,9 +43,7 @@ static void mark_trilogy(void *ptr) static void free_trilogy(void *ptr) { struct trilogy_ctx *ctx = ptr; - if (ctx->conn.socket != NULL) { - trilogy_free(&ctx->conn); - } + trilogy_free(&ctx->conn); xfree(ptr); } @@ -90,16 +88,26 @@ static struct trilogy_ctx *get_open_ctx(VALUE obj) NORETURN(static void trilogy_syserr_fail_str(int, VALUE)); static void trilogy_syserr_fail_str(int e, VALUE msg) { - if (e == ECONNREFUSED) { - rb_raise(Trilogy_ConnectionRefusedError, "%" PRIsVALUE, msg); - } else if (e == ECONNRESET) { - rb_raise(Trilogy_ConnectionResetError, "%" PRIsVALUE, msg); + if (e == EPIPE) { + // Backwards compatibility: This error message is a bit odd, but includes "TRILOGY_CLOSED_CONNECTION" to match legacy string matching + rb_raise(Trilogy_EOFError, "%" PRIsVALUE ": TRILOGY_CLOSED_CONNECTION: EPIPE", msg); } else { VALUE exc = rb_funcall(Trilogy_SyscallError, id_from_errno, 2, INT2NUM(e), msg); rb_exc_raise(exc); } } +static int trilogy_error_recoverable_p(int rc) +{ + // TRILOGY_OPENSSL_ERR and TRILOGY_SYSERR (which can result from an SSL error) must shut down the socket, as further + // SSL calls would be invalid. + // TRILOGY_ERR, which represents an error message sent to us from the server, is recoverable. + // TRILOGY_MAX_PACKET_EXCEEDED is also recoverable as we do not send data when it occurs. + // For other exceptions we will also close the socket to prevent further use, as the connection is probably in an + // invalid state. + return rc == TRILOGY_ERR || rc == TRILOGY_MAX_PACKET_EXCEEDED; +} + NORETURN(static void handle_trilogy_error(struct trilogy_ctx *, int, const char *, ...)); static void handle_trilogy_error(struct trilogy_ctx *ctx, int rc, const char *msg, ...) { @@ -108,14 +116,20 @@ static void handle_trilogy_error(struct trilogy_ctx *ctx, int rc, const char *ms VALUE rbmsg = rb_vsprintf(msg, args); va_end(args); + if (!trilogy_error_recoverable_p(rc)) { + if (ctx->conn.socket != NULL) { + // trilogy_sock_shutdown may affect errno + int errno_was = errno; + trilogy_sock_shutdown(ctx->conn.socket); + errno = errno_was; + } + } + switch (rc) { case TRILOGY_SYSERR: trilogy_syserr_fail_str(errno, rbmsg); case TRILOGY_TIMEOUT: - if (ctx->conn.socket != NULL) { - trilogy_sock_shutdown(ctx->conn.socket); - } rb_raise(Trilogy_TimeoutError, "%" PRIsVALUE, rbmsg); case TRILOGY_ERR: { @@ -131,11 +145,6 @@ static void handle_trilogy_error(struct trilogy_ctx *ctx, int rc, const char *ms int err_reason = ERR_GET_REASON(ossl_error); trilogy_syserr_fail_str(err_reason, rbmsg); } - // We can't recover from OpenSSL level errors if there's - // an active connection. - if (ctx->conn.socket != NULL) { - trilogy_sock_shutdown(ctx->conn.socket); - } rb_raise(Trilogy_SSLError, "%" PRIsVALUE ": SSL Error: %s", rbmsg, ERR_reason_error_string(ossl_error)); } @@ -143,6 +152,10 @@ static void handle_trilogy_error(struct trilogy_ctx *ctx, int rc, const char *ms rb_raise(Trilogy_BaseConnectionError, "%" PRIsVALUE ": TRILOGY_DNS_ERROR", rbmsg); } + case TRILOGY_CLOSED_CONNECTION: { + rb_raise(Trilogy_EOFError, "%" PRIsVALUE ": TRILOGY_CLOSED_CONNECTION", rbmsg); + } + default: rb_raise(Trilogy_QueryError, "%" PRIsVALUE ": %s", rbmsg, trilogy_error(rc)); } @@ -290,6 +303,7 @@ static int try_connect(struct trilogy_ctx *ctx, trilogy_handshake_t *handshake, int rc = args.rc; if (rc != TRILOGY_OK) { + trilogy_sock_close(sock); return rc; } @@ -297,8 +311,10 @@ static int try_connect(struct trilogy_ctx *ctx, trilogy_handshake_t *handshake, escape the GVL on each wait operation without going through call_without_gvl */ sock->wait_cb = _cb_ruby_wait; rc = trilogy_connect_send_socket(&ctx->conn, sock); - if (rc < 0) + if (rc < 0) { + trilogy_sock_close(sock); return rc; + } while (1) { rc = trilogy_connect_recv(&ctx->conn, handshake); @@ -406,7 +422,7 @@ static void authenticate(struct trilogy_ctx *ctx, trilogy_handshake_t *handshake } } -static VALUE rb_trilogy_initialize(VALUE self, VALUE encoding, VALUE charset, VALUE opts) +static VALUE rb_trilogy_connect(VALUE self, VALUE encoding, VALUE charset, VALUE opts) { struct trilogy_ctx *ctx = get_ctx(self); trilogy_sockopt_t connopt = {0}; @@ -417,7 +433,6 @@ static VALUE rb_trilogy_initialize(VALUE self, VALUE encoding, VALUE charset, VA connopt.encoding = NUM2INT(charset); Check_Type(opts, T_HASH); - rb_ivar_set(self, id_connection_options, opts); if ((val = rb_hash_lookup(opts, ID2SYM(id_ssl_mode))) != Qnil) { Check_Type(val, T_FIXNUM); @@ -559,9 +574,6 @@ static VALUE rb_trilogy_initialize(VALUE self, VALUE encoding, VALUE charset, VA } int rc = try_connect(ctx, &handshake, &connopt); - if (rc == TRILOGY_TIMEOUT) { - rb_raise(Trilogy_TimeoutError, "trilogy_connect_recv"); - } if (rc != TRILOGY_OK) { if (connopt.path) { handle_trilogy_error(ctx, rc, "trilogy_connect - unable to connect to %s", connopt.path); @@ -982,6 +994,10 @@ static VALUE rb_trilogy_close(VALUE self) } } + // We aren't checking or raising errors here (we need close to always close the socket and free the connection), so + // we must clear any SSL errors left in the queue from a read/write. + ERR_clear_error(); + trilogy_free(&ctx->conn); return Qnil; @@ -1081,7 +1097,7 @@ RUBY_FUNC_EXPORTED void Init_cext() VALUE Trilogy = rb_const_get(rb_cObject, rb_intern("Trilogy")); rb_define_alloc_func(Trilogy, allocate_trilogy); - rb_define_private_method(Trilogy, "_initialize", rb_trilogy_initialize, 3); + rb_define_private_method(Trilogy, "_connect", rb_trilogy_connect, 3); rb_define_method(Trilogy, "change_db", rb_trilogy_change_db, 1); rb_define_alias(Trilogy, "select_db", "change_db"); rb_define_method(Trilogy, "query", rb_trilogy_query, 1); @@ -1136,12 +1152,6 @@ RUBY_FUNC_EXPORTED void Init_cext() Trilogy_TimeoutError = rb_const_get(Trilogy, rb_intern("TimeoutError")); rb_global_variable(&Trilogy_TimeoutError); - Trilogy_ConnectionRefusedError = rb_const_get(Trilogy, rb_intern("ConnectionRefusedError")); - rb_global_variable(&Trilogy_ConnectionRefusedError); - - Trilogy_ConnectionResetError = rb_const_get(Trilogy, rb_intern("ConnectionResetError")); - rb_global_variable(&Trilogy_ConnectionResetError); - Trilogy_BaseConnectionError = rb_const_get(Trilogy, rb_intern("BaseConnectionError")); rb_global_variable(&Trilogy_BaseConnectionError); @@ -1157,6 +1167,9 @@ RUBY_FUNC_EXPORTED void Init_cext() Trilogy_CastError = rb_const_get(Trilogy, rb_intern("CastError")); rb_global_variable(&Trilogy_CastError); + Trilogy_EOFError = rb_const_get(Trilogy, rb_intern("EOFError")); + rb_global_variable(&Trilogy_EOFError); + id_socket = rb_intern("socket"); id_host = rb_intern("host"); id_port = rb_intern("port"); diff --git a/contrib/ruby/ext/trilogy-ruby/extconf.rb b/contrib/ruby/ext/trilogy-ruby/extconf.rb index 05ad2aec..f618ef21 100644 --- a/contrib/ruby/ext/trilogy-ruby/extconf.rb +++ b/contrib/ruby/ext/trilogy-ruby/extconf.rb @@ -2,8 +2,12 @@ # concatenate trilogy library sources to allow the compiler to optimise across # source files + +trilogy_src_dir = File.realpath("src", __dir__) File.binwrite("trilogy.c", - Dir["#{__dir__}/src/**/*.c"].map { |src| File.binread(src) }.join) + Dir["#{trilogy_src_dir}/**/*.c"].map { |src| + %{#line 1 "#{src}"\n} + File.binread(src) + }.join) $objs = %w[trilogy.o cast.o cext.o] $CFLAGS << " -I #{__dir__}/inc -std=gnu99 -fvisibility=hidden" diff --git a/contrib/ruby/lib/trilogy.rb b/contrib/ruby/lib/trilogy.rb index 45e2d025..bde8a033 100644 --- a/contrib/ruby/lib/trilogy.rb +++ b/contrib/ruby/lib/trilogy.rb @@ -8,12 +8,14 @@ class Trilogy def initialize(options = {}) - options[:port] = options[:port].to_i if options.key?(:port) + options[:port] = options[:port].to_i if options[:port] mysql_encoding = options[:encoding] || "utf8mb4" encoding = Trilogy::Encoding.find(mysql_encoding) charset = Trilogy::Encoding.charset(mysql_encoding) + @connection_options = options + @connected_host = nil - _initialize(encoding, charset, options) + _connect(encoding, charset, options) end def connection_options diff --git a/contrib/ruby/lib/trilogy/error.rb b/contrib/ruby/lib/trilogy/error.rb index 72569f9e..614d2d40 100644 --- a/contrib/ruby/lib/trilogy/error.rb +++ b/contrib/ruby/lib/trilogy/error.rb @@ -12,7 +12,7 @@ module ConnectionError end # Trilogy may raise various syscall errors, which we treat as Trilogy::Errors. - class SyscallError + module SyscallError ERRORS = {} Errno.constants @@ -20,7 +20,10 @@ class SyscallError .select { |c| c.is_a?(Class) && c < SystemCallError } .each do |c| errno_name = c.to_s.split('::').last - ERRORS[c::Errno] = const_set(errno_name, Class.new(c) { include Trilogy::Error }) + ERRORS[c::Errno] = const_set(errno_name, Class.new(c) { + include Trilogy::ConnectionError + singleton_class.define_method(:===, Module.instance_method(:===)) + }) end ERRORS.freeze @@ -32,6 +35,11 @@ def from_errno(errno, message) end end + ConnectionRefusedError = SyscallError::ECONNREFUSED + deprecate_constant :ConnectionRefusedError + ConnectionResetError = SyscallError::ECONNRESET + deprecate_constant :ConnectionResetError + class BaseError < StandardError include Error @@ -58,21 +66,7 @@ class QueryError < ClientError class CastError < ClientError end - class TimeoutError < Errno::ETIMEDOUT - include ConnectionError - - def initialize(error_message = nil, error_code = nil) - super - @error_code = error_code - end - end - - class ConnectionRefusedError < Errno::ECONNREFUSED - include ConnectionError - end - - class ConnectionResetError < Errno::ECONNRESET - include ConnectionError + class TimeoutError < BaseConnectionError end # DatabaseError was replaced by ProtocolError, but we'll keep it around as an @@ -112,7 +106,13 @@ class SSLError < BaseError include ConnectionError end + # Raised on attempt to use connection which was explicitly closed by the user class ConnectionClosed < IOError include ConnectionError end + + # Occurs when a socket read or write returns EOF or when an operation is + # attempted on a socket which previously encountered an error. + class EOFError < BaseConnectionError + end end diff --git a/contrib/ruby/lib/trilogy/version.rb b/contrib/ruby/lib/trilogy/version.rb index e28d4e45..ef58fd83 100644 --- a/contrib/ruby/lib/trilogy/version.rb +++ b/contrib/ruby/lib/trilogy/version.rb @@ -1,3 +1,3 @@ class Trilogy - VERSION = "2.4.1" + VERSION = "2.7.0" end diff --git a/contrib/ruby/test/client_test.rb b/contrib/ruby/test/client_test.rb index 33ef1fc5..269e2269 100644 --- a/contrib/ruby/test/client_test.rb +++ b/contrib/ruby/test/client_test.rb @@ -42,7 +42,9 @@ def test_trilogy_connect_unix_socket socket = new_tcp_client.query("SHOW VARIABLES LIKE 'socket'").to_a[0][1] - assert File.exist?(socket), "cound not find socket at #{socket}" + if !File.exist?(socket) + skip "cound not find socket at #{socket}" + end client = new_unix_client(socket) refute_nil client @@ -773,6 +775,8 @@ def test_packet_size_lower_than_trilogy_max_packet_len end assert_equal "trilogy_query_send: TRILOGY_MAX_PACKET_EXCEEDED", exception.message + + assert client.ping ensure ensure_closed client end @@ -791,6 +795,8 @@ def test_packet_size_greater_than_trilogy_max_packet_len end assert_equal "trilogy_query_send: TRILOGY_MAX_PACKET_EXCEEDED", exception.message + + assert client.ping ensure ensure_closed client end @@ -809,6 +815,8 @@ def test_configured_max_packet_below_server end assert_equal "trilogy_query_send: TRILOGY_MAX_PACKET_EXCEEDED", exception.message + + assert client.ping ensure ensure_closed client end @@ -833,6 +841,10 @@ def test_configured_max_packet_above_server end refute_match(/TRILOGY_MAX_PACKET_EXCEEDED/, exception.message) + + assert_raises_connection_error do + client.ping + end ensure ensure_closed client end @@ -851,6 +863,8 @@ def test_absolute_maximum_packet_size end assert_equal "trilogy_query_send: TRILOGY_MAX_PACKET_EXCEEDED", exception.message + + assert client.ping ensure ensure_closed client end @@ -998,6 +1012,18 @@ def test_close_terminate_parent_connection end end + def test_discard_closes_connection + client = new_tcp_client + + assert_equal [1], client.query("SELECT 1").to_a.first + + client.discard! + + assert_raises Trilogy::ConnectionClosed do + client.query("SELECT 1") + end + end + def test_discard_doesnt_terminate_parent_connection skip("Fork isn't supported on this platform") unless Process.respond_to?(:fork) @@ -1073,4 +1099,14 @@ def test_connection_options_casting assert client.query("SELECT 1") end + + def test_error_classes_exclusively_match_subclasses + klass = Trilogy::SyscallError::ECONNRESET + assert_operator klass, :===, klass.new + refute_operator klass, :===, Errno::ECONNRESET.new + + assert_operator Errno::ECONNRESET, :===, klass.new + assert_operator SystemCallError, :===, klass.new + assert_operator Trilogy::ConnectionError, :===, klass.new + end end diff --git a/contrib/ruby/test/test_helper.rb b/contrib/ruby/test/test_helper.rb index 8937f07a..e8983b94 100644 --- a/contrib/ruby/test/test_helper.rb +++ b/contrib/ruby/test/test_helper.rb @@ -10,7 +10,14 @@ if GC.respond_to?(:verify_compaction_references) # This method was added in Ruby 3.0.0. Calling it this way asks the GC to # move objects around, helping to find object movement bugs. - GC.verify_compaction_references(double_heap: true, toward: :empty) + if Gem::Version::new(RUBY_VERSION) >= Gem::Version::new("3.2.0") + # double_heap is deprecated and expand_heap is the updated argument. This change + # was introduced in: + # https://github.com/ruby/ruby/commit/a6dd859affc42b667279e513bb94fb75cfb133c1 + GC.verify_compaction_references(expand_heap: true, toward: :empty) + else + GC.verify_compaction_references(double_heap: true, toward: :empty) + end end class TrilogyTest < Minitest::Test @@ -137,8 +144,10 @@ def create_test_table(client) def assert_raises_connection_error(&block) err = assert_raises(Trilogy::Error, &block) - if err.is_a?(Trilogy::QueryError) + if err.is_a?(Trilogy::EOFError) assert_includes err.message, "TRILOGY_CLOSED_CONNECTION" + elsif err.is_a?(Trilogy::SSLError) + assert_includes err.message, "unexpected eof while reading" else assert_instance_of Trilogy::ConnectionResetError, err end diff --git a/docker-compose.yml b/docker-compose.yml index 2d34ab08..96dd91a3 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,6 +1,7 @@ version: "3.5" services: db: + platform: linux/x86_64 image: "mysql:${MYSQL_VERSION}-debian" command: - --sql_mode=NO_ENGINE_SUBSTITUTION diff --git a/inc/trilogy/socket.h b/inc/trilogy/socket.h index 55bb1283..967b5615 100644 --- a/inc/trilogy/socket.h +++ b/inc/trilogy/socket.h @@ -110,6 +110,5 @@ static inline int trilogy_sock_fd(trilogy_sock_t *sock) { return sock->fd_cb(soc trilogy_sock_t *trilogy_sock_new(const trilogy_sockopt_t *opts); int trilogy_sock_resolve(trilogy_sock_t *raw); int trilogy_sock_upgrade_ssl(trilogy_sock_t *raw); -int trilogy_sock_discard(trilogy_sock_t *sock); #endif diff --git a/src/buffer.c b/src/buffer.c index cdb95216..a4c78393 100644 --- a/src/buffer.c +++ b/src/buffer.c @@ -57,4 +57,9 @@ int trilogy_buffer_putc(trilogy_buffer_t *buffer, uint8_t c) return TRILOGY_OK; } -void trilogy_buffer_free(trilogy_buffer_t *buffer) { free(buffer->buff); } +void trilogy_buffer_free(trilogy_buffer_t *buffer) +{ + free(buffer->buff); + buffer->buff = NULL; + buffer->len = buffer->cap = 0; +} diff --git a/src/client.c b/src/client.c index e78abd6e..9a68a05e 100644 --- a/src/client.c +++ b/src/client.c @@ -1,4 +1,3 @@ -#include #include #include "trilogy/client.h" @@ -76,11 +75,6 @@ static int read_packet(trilogy_conn_t *conn) if (nread < 0) { int rc = (int)nread; - if (rc == TRILOGY_SYSERR) { - if (errno == EINTR || errno == EAGAIN) { - return TRILOGY_AGAIN; - } - } return rc; } @@ -162,16 +156,6 @@ int trilogy_flush_writes(trilogy_conn_t *conn) if (bytes < 0) { int rc = (int)bytes; - if (rc == TRILOGY_SYSERR) { - if (errno == EINTR || errno == EAGAIN) { - return TRILOGY_AGAIN; - } - - if (errno == EPIPE) { - return TRILOGY_CLOSED_CONNECTION; - } - } - return rc; } @@ -770,7 +754,7 @@ void trilogy_free(trilogy_conn_t *conn) int trilogy_discard(trilogy_conn_t *conn) { - int rc = trilogy_sock_discard(conn->socket); + int rc = trilogy_sock_shutdown(conn->socket); if (rc == TRILOGY_OK) { trilogy_free(conn); } diff --git a/src/socket.c b/src/socket.c index 90cd0e83..954be5cd 100644 --- a/src/socket.c +++ b/src/socket.c @@ -1,5 +1,6 @@ #include #include +#include #include #include #include @@ -65,7 +66,11 @@ static ssize_t _cb_raw_read(trilogy_sock_t *_sock, void *buf, size_t nread) struct trilogy_sock *sock = (struct trilogy_sock *)_sock; ssize_t data_read = read(sock->fd, buf, nread); if (data_read < 0) { - return (ssize_t)TRILOGY_SYSERR; + if (errno == EINTR || errno == EAGAIN) { + return (ssize_t)TRILOGY_AGAIN; + } else { + return (ssize_t)TRILOGY_SYSERR; + } } return data_read; } @@ -75,6 +80,14 @@ static ssize_t _cb_raw_write(trilogy_sock_t *_sock, const void *buf, size_t nwri struct trilogy_sock *sock = (struct trilogy_sock *)_sock; ssize_t data_written = write(sock->fd, buf, nwrite); if (data_written < 0) { + if (errno == EINTR || errno == EAGAIN) { + return (ssize_t)TRILOGY_AGAIN; + } + + if (errno == EPIPE) { + return (ssize_t)TRILOGY_CLOSED_CONNECTION; + } + return (ssize_t)TRILOGY_SYSERR; } return data_written; @@ -87,8 +100,15 @@ static int _cb_raw_close(trilogy_sock_t *_sock) if (sock->fd != -1) { rc = close(sock->fd); } + if (sock->addr) { - freeaddrinfo(sock->addr); + if (sock->base.opts.hostname == NULL && sock->base.opts.path != NULL) { + /* We created these with calloc so must free them instead of calling freeaddrinfo */ + free(sock->addr->ai_addr); + free(sock->addr); + } else { + freeaddrinfo(sock->addr); + } } free(sock->base.opts.hostname); @@ -109,7 +129,53 @@ static int _cb_raw_close(trilogy_sock_t *_sock) return rc; } -static int _cb_raw_shutdown(trilogy_sock_t *_sock) { return shutdown(trilogy_sock_fd(_sock), SHUT_RDWR); } +static int _cb_shutdown_connect(trilogy_sock_t *_sock) { + (void)_sock; + return TRILOGY_CLOSED_CONNECTION; +} +static ssize_t _cb_shutdown_write(trilogy_sock_t *_sock, const void *buf, size_t nwrite) { + (void)_sock; + (void)buf; + (void)nwrite; + return TRILOGY_CLOSED_CONNECTION; +} +static ssize_t _cb_shutdown_read(trilogy_sock_t *_sock, void *buf, size_t nread) { + (void)_sock; + (void)buf; + (void)nread; + return TRILOGY_CLOSED_CONNECTION; +} +static int _cb_shutdown_wait(trilogy_sock_t *_sock, trilogy_wait_t wait) { + (void)_sock; + (void)wait; + return TRILOGY_OK; +} +static int _cb_shutdown_shutdown(trilogy_sock_t *_sock) { + (void)_sock; + return TRILOGY_OK; +} + +// Shutdown will close the underlying socket fd and replace all I/O operations with stubs which perform no action. +static int _cb_raw_shutdown(trilogy_sock_t *_sock) { + struct trilogy_sock *sock = (struct trilogy_sock *)_sock; + + // Replace all operations with stubs which return immediately + sock->base.connect_cb = _cb_shutdown_connect; + sock->base.read_cb = _cb_shutdown_read; + sock->base.write_cb = _cb_shutdown_write; + sock->base.wait_cb = _cb_shutdown_wait; + sock->base.shutdown_cb = _cb_shutdown_shutdown; + + // These "raw" callbacks won't attempt further operations on the socket and work correctly with fd set to -1 + sock->base.close_cb = _cb_raw_close; + sock->base.fd_cb = _cb_raw_fd; + + if (sock->fd != -1) + close(sock->fd); + sock->fd = -1; + + return TRILOGY_OK; +} static int set_nonblocking_fd(int sock) { @@ -130,12 +196,21 @@ static int raw_connect_internal(struct trilogy_sock *sock, const struct addrinfo { int sockerr; socklen_t sockerr_len = sizeof(sockerr); + int rc = TRILOGY_SYSERR; sock->fd = socket(ai->ai_family, SOCK_STREAM, ai->ai_protocol); if (sock->fd < 0) { return TRILOGY_SYSERR; } +#ifdef TCP_NODELAY + if (sock->addr->ai_family != PF_UNIX) { + int flags = 1; + if (setsockopt(sock->fd, IPPROTO_TCP, TCP_NODELAY, (void *)&flags, sizeof(flags)) < 0) { + goto fail; + } + } +#endif if (sock->base.opts.keepalive_enabled) { int flags = 1; if (setsockopt(sock->fd, SOL_SOCKET, SO_KEEPALIVE, (void *)&flags, sizeof(flags)) < 0) { @@ -177,8 +252,8 @@ static int raw_connect_internal(struct trilogy_sock *sock, const struct addrinfo } } - if (trilogy_sock_wait_write((trilogy_sock_t *)sock) < 0) { - goto fail; + if ((rc = trilogy_sock_wait_write((trilogy_sock_t *)sock)) < 0) { + goto failrc; } if (getsockopt(sock->fd, SOL_SOCKET, SO_ERROR, &sockerr, &sockerr_len) < 0) { @@ -196,9 +271,11 @@ static int raw_connect_internal(struct trilogy_sock *sock, const struct addrinfo return TRILOGY_OK; fail: + rc = TRILOGY_SYSERR; +failrc: close(sock->fd); sock->fd = -1; - return TRILOGY_SYSERR; + return rc; } static int _cb_raw_connect(trilogy_sock_t *_sock) @@ -306,12 +383,21 @@ int trilogy_sock_resolve(trilogy_sock_t *_sock) static ssize_t ssl_io_return(struct trilogy_sock *sock, ssize_t ret) { - if (ret < 0) { + if (ret <= 0) { int rc = SSL_get_error(sock->ssl, (int)ret); if (rc == SSL_ERROR_WANT_WRITE || rc == SSL_ERROR_WANT_READ) { return (ssize_t)TRILOGY_AGAIN; - } else if (rc == SSL_ERROR_SYSCALL && errno != 0) { - return (ssize_t)TRILOGY_SYSERR; + } else if (rc == SSL_ERROR_ZERO_RETURN) { + // Server has closed the connection for writing by sending the close_notify alert + return (ssize_t)TRILOGY_CLOSED_CONNECTION; + } else if (rc == SSL_ERROR_SYSCALL && !ERR_peek_error()) { + if (errno == 0) { + // On OpenSSL <= 1.1.1, SSL_ERROR_SYSCALL with an errno value + // of 0 indicates unexpected EOF from the peer. + return (ssize_t)TRILOGY_CLOSED_CONNECTION; + } else { + return (ssize_t)TRILOGY_SYSERR; + } } return (ssize_t)TRILOGY_OPENSSL_ERR; } @@ -321,6 +407,11 @@ static ssize_t ssl_io_return(struct trilogy_sock *sock, ssize_t ret) static ssize_t _cb_ssl_read(trilogy_sock_t *_sock, void *buf, size_t nread) { struct trilogy_sock *sock = (struct trilogy_sock *)_sock; + + // This shouldn't be necessary, but protects against other libraries in the same process incorrectly leaving errors + // in the queue. + ERR_clear_error(); + ssize_t data_read = (ssize_t)SSL_read(sock->ssl, buf, (int)nread); return ssl_io_return(sock, data_read); } @@ -328,6 +419,11 @@ static ssize_t _cb_ssl_read(trilogy_sock_t *_sock, void *buf, size_t nread) static ssize_t _cb_ssl_write(trilogy_sock_t *_sock, const void *buf, size_t nwrite) { struct trilogy_sock *sock = (struct trilogy_sock *)_sock; + + // This shouldn't be necessary, but protects against other libraries in the same process incorrectly leaving errors + // in the queue. + ERR_clear_error(); + ssize_t data_written = (ssize_t)SSL_write(sock->ssl, buf, (int)nwrite); return ssl_io_return(sock, data_written); } @@ -340,15 +436,9 @@ static int _cb_ssl_shutdown(trilogy_sock_t *_sock) // we need to close it. The OpenSSL explicitly states // not to call SSL_shutdown on a broken SSL socket. SSL_free(sock->ssl); - // Reset the handlers since we tore down SSL, so we - // fall back to the regular methods for detecting - // we have a closed connection and for the cleanup. - sock->base.read_cb = _cb_raw_read; - sock->base.write_cb = _cb_raw_write; - sock->base.shutdown_cb = _cb_raw_shutdown; - sock->base.close_cb = _cb_raw_close; sock->ssl = NULL; + // This will rewrite the handlers return _cb_raw_shutdown(_sock); } @@ -357,7 +447,10 @@ static int _cb_ssl_close(trilogy_sock_t *_sock) struct trilogy_sock *sock = (struct trilogy_sock *)_sock; if (sock->ssl != NULL) { if (SSL_in_init(sock->ssl) == 0) { - SSL_shutdown(sock->ssl); + (void)SSL_shutdown(sock->ssl); + // SSL_shutdown might return WANT_WRITE or WANT_READ. Ideally we would retry but we don't want to block. + // It may also push an error onto the OpenSSL error queue, so clear that. + ERR_clear_error(); } SSL_free(sock->ssl); sock->ssl = NULL; @@ -539,6 +632,10 @@ int trilogy_sock_upgrade_ssl(trilogy_sock_t *_sock) { struct trilogy_sock *sock = (struct trilogy_sock *)_sock; + // This shouldn't be necessary, but protects against other libraries in the same process incorrectly leaving errors + // in the queue. + ERR_clear_error(); + SSL_CTX *ctx = trilogy_ssl_ctx(&sock->base.opts); if (!ctx) { @@ -581,6 +678,10 @@ int trilogy_sock_upgrade_ssl(trilogy_sock_t *_sock) goto fail; for (;;) { + // This shouldn't be necessary, but protects against other libraries in the same process incorrectly leaving errors + // in the queue. + ERR_clear_error(); + int ret = SSL_connect(sock->ssl); if (ret == 1) { #if OPENSSL_VERSION_NUMBER < 0x1000200fL @@ -623,28 +724,3 @@ int trilogy_sock_upgrade_ssl(trilogy_sock_t *_sock) sock->ssl = NULL; return TRILOGY_OPENSSL_ERR; } - -int trilogy_sock_discard(trilogy_sock_t *_sock) -{ - struct trilogy_sock *sock = (struct trilogy_sock *)_sock; - - if (sock->fd < 0) { - return TRILOGY_OK; - } - - int null_fd = open("/dev/null", O_RDWR | O_CLOEXEC); - if (null_fd < 0) { - return TRILOGY_SYSERR; - } - - if (dup2(null_fd, sock->fd) < 0) { - close(null_fd); - return TRILOGY_SYSERR; - } - - if (close(null_fd) < 0) { - return TRILOGY_SYSERR; - } - - return TRILOGY_OK; -}