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

MySQL u32 overflow at i32::max_value() #1745

Closed
bpbp-boop opened this Issue Jun 1, 2018 · 4 comments

Comments

Projects
None yet
3 participants
@bpbp-boop

bpbp-boop commented Jun 1, 2018

Setup

Versions

  • Rust: rustc 1.26.1 (827013a31 2018-05-25)
  • Diesel: 1.3.0
  • Database: MySQL 8
  • Operating System CentOS

Feature Flags

  • diesel: diesel = { version = "1.0.0", features = ["mysql", "chrono"] }

Problem Description

Trying to insert a u32 above i32::max_value causes an overflow and database error on MySQL

Steps to reproduce (reduced testcase)

#[macro_use]
extern crate diesel;
extern crate dotenv;

use diesel::prelude::*;
use diesel::insert_into;
use dotenv::dotenv;
use std::env;

table! {
    DATA (id) {
        id -> Integer,
        record_id -> Unsigned<Integer>,
        session_id -> Unsigned<Integer>,
    }
}

#[derive(Debug, Insertable)]
#[table_name = "DATA"]
pub struct Data {
    pub record_id: u32,
    pub session_id: u32,
}

fn main() {
    use DATA::dsl::*;

    dotenv().ok();
    let database_url = env::var("DATABASE_URL").expect("DATABASE_URL must be set");
    let connection = MysqlConnection::establish(&database_url)
        .expect(&format!("Error connecting to {}", database_url));

    let i32_max = i32::max_value() as u32;
    let data_ok = Data {
        record_id: 1,
        session_id: i32_max
    };

    insert_into(DATA).values(&data_ok).execute(&connection).unwrap();

    let data_error = Data {
        record_id: 1,
        session_id: i32_max + 1
    };

    insert_into(DATA).values(&data_error).execute(&connection).unwrap();
}

Output

    Finished dev [unoptimized + debuginfo] target(s) in 10.45 secs
     Running `target/debug/overflow`

thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: DatabaseError(__Unknown, "Out of range value for column \'session_id\' at row 1")', libcore/result.rs:945:5
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:207
   3: std::panicking::default_hook
             at libstd/panicking.rs:223
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:402
   5: std::panicking::begin_panic_fmt
             at libstd/panicking.rs:349
   6: rust_begin_unwind
             at libstd/panicking.rs:325
   7: core::panicking::panic_fmt
             at libcore/panicking.rs:72
   8: core::result::unwrap_failed
             at /checkout/src/libcore/macros.rs:26
   9: <core::result::Result<T, E>>::unwrap
             at /checkout/src/libcore/result.rs:782
  10: overflow::main
             at src/main.rs:46
  11: std::rt::lang_start::{{closure}}
             at /checkout/src/libstd/rt.rs:74
  12: std::panicking::try::do_call
             at libstd/rt.rs:59
             at libstd/panicking.rs:306
  13: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:102
  14: std::rt::lang_start_internal
             at libstd/panicking.rs:285
             at libstd/panic.rs:361
             at libstd/rt.rs:58
  15: std::rt::lang_start
             at /checkout/src/libstd/rt.rs:74
  16: main
  17: __libc_start_main
  18: <unknown>


Actual SQL executed via the general log

2018-06-01T04:52:07.152401Z        74 Query     SET sql_mode=(SELECT CONCAT(@@sql_mode, ',PIPES_AS_CONCAT'))
2018-06-01T04:52:07.152678Z        74 Query     SET time_zone = '+00:00'
2018-06-01T04:52:07.152931Z        74 Query     SET character_set_client = 'utf8mb4'
2018-06-01T04:52:07.153124Z        74 Query     SET character_set_connection = 'utf8mb4'
2018-06-01T04:52:07.153405Z        74 Query     SET character_set_results = 'utf8mb4'
2018-06-01T04:52:07.154014Z        74 Prepare   INSERT INTO `DATA` (`record_id`, `session_id`) VALUES (?, ?)
2018-06-01T04:52:07.154222Z        74 Execute   INSERT INTO `DATA` (`record_id`, `session_id`) VALUES (1, 2147483647)
2018-06-01T04:52:07.323184Z        74 Close stmt
2018-06-01T04:52:07.323492Z        74 Prepare   INSERT INTO `DATA` (`record_id`, `session_id`) VALUES (?, ?)
2018-06-01T04:52:07.323738Z        74 Execute   INSERT INTO `DATA` (`record_id`, `session_id`) VALUES (1, -2147483648)
2018-06-01T04:52:07.324176Z        74 Close stmt
2018-06-01T04:52:07.324698Z        74 Quit

Checklist

  • I have already looked over the issue tracker for similar issues.
  • This issue can be reproduced on Rust's stable channel. (Your issue will be
    closed if this is not the case)
@weiznich

This comment has been minimized.

Contributor

weiznich commented Jun 1, 2018

That looks strange. We have explicitly a test case testing u32::max_value().

Could you provide the sql that you used to construct your table?

@sgrif

This comment has been minimized.

Member

sgrif commented Jun 1, 2018

Fixing this is going to require a major version bump :\

sgrif added a commit that referenced this issue Jun 1, 2018

Fix our handling of unsigned types on MySQL
We were sending the right bytes, but we weren't properly setting the
`is_unsigned` flag on the bind parameters, resulting in it always being
sent as signed. This results in a client error about overflow when
deserializing large values, and either a server error or incorrect
results when serializing them.

The fact that the tests for this all used `-1` should have been a huge
red flag, but it slipped through.

While changing the type of `Mysql::BindCollector` could technically be
considered a breaking change under the strictest definition, I do not
consider this to be one. We have stated in the past that folks should
not rely on the exact types used in `impl Backend`. And unlike changing
`RawValue`, there's literally no reason for anybody to have depended on
the exact value of this type, unless they're implementing their own
Connection.

If we really want to avoid any potential source incompatibility, we can
actually do this without the custom bind collector, if we have the
`ToSql` implementations for `Unsigned` add an extra byte at the end, and
check to see if the length of the buffer is 1 byte larger than the
expected length. However, this is super hacky and much more likely to
break applications than this approach.

Fixes #1745.
Close #1749.

sgrif added a commit that referenced this issue Jun 1, 2018

Fix our handling of unsigned types on MySQL
We were sending the right bytes, but we weren't properly setting the
`is_unsigned` flag on the bind parameters, resulting in it always being
sent as signed. This results in a client error about overflow when
deserializing large values, and either a server error or incorrect
results when serializing them.

The fact that the tests for this all used `-1` should have been a huge
red flag, but it slipped through.

While changing the type of `Mysql::BindCollector` could technically be
considered a breaking change under the strictest definition, I do not
consider this to be one. We have stated in the past that folks should
not rely on the exact types used in `impl Backend`. And unlike changing
`RawValue`, there's literally no reason for anybody to have depended on
the exact value of this type, unless they're implementing their own
Connection.

If we really want to avoid any potential source incompatibility, we can
actually do this without the custom bind collector, if we have the
`ToSql` implementations for `Unsigned` add an extra byte at the end, and
check to see if the length of the buffer is 1 byte larger than the
expected length. However, this is super hacky and much more likely to
break applications than this approach.

Fixes #1745.
Close #1749.

@sgrif sgrif closed this in #1751 Jun 13, 2018

@bpbp-boop

This comment has been minimized.

bpbp-boop commented Jun 13, 2018

Thanks!

@sgrif

This comment has been minimized.

Member

sgrif commented Jun 14, 2018

1.3.2 has been released with this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment