Repository: toml-rs/toml (language: rust)

Issue:
Incorrect parsing of sub-hour timezone offsets
toml-rs parses the following TOML document incorrectly:
```
d = 1911-01-01T10:11:12-00:36
```
This can be seen for example by running the example program `toml2json` which outputs:
```json
{
  "d": "1911-01-01T10:11:12+00:36"
}
```

It looks like the current `toml_datetime::Offset` structure is not even capable of representing a sub-hour negative offset.

This issue applies to the recent commit cc869e9dd5d87a2c7bee3fabcc94cbbed7538086 on main.

I'm experimenting with a TOML fuzzer which revealed this issue. I guess most normal applications are not likely to hit this case, so I understand if this gets low priority. It still seems worthwhile to fix it though.


Discussion / hints:

Fix patch:
diff --git a/crates/toml_datetime/src/datetime.rs b/crates/toml_datetime/src/datetime.rs
index d468c420..15781b91 100644
--- a/crates/toml_datetime/src/datetime.rs
+++ b/crates/toml_datetime/src/datetime.rs
@@ -94,9 +94,8 @@ pub struct Datetime {
 
 /// Error returned from parsing a `Datetime` in the `FromStr` implementation.
 #[derive(Debug, Clone)]
-pub struct DatetimeParseError {
-    _private: (),
-}
+#[non_exhaustive]
+pub struct DatetimeParseError {}
 
 // Currently serde itself doesn't have a datetime type, so we map our `Datetime`
 // to a special value in the serde data model. Namely one with these special
@@ -180,11 +179,8 @@ pub enum Offset {
 
     /// Offset between local time and UTC
     Custom {
-        /// Hours: -12 to +12
-        hours: i8,
-
-        /// Minutes: 0 to 59
-        minutes: u8,
+        /// Minutes: -1_440..1_440
+        minutes: i16,
     },
 }
 
@@ -247,7 +243,16 @@ impl fmt::Display for Offset {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match *self {
             Offset::Z => write!(f, "Z"),
-            Offset::Custom { hours, minutes } => write!(f, "{:+03}:{:02}", hours, minutes),
+            Offset::Custom { mut minutes } => {
+                let mut sign = '+';
+                if minutes < 0 {
+                    minutes *= -1;
+                    sign = '-';
+                }
+                let hours = minutes / 60;
+                let minutes = minutes % 60;
+                write!(f, "{}{:02}:{:02}", sign, hours, minutes)
+            }
         }
     }
 }
