Skip to content

Commit

Permalink
Don't confuse length encoded int with EOF
Browse files Browse the repository at this point in the history
Prior to this commit a row starting with a field with length 2 ** 24 or
higher would get interpreted as an EOF and we'd fail with:

```
Trilogy::QueryError: trilogy_read_row: TRILOGY_EXTRA_DATA_IN_PACKET
```

rows are sent as a series of length encoded strings. These are strings
prefixed by a length encoded integer, where 0xFE is the prefix byte for
an 8 byte integer.

After all the rows are sent, we get a OK/EOF packet beginning with 0xFE.

The way to tell the OK/EOF apart is by checking that the length of the
whole payload is < 9 (i.e. there isn't enough room for an 8-byte int).

We were doing the `< 9` check for the deprecated EOF packets, but not
for the newer OK packets.
  • Loading branch information
composerinteralia committed Apr 9, 2024
1 parent f199c58 commit 8ea052e
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 25 deletions.
18 changes: 18 additions & 0 deletions contrib/ruby/test/client_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1122,4 +1122,22 @@ def test_error_classes_exclusively_match_subclasses
assert_operator SystemCallError, :===, klass.new
assert_operator Trilogy::ConnectionError, :===, klass.new
end

def test_reads_row_not_eof
set_max_allowed_packet(1024 * 1024 * 1024) # In case other tests lowered it
client = new_tcp_client
client.change_db "test"
client.query("DROP TABLE IF EXISTS trilogy_longtext_test")
client.query("CREATE TABLE `trilogy_longtext_test` (`text` LONGTEXT)")

smalles_8byte_length_encoded_int = 2 ** 24
value = "x" * smalles_8byte_length_encoded_int
client.query("INSERT INTO `trilogy_longtext_test` (`text`) VALUES (\"#{value}\")")

# Ensure we read the row and don't confuse it with an EOF packet
result = client.query("SELECT `text` FROM `trilogy_longtext_test` LIMIT 1")
assert_equal value.length, result.rows[0][0].length
ensure
ensure_closed client
end
end
6 changes: 3 additions & 3 deletions contrib/ruby/test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,9 @@ def new_tcp_client(opts = {})
port: DEFAULT_PORT,
username: DEFAULT_USER,
password: DEFAULT_PASS,
ssl: true,
ssl_mode: Trilogy::SSL_PREFERRED_NOVERIFY,
tls_min_version: Trilogy::TLS_VERSION_12,
# ssl: true,
# ssl_mode: Trilogy::SSL_PREFERRED_NOVERIFY,
# tls_min_version: Trilogy::TLS_VERSION_12,
}.merge(opts)

c = Trilogy.new defaults
Expand Down
47 changes: 25 additions & 22 deletions src/client.c
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ static int read_err_packet(trilogy_conn_t *conn)
return TRILOGY_ERR;
}

static int read_eof_packet(trilogy_conn_t *conn)
static int read_deprecated_eof_packet(trilogy_conn_t *conn)
{
trilogy_eof_packet_t eof_packet;

Expand All @@ -236,6 +236,21 @@ static int read_eof_packet(trilogy_conn_t *conn)
return TRILOGY_EOF;
}

static int read_eof_packet(trilogy_conn_t *conn)
{
int rc;

if (conn->capabilities & TRILOGY_CAPABILITIES_DEPRECATE_EOF) {
return read_ok_packet(conn);
} else {
if ((rc = read_deprecated_eof_packet(conn)) != TRILOGY_EOF) {
return rc;
}

return TRILOGY_OK;
}
}

static int read_auth_switch_packet(trilogy_conn_t *conn, trilogy_handshake_t *handshake)
{
trilogy_auth_switch_request_packet_t auth_switch_packet;
Expand Down Expand Up @@ -647,15 +662,7 @@ static int read_eof(trilogy_conn_t *conn)
return rc;
}

if (conn->capabilities & TRILOGY_CAPABILITIES_DEPRECATE_EOF) {
return read_ok_packet(conn);
} else {
if ((rc = read_eof_packet(conn)) != TRILOGY_EOF) {
return rc;
}

return TRILOGY_OK;
}
return read_eof_packet(conn);
}

int trilogy_read_row(trilogy_conn_t *conn, trilogy_value_t *values_out)
Expand All @@ -680,14 +687,12 @@ int trilogy_read_row(trilogy_conn_t *conn, trilogy_value_t *values_out)
return rc;
}

if (conn->capabilities & TRILOGY_CAPABILITIES_DEPRECATE_EOF && current_packet_type(conn) == TRILOGY_PACKET_EOF) {
if ((rc = read_ok_packet(conn)) != TRILOGY_OK) {
if (current_packet_type(conn) == TRILOGY_PACKET_EOF && conn->packet_buffer.len < 9) {
if ((rc = read_eof_packet(conn)) != TRILOGY_OK) {
return rc;
}

return TRILOGY_EOF;
} else if (current_packet_type(conn) == TRILOGY_PACKET_EOF && conn->packet_buffer.len < 9) {
return read_eof_packet(conn);
} else if (current_packet_type(conn) == TRILOGY_PACKET_ERR) {
return read_err_packet(conn);
} else {
Expand Down Expand Up @@ -941,20 +946,18 @@ int trilogy_stmt_bind_data_send(trilogy_conn_t *conn, trilogy_stmt_t *stmt, uint
int trilogy_stmt_read_row(trilogy_conn_t *conn, trilogy_stmt_t *stmt, trilogy_column_packet_t *columns,
trilogy_binary_value_t *values_out)
{
int err = read_packet(conn);
int rc = read_packet(conn);

if (err < 0) {
return err;
if (rc < 0) {
return rc;
}

if (conn->capabilities & TRILOGY_CAPABILITIES_DEPRECATE_EOF && current_packet_type(conn) == TRILOGY_PACKET_EOF) {
if ((err = read_ok_packet(conn)) != TRILOGY_OK) {
return err;
if (current_packet_type(conn) == TRILOGY_PACKET_EOF && conn->packet_buffer.len < 9) {
if ((rc = read_eof_packet(conn)) != TRILOGY_OK) {
return rc;
}

return TRILOGY_EOF;
} else if (current_packet_type(conn) == TRILOGY_PACKET_EOF && conn->packet_buffer.len < 9) {
return read_eof_packet(conn);
} else if (current_packet_type(conn) == TRILOGY_PACKET_ERR) {
return read_err_packet(conn);
} else {
Expand Down

0 comments on commit 8ea052e

Please sign in to comment.