feat: add MySqlTime, audit mysql::types for panics (#3154)

Also clarifies the handling of `TIME` (we never realized it's used for both time-of-day and signed intervals) and adds appropriate impls for `std::time::Duration`, `time::Duration`, `chrono::TimeDelta`

sqlx-mysql/src/protocol/statement/row.rs

@@ -45,7 +45,20 @@ impl<'de> Decode<'de, &'de [MySqlColumn]> for BinaryRow {
             // NOTE: MySQL will never generate NULL types for non-NULL values
             let type_info = &column.type_info;
 
+            // Unlike Postgres, MySQL does not length-prefix every value in a binary row.
+            // Values are *either* fixed-length or length-prefixed,
+            // so we need to inspect the type code to be sure.
             let size: usize = match type_info.r#type {
+                // All fixed-length types.
+                ColumnType::LongLong => 8,
+                ColumnType::Long | ColumnType::Int24 => 4,
+                ColumnType::Short | ColumnType::Year => 2,
+                ColumnType::Tiny => 1,
+                ColumnType::Float => 4,
+                ColumnType::Double => 8,
+
+                // Blobs and strings are prefixed with their length,
+                // which is itself a length-encoded integer.
                 ColumnType::String
                 | ColumnType::VarChar
                 | ColumnType::VarString
@@ -61,18 +74,13 @@ impl<'de> Decode<'de, &'de [MySqlColumn]> for BinaryRow {
                 | ColumnType::Json
                 | ColumnType::NewDecimal => buf.get_uint_lenenc() as usize,
 
-                ColumnType::LongLong => 8,
-                ColumnType::Long | ColumnType::Int24 => 4,
-                ColumnType::Short | ColumnType::Year => 2,
-                ColumnType::Tiny => 1,
-                ColumnType::Float => 4,
-                ColumnType::Double => 8,
-
+                // Like strings and blobs, these values are variable-length.
+                // Unlike strings and blobs, however, they exclusively use one byte for length.
                 ColumnType::Time
                 | ColumnType::Timestamp
                 | ColumnType::Date
                 | ColumnType::Datetime => {
-                    // The size of this type is important for decoding
+                    // Leave the length byte on the front of the value because decoding uses it.
                     buf[0] as usize + 1
                 }
 

sqlx-mysql/src/types/chrono.rs

@@ -2,13 +2,14 @@ use bytes::Buf;
 use chrono::{
     DateTime, Datelike, Local, NaiveDate, NaiveDateTime, NaiveTime, TimeZone, Timelike, Utc,
 };
+use sqlx_core::database::Database;
 
 use crate::decode::Decode;
 use crate::encode::{Encode, IsNull};
 use crate::error::{BoxDynError, UnexpectedNullError};
 use crate::protocol::text::ColumnType;
 use crate::type_info::MySqlTypeInfo;
-use crate::types::Type;
+use crate::types::{MySqlTime, MySqlTimeSign, Type};
 use crate::{MySql, MySqlValueFormat, MySqlValueRef};
 
 impl Type<MySql> for DateTime<Utc> {
@@ -63,7 +64,7 @@ impl<'r> Decode<'r, MySql> for DateTime<Local> {
 
 impl Type<MySql> for NaiveTime {
     fn type_info() -> MySqlTypeInfo {
-        MySqlTypeInfo::binary(ColumnType::Time)
+        MySqlTime::type_info()
     }
 }
 
@@ -75,7 +76,7 @@ impl Encode<'_, MySql> for NaiveTime {
         // NaiveTime is not negative
         buf.push(0);
 
-        // "date on 4 bytes little-endian format" (?)
+        // Number of days in the interval; always 0 for time-of-day values.
         // https://mariadb.com/kb/en/resultset-row/#teimstamp-binary-encoding
         buf.extend_from_slice(&[0_u8; 4]);
 
@@ -95,34 +96,18 @@ impl Encode<'_, MySql> for NaiveTime {
     }
 }
 
+/// Decode from a `TIME` value.
+///
+/// ### Errors
+/// Returns an error if the `TIME` value is negative or exceeds `23:59:59.999999`.
 impl<'r> Decode<'r, MySql> for NaiveTime {
     fn decode(value: MySqlValueRef<'r>) -> Result<Self, BoxDynError> {
         match value.format() {
             MySqlValueFormat::Binary => {
-                let mut buf = value.as_bytes()?;
-
-                // data length, expecting 8 or 12 (fractional seconds)
-                let len = buf.get_u8();
-
-                // MySQL specifies that if all of hours, minutes, seconds, microseconds
-                // are 0 then the length is 0 and no further data is send
-                // https://dev.mysql.com/doc/internals/en/binary-protocol-value.html
-                if len == 0 {
-                    return Ok(NaiveTime::from_hms_micro_opt(0, 0, 0, 0)
-                        .expect("expected NaiveTime to construct from all zeroes"));
-                }
-
-                // is negative : int<1>
-                let is_negative = buf.get_u8();
-                debug_assert_eq!(is_negative, 0, "Negative dates/times are not supported");
-
-                // "date on 4 bytes little-endian format" (?)
-                // https://mariadb.com/kb/en/resultset-row/#timestamp-binary-encoding
-                buf.advance(4);
-
-                decode_time(len - 5, buf)
+                // Covers most possible failure modes.
+                MySqlTime::decode(value)?.try_into()
             }
-
+            // Retaining this parsing for now as it allows us to cross-check our impl.
             MySqlValueFormat::Text => {
                 let s = value.as_str()?;
                 NaiveTime::parse_from_str(s, "%H:%M:%S%.f").map_err(Into::into)
@@ -131,6 +116,57 @@ impl<'r> Decode<'r, MySql> for NaiveTime {
     }
 }
 
+impl TryFrom<MySqlTime> for NaiveTime {
+    type Error = BoxDynError;
+
+    fn try_from(time: MySqlTime) -> Result<Self, Self::Error> {
+        NaiveTime::from_hms_micro_opt(
+            time.hours(),
+            time.minutes() as u32,
+            time.seconds() as u32,
+            time.microseconds(),
+        )
+        .ok_or_else(|| format!("Cannot convert `MySqlTime` value to `NaiveTime`: {time}").into())
+    }
+}
+
+impl From<MySqlTime> for chrono::TimeDelta {
+    fn from(time: MySqlTime) -> Self {
+        chrono::TimeDelta::new(time.whole_seconds_signed(), time.subsec_nanos())
+            .expect("BUG: chrono::TimeDelta should have a greater range than MySqlTime")
+    }
+}
+
+impl TryFrom<chrono::TimeDelta> for MySqlTime {
+    type Error = BoxDynError;
+
+    fn try_from(value: chrono::TimeDelta) -> Result<Self, Self::Error> {
+        let sign = if value < chrono::TimeDelta::zero() {
+            MySqlTimeSign::Negative
+        } else {
+            MySqlTimeSign::Positive
+        };
+
+        Ok(
+            // `std::time::Duration` has a greater positive range than `TimeDelta`
+            // which makes it a great intermediate if you ignore the sign.
+            MySqlTime::try_from(value.abs().to_std()?)?.with_sign(sign),
+        )
+    }
+}
+
+impl Type<MySql> for chrono::TimeDelta {
+    fn type_info() -> MySqlTypeInfo {
+        MySqlTime::type_info()
+    }
+}
+
+impl<'r> Decode<'r, MySql> for chrono::TimeDelta {
+    fn decode(value: <MySql as Database>::ValueRef<'r>) -> Result<Self, BoxDynError> {
+        Ok(MySqlTime::decode(value)?.into())
+    }
+}
+
 impl Type<MySql> for NaiveDate {
     fn type_info() -> MySqlTypeInfo {
         MySqlTypeInfo::binary(ColumnType::Date)
@@ -155,7 +191,14 @@ impl<'r> Decode<'r, MySql> for NaiveDate {
     fn decode(value: MySqlValueRef<'r>) -> Result<Self, BoxDynError> {
         match value.format() {
             MySqlValueFormat::Binary => {
-                decode_date(&value.as_bytes()?[1..])?.ok_or_else(|| UnexpectedNullError.into())
+                let buf = value.as_bytes()?;
+
+                // Row decoding should have left the length prefix.
+                if buf.is_empty() {
+                    return Err("empty buffer".into());
+                }
+
+                decode_date(&buf[1..])?.ok_or_else(|| UnexpectedNullError.into())
             }
 
             MySqlValueFormat::Text => {
@@ -214,6 +257,10 @@ impl<'r> Decode<'r, MySql> for NaiveDateTime {
             MySqlValueFormat::Binary => {
                 let buf = value.as_bytes()?;
 
+                if buf.is_empty() {
+                    return Err("empty buffer".into());
+                }
+
                 let len = buf[0];
                 let date = decode_date(&buf[1..])?.ok_or(UnexpectedNullError)?;
 

sqlx-mysql/src/types/float.rs

@@ -8,6 +8,7 @@ use crate::types::Type;
 use crate::{MySql, MySqlTypeInfo, MySqlValueFormat, MySqlValueRef};
 
 fn real_compatible(ty: &MySqlTypeInfo) -> bool {
+    // NOTE: `DECIMAL` is explicitly excluded because floating-point numbers have different semantics.
     matches!(ty.r#type, ColumnType::Float | ColumnType::Double)
 }
 
@@ -53,12 +54,22 @@ impl Decode<'_, MySql> for f32 {
             MySqlValueFormat::Binary => {
                 let buf = value.as_bytes()?;
 
-                if buf.len() == 8 {
+                match buf.len() {
+                    // These functions panic if `buf` is not exactly the right size.
+                    4 => LittleEndian::read_f32(buf),
                     // MySQL can return 8-byte DOUBLE values for a FLOAT
-                    // We take and truncate to f32 as that's the same behavior as *in* MySQL
-                    LittleEndian::read_f64(buf) as f32
-                } else {
-                    LittleEndian::read_f32(buf)
+                    // We take and truncate to f32 as that's the same behavior as *in* MySQL,
+                    8 => LittleEndian::read_f64(buf) as f32,
+                    other => {
+                        // Users may try to decode a DECIMAL as floating point;
+                        // inform them why that's a bad idea.
+                        return Err(format!(
+                            "expected a FLOAT as 4 or 8 bytes, got {other} bytes; \
+                             note that decoding DECIMAL as `f32` is not supported \
+                             due to differing semantics"
+                        )
+                        .into());
+                    }
                 }
             }
 
@@ -70,7 +81,26 @@ impl Decode<'_, MySql> for f32 {
 impl Decode<'_, MySql> for f64 {
     fn decode(value: MySqlValueRef<'_>) -> Result<Self, BoxDynError> {
         Ok(match value.format() {
-            MySqlValueFormat::Binary => LittleEndian::read_f64(value.as_bytes()?),
+            MySqlValueFormat::Binary => {
+                let buf = value.as_bytes()?;
+
+                // The `read_*` functions panic if `buf` is not exactly the right size.
+                match buf.len() {
+                    // Allow implicit widening here
+                    4 => LittleEndian::read_f32(buf) as f64,
+                    8 => LittleEndian::read_f64(buf),
+                    other => {
+                        // Users may try to decode a DECIMAL as floating point;
+                        // inform them why that's a bad idea.
+                        return Err(format!(
+                            "expected a DOUBLE as 4 or 8 bytes, got {other} bytes; \
+                             note that decoding DECIMAL as `f64` is not supported \
+                             due to differing semantics"
+                        )
+                        .into());
+                    }
+                }
+            }
             MySqlValueFormat::Text => value.as_str()?.parse()?,
         })
     }

sqlx-mysql/src/types/int.rs

@@ -95,6 +95,20 @@ fn int_decode(value: MySqlValueRef<'_>) -> Result<i64, BoxDynError> {
         MySqlValueFormat::Text => value.as_str()?.parse()?,
         MySqlValueFormat::Binary => {
             let buf = value.as_bytes()?;
+
+            // Check conditions that could cause `read_int()` to panic.
+            if buf.is_empty() {
+                return Err("empty buffer".into());
+            }
+
+            if buf.len() > 8 {
+                return Err(format!(
+                    "expected no more than 8 bytes for integer value, got {}",
+                    buf.len()
+                )
+                .into());
+            }
+
             LittleEndian::read_int(buf, buf.len())
         }
     })

sqlx-mysql/src/types/mod.rs

@@ -20,6 +20,8 @@
 //! | `IpAddr`                              | VARCHAR, TEXT                                        |
 //! | `Ipv4Addr`                            | INET4 (MariaDB-only), VARCHAR, TEXT                  |
 //! | `Ipv6Addr`                            | INET6 (MariaDB-only), VARCHAR, TEXT                  |
+//! | [`MySqlTime`]                         | TIME (encode and decode full range)                  |
+//! | [`Duration`]                          | TIME (for decoding positive values only)             |
 //!
 //! ##### Note: `BOOLEAN`/`BOOL` Type
 //! MySQL and MariaDB treat `BOOLEAN` as an alias of the `TINYINT` type:
@@ -38,6 +40,12 @@
 //! Thus, you must use the type override syntax in the query to tell the macros you are expecting
 //! a `bool` column. See the docs for `query!()` and `query_as!()` for details on this syntax.
 //!
+//! ### NOTE: MySQL's `TIME` type is signed
+//! MySQL's `TIME` type can be used as either a time-of-day value, or a signed interval.
+//! Thus, it may take on negative values.
+//!
+//! Decoding a [`std::time::Duration`] returns an error if the `TIME` value is negative.
+//!
 //! ### [`chrono`](https://crates.io/crates/chrono)
 //!
 //! Requires the `chrono` Cargo feature flag.
@@ -48,7 +56,20 @@
 //! | `chrono::DateTime<Local>`             | TIMESTAMP                                            |
 //! | `chrono::NaiveDateTime`               | DATETIME                                             |
 //! | `chrono::NaiveDate`                   | DATE                                                 |
-//! | `chrono::NaiveTime`                   | TIME                                                 |
+//! | `chrono::NaiveTime`                   | TIME (time-of-day only)                              |
+//! | `chrono::TimeDelta`                   | TIME (decodes full range; see note for encoding)     |
+//!
+//! ### NOTE: MySQL's `TIME` type is dual-purpose
+//! MySQL's `TIME` type can be used as either a time-of-day value, or an interval.
+//! However, `chrono::NaiveTime` is designed only to represent a time-of-day.
+//!
+//! Decoding a `TIME` value as `chrono::NaiveTime` will return an error if the value is out of range.
+//!
+//! The [`MySqlTime`] type supports the full range and it also implements `TryInto<chrono::NaiveTime>`.
+//!
+//! Decoding a `chrono::TimeDelta` also supports the full range.
+//!
+//! To encode a `chrono::TimeDelta`, convert it to [`MySqlTime`] first using `TryFrom`/`TryInto`.
 //!
 //! ### [`time`](https://crates.io/crates/time)
 //!
@@ -59,7 +80,20 @@
 //! | `time::PrimitiveDateTime`             | DATETIME                                             |
 //! | `time::OffsetDateTime`                | TIMESTAMP                                            |
 //! | `time::Date`                          | DATE                                                 |
-//! | `time::Time`                          | TIME                                                 |
+//! | `time::Time`                          | TIME (time-of-day only)                              |
+//! | `time::Duration`                      | TIME (decodes full range; see note for encoding)     |
+//!
+//! ### NOTE: MySQL's `TIME` type is dual-purpose
+//! MySQL's `TIME` type can be used as either a time-of-day value, or an interval.
+//! However, `time::Time` is designed only to represent a time-of-day.
+//!
+//! Decoding a `TIME` value as `time::Time` will return an error if the value is out of range.
+//!
+//! The [`MySqlTime`] type supports the full range, and it also implements `TryInto<time::Time>`.
+//!
+//! Decoding a `time::Duration` also supports the full range.
+//!
+//! To encode a `time::Duration`, convert it to [`MySqlTime`] first using `TryFrom`/`TryInto`.
 //!
 //! ### [`bigdecimal`](https://crates.io/crates/bigdecimal)
 //! Requires the `bigdecimal` Cargo feature flag.
@@ -102,11 +136,14 @@
 
 pub(crate) use sqlx_core::types::*;
 
+pub use mysql_time::{MySqlTime, MySqlTimeError, MySqlTimeSign};
+
 mod bool;
 mod bytes;
 mod float;
 mod inet;
 mod int;
+mod mysql_time;
 mod str;
 mod text;
 mod uint;