@@ -263,7 +268,7 @@ impl FromStr for Datetime {
         // 0000-00-00
         // 00:00:00.00
         if date.len() < 3 {
-            return Err(DatetimeParseError { _private: () });
+            return Err(DatetimeParseError {});
         }
         let mut offset_allowed = true;
         let mut chars = date.chars();
@@ -280,7 +285,7 @@ impl FromStr for Datetime {
 
             match chars.next() {
                 Some('-') => {}
-                _ => return Err(DatetimeParseError { _private: () }),
+                _ => return Err(DatetimeParseError {}),
             }
 
             let m1 = digit(&mut chars)?;
@@ -288,7 +293,7 @@ impl FromStr for Datetime {
 
             match chars.next() {
                 Some('-') => {}
-                _ => return Err(DatetimeParseError { _private: () }),
+                _ => return Err(DatetimeParseError {}),
             }
 
             let d1 = digit(&mut chars)?;
@@ -301,10 +306,10 @@ impl FromStr for Datetime {
             };
 
             if date.month < 1 || date.month > 12 {
-                return Err(DatetimeParseError { _private: () });
+                return Err(DatetimeParseError {});
             }
             if date.day < 1 || date.day > 31 {
-                return Err(DatetimeParseError { _private: () });
+                return Err(DatetimeParseError {});
             }
 
             Some(date)
@@ -326,13 +331,13 @@ impl FromStr for Datetime {
             let h2 = digit(&mut chars)?;
             match chars.next() {
                 Some(':') => {}
-                _ => return Err(DatetimeParseError { _private: () }),
+                _ => return Err(DatetimeParseError {}),
             }
             let m1 = digit(&mut chars)?;
             let m2 = digit(&mut chars)?;
             match chars.next() {
                 Some(':') => {}
-                _ => return Err(DatetimeParseError { _private: () }),
+                _ => return Err(DatetimeParseError {}),
             }
             let s1 = digit(&mut chars)?;
             let s2 = digit(&mut chars)?;
@@ -358,7 +363,7 @@ impl FromStr for Datetime {
                     }
                 }
                 if end == 0 {
-                    return Err(DatetimeParseError { _private: () });
+                    return Err(DatetimeParseError {});
                 }
                 chars = whole[end..].chars();
             }
@@ -371,16 +376,16 @@ impl FromStr for Datetime {
             };
 
             if time.hour > 24 {
-                return Err(DatetimeParseError { _private: () });
+                return Err(DatetimeParseError {});
             }
             if time.minute > 59 {
-                return Err(DatetimeParseError { _private: () });
+                return Err(DatetimeParseError {});
             }
             if time.second > 59 {
-                return Err(DatetimeParseError { _private: () });
+                return Err(DatetimeParseError {});
             }
             if time.nanosecond > 999_999_999 {
-                return Err(DatetimeParseError { _private: () });
+                return Err(DatetimeParseError {});
             }
 
             Some(time)
@@ -401,21 +406,29 @@ impl FromStr for Datetime {
                 let sign = match next {
                     Some('+') => 1,
                     Some('-') => -1,
-                    _ => return Err(DatetimeParseError { _private: () }),
+                    _ => return Err(DatetimeParseError {}),
                 };
                 chars.next();
-                let h1 = digit(&mut chars)? as i8;
-                let h2 = digit(&mut chars)? as i8;
+                let h1 = digit(&mut chars)? as i16;
+                let h2 = digit(&mut chars)? as i16;
                 match chars.next() {
                     Some(':') => {}
-                    _ => return Err(DatetimeParseError { _private: () }),
+                    _ => return Err(DatetimeParseError {}),
+                }
+                let m1 = digit(&mut chars)? as i16;
+                let m2 = digit(&mut chars)? as i16;
+
+                let hours = h1 * 10 + h2;
+                let minutes = m1 * 10 + m2;
+
+                let total_minutes = sign * (hours * 60 + minutes);
+
+                if !((-24 * 60)..=(24 * 60)).contains(&total_minutes) {
+                    return Err(DatetimeParseError {});
                 }
-                let m1 = digit(&mut chars)?;
-                let m2 = digit(&mut chars)?;
 
                 Some(Offset::Custom {
-                    hours: sign * (h1 * 10 + h2),
-                    minutes: m1 * 10 + m2,
+                    minutes: total_minutes,
                 })
             }
         } else {
@@ -425,7 +438,7 @@ impl FromStr for Datetime {
         // Return an error if we didn't hit eof, otherwise return our parsed
         // date
         if chars.next().is_some() {
-            return Err(DatetimeParseError { _private: () });
+            return Err(DatetimeParseError {});
         }
 
         Ok(Datetime {
@@ -439,7 +452,7 @@ impl FromStr for Datetime {
 fn digit(chars: &mut str::Chars<'_>) -> Result<u8, DatetimeParseError> {
     match chars.next() {
         Some(c) if ('0'..='9').contains(&c) => Ok(c as u8 - b'0'),
-        _ => Err(DatetimeParseError { _private: () }),
+        _ => Err(DatetimeParseError {}),
     }
 }
 
diff --git a/crates/toml_edit/src/parser/datetime.rs b/crates/toml_edit/src/parser/datetime.rs
index 5a0235a4..63a76165 100644
--- a/crates/toml_edit/src/parser/datetime.rs
+++ b/crates/toml_edit/src/parser/datetime.rs
@@ -75,17 +75,17 @@ pub(crate) fn partial_time(input: Input<'_>) -> IResult<Input<'_>, Time, ParserE
 pub(crate) fn time_offset(input: Input<'_>) -> IResult<Input<'_>, Offset, ParserError<'_>> {
     alt((
         one_of((b'Z', b'z')).value(Offset::Z),
-        (one_of((b'+', b'-')), cut((time_hour, b':', time_minute))).map(
-            |(sign, (hours, _, minutes))| {
-                let hours = hours as i8;
-                let hours = match sign {
-                    b'+' => hours,
-                    b'-' => -hours,
+        (one_of((b'+', b'-')), cut((time_hour, b':', time_minute)))
+            .map(|(sign, (hours, _, minutes))| {
+                let sign = match sign {
+                    b'+' => 1,
+                    b'-' => -1,
                     _ => unreachable!("Parser prevents this"),
                 };
-                Offset::Custom { hours, minutes }
-            },
-        ),
+                sign * (hours as i16 * 60 + minutes as i16)
+            })
+            .verify(|minutes| ((-24 * 60)..=(24 * 60)).contains(minutes))
+            .map(|minutes| Offset::Custom { minutes }),
     ))
     .context(Context::Expression("time offset"))
     .parse(input)
@@ -229,40 +229,196 @@ mod test {
     #[test]
     fn offset_date_time() {
         let inputs = [
-            "1979-05-27T07:32:00Z",
-            "1979-05-27T00:32:00-07:00",
-            "1979-05-27T00:32:00.999999-07:00",
+            (
+                "1979-05-27T07:32:00Z",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: Some(Time {
+                        hour: 7,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 0,
+                    }),
+                    offset: Some(Offset::Z),
+                },
+            ),
+            (
+                "1979-05-27T00:32:00-07:00",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: Some(Time {
+                        hour: 0,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 0,
+                    }),
+                    offset: Some(Offset::Custom { minutes: -7 * 60 }),
+                },
+            ),
+            (
+                "1979-05-27T00:32:00-00:36",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: Some(Time {
+                        hour: 0,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 0,
+                    }),
+                    offset: Some(Offset::Custom { minutes: -36 }),
+                },
+            ),
+            (
+                "1979-05-27T00:32:00.999999",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: Some(Time {
+                        hour: 0,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 999999000,
+                    }),
+                    offset: None,
+                },
+            ),
         ];
-        for input in inputs {
+        for (input, expected) in inputs {
             dbg!(input);
-            date_time.parse(new_input(input)).finish().unwrap();
+            let actual = date_time.parse(new_input(input)).finish().unwrap();
+            assert_eq!(expected, actual);
         }
     }
 
     #[test]
     fn local_date_time() {
-        let inputs = ["1979-05-27T07:32:00", "1979-05-27T00:32:00.999999"];
-        for input in inputs {
+        let inputs = [
+            (
+                "1979-05-27T07:32:00",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: Some(Time {
+                        hour: 7,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 0,
+                    }),
+                    offset: None,
+                },
+            ),
+            (
+                "1979-05-27T00:32:00.999999",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: Some(Time {
+                        hour: 0,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 999999000,
+                    }),
+                    offset: None,
+                },
+            ),
+        ];
+        for (input, expected) in inputs {
             dbg!(input);
-            date_time.parse(new_input(input)).finish().unwrap();
+            let actual = date_time.parse(new_input(input)).finish().unwrap();
+            assert_eq!(expected, actual);
         }
     }
 
     #[test]
     fn local_date() {
-        let inputs = ["1979-05-27", "2017-07-20"];
-        for input in inputs {
+        let inputs = [
+            (
+                "1979-05-27",
+                Datetime {
+                    date: Some(Date {
+                        year: 1979,
+                        month: 5,
+                        day: 27,
+                    }),
+                    time: None,
+                    offset: None,
+                },
+            ),
+            (
+                "2017-07-20",
+                Datetime {
+                    date: Some(Date {
+                        year: 2017,
+                        month: 7,
+                        day: 20,
+                    }),
+                    time: None,
+                    offset: None,
+                },
+            ),
+        ];
+        for (input, expected) in inputs {
             dbg!(input);
-            date_time.parse(new_input(input)).finish().unwrap();
+            let actual = date_time.parse(new_input(input)).finish().unwrap();
+            assert_eq!(expected, actual);
         }
     }
 
     #[test]
     fn local_time() {
-        let inputs = ["07:32:00", "00:32:00.999999"];
-        for input in inputs {
+        let inputs = [
+            (
+                "07:32:00",
+                Datetime {
+                    date: None,
+                    time: Some(Time {
+                        hour: 7,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 0,
+                    }),
+                    offset: None,
+                },
+            ),
+            (
+                "00:32:00.999999",
+                Datetime {
+                    date: None,
+                    time: Some(Time {
+                        hour: 0,
+                        minute: 32,
+                        second: 0,
+                        nanosecond: 999999000,
+                    }),
+                    offset: None,
+                },
+            ),
+        ];
+        for (input, expected) in inputs {
             dbg!(input);
-            date_time.parse(new_input(input)).finish().unwrap();
+            let actual = date_time.parse(new_input(input)).finish().unwrap();
+            assert_eq!(expected, actual);
         }
     }
 


Test patch:
diff --git a/crates/toml/tests/testsuite/serde.rs b/crates/toml/tests/testsuite/serde.rs
index 88990f23..8bbff56e 100644
--- a/crates/toml/tests/testsuite/serde.rs
+++ b/crates/toml/tests/testsuite/serde.rs
@@ -1050,3 +1050,11 @@ fn serialize_datetime_issue_333() {
     .unwrap();
     assert_eq!(toml, "date = 2022-01-01\n");
 }
+
+#[test]
+fn datetime_offset_issue_496() {
+    let original = "value = 1911-01-01T10:11:12-00:36\n";
+    let toml = original.parse::<toml::Table>().unwrap();
+    let output = toml.to_string();
+    snapbox::assert_eq(original, output);
+}