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

Fix panic in Postgres Bytes decode #1948

Merged
merged 1 commit into from
Jul 7, 2022
Merged

Conversation

e00E
Copy link
Contributor

@e00E e00E commented Jul 4, 2022

This function can panic due to slicing out of bounds when the server
responds without the \x prefix. With this commit we instead error and
also ensure that the prefix is what we expect instead of blindly
removing it.

Not directly related to the panic, we replace as_str() with as_bytes()
because there is no reason to perform a utf8 validity check when
hex::decode already checks that the content is valid.

@abonander
Copy link
Collaborator

abonander commented Jul 4, 2022

Good fix, though should we perhaps have a regression test?

It should be as simple as something like:

let mut conn = new::<Postgres>().await?;

conn.execute("SET bytea_output = 'escape';").await?;

let res: sqlx::Result<Vec<u8>> = conn.fetch_one("SELECT '\xDEADBEEF'::bytea")
    .await?
    .try_get(0usize);

// Binary deserialization only supports the `hex` format, but should not panic.
let _ = res.unwrap_err();

This function can panic due to slicing out of bounds when the server
responds without the `\x` prefix. With this commit we instead error and
also ensure that the prefix is what we expect instead of blindly
removing it.

Not directly related to the panic, we replace as_str() with as_bytes()
because there is no reason to perform a utf8 validity check when
hex::decode already checks that the content is valid.
@e00E
Copy link
Contributor Author

e00E commented Jul 5, 2022

I've added a test. I had to adapt your example to test an empty hex string to trigger the panic without this commit.

@abonander abonander merged commit b3bbdab into launchbadge:main Jul 7, 2022
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.

2 participants