Skip to content
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.

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

Don't confuse length encoded int with EOF #179

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

composerinteralia
Copy link
Contributor

@composerinteralia composerinteralia commented Apr 9, 2024

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 from an 8-byte length-encoded int 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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant