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

Crash in the mysql driver when decoding short decimal values as f64 #2691

Closed
lovasoa opened this issue Aug 11, 2023 · 6 comments · Fixed by #3154
Closed

Crash in the mysql driver when decoding short decimal values as f64 #2691

lovasoa opened this issue Aug 11, 2023 · 6 comments · Fixed by #3154

Comments

@lovasoa
Copy link
Contributor

lovasoa commented Aug 11, 2023

Bug Description

the mysql driver crashes the entire program when decoding short decimal values as f64

see the fix in sqlx-oldapi: sqlpage@db000fb

@ebissi
Copy link

ebissi commented Aug 15, 2023

IMHO this isn't a bug and that conversion should not be done by sqlx because it would break the expected behavior of DECIMAL on rust side.

The conversion DECIMAL -> f64 should be explicitly done by the programmer in SQL using CAST(decimal_column AS DOUBLE) or in rust using rust_decimal to_f64. This way it is up to the programmer to handle its consequences.

@lovasoa
Copy link
Contributor Author

lovasoa commented Aug 15, 2023

I don't think it would be an issue to return an error in this case. But currently, sqlx does not return an error, it crashes the entire program.

@chenlcacentury
Copy link

just checking some opening issues to evaluate how stable and robust this crate is and is it ready to be used in production. It seems some philosophies behind are potentially very risky.

This issue is indeed a bug and a very bad bug because it will crash the whole program which is definitely unacceptable in production.

Be very careful to use unwrap, expect etc., and only use them while it is 100000% no crash or intended to be crashed

@abonander
Copy link
Collaborator

We'd appreciate a PR. I'd just check that the binary length is either 4 or 8, or return an error.

@lovasoa
Copy link
Contributor Author

lovasoa commented Mar 14, 2024

@abonander I have a link to the fix in my initial comment, if you want.

@abonander
Copy link
Collaborator

abonander commented Mar 14, 2024

I saw your patch, but I agree with @ebissi. f64 should not decode from DECIMAL as they have distinctly different semantics.

The bug is simply that LittleEndian::read_f32() panics if the buffer contains less than four bytes, which should be checked before calling it.

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 a pull request may close this issue.

4 participants