Skip to content

fix(prost-types): Fix date-time parsing #1096

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 12, 2024
Merged

fix(prost-types): Fix date-time parsing #1096

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c528cb1
upstream date-time parsing fixes
mumbleskates Jul 7, 2024
a299713
add regression tests
mumbleskates Jul 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions prost-types/src/datetime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,9 @@ fn parse_offset(s: &str) -> Option<(i8, i8, &str)> {
/// string.
fn parse_two_digit_numeric(s: &str) -> Option<(u8, &str)> {
debug_assert!(s.is_ascii());

if s.len() < 2 {
return None;
}
let (digits, s) = s.split_at(2);
Some((digits.parse().ok()?, s))
}
Expand Down Expand Up @@ -420,8 +422,8 @@ pub(crate) fn year_to_seconds(year: i64) -> (i128, bool) {
let is_leap;
let year = year - 1900;

// Fast path for years 1900 - 2038.
if year as u64 <= 138 {
// Fast path for years 1901 - 2038.
if (1..=138).contains(&year) {
Copy link
Contributor

@caspermeijn caspermeijn Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So basically, you are saying that the fast path was incorrect for 1900, but the slow path is correct. I can't reason why that is, but I assume that you are right :-)

Copy link
Contributor Author

@mumbleskates mumbleskates Jul 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cited function in musl actually writes this condition as if (year-2ULL <= 136) {... which isn't the same thing as if year as u64 <= 138 { at all! if year is either 0 or 1 in the original function it wraps to max.

the musl code's fast-path works within i32 seconds of the epoch, which ranges from 1901-12-13 to 2038-01-18, so it actually skips the fast-path for 1901 as well because the start of that year is an underflow. for our code, we are calculating in i64 and returning an i128 anyway so it doesn't matter. (the code in this pr is still equally correct here if we write if (1..=199).contains(&year) { :) )

what DOES matter is that the fast-path ignores the century rule for leap years, because 2000 is a multiple of 400 and happens to be one! it's also the only century that occurs in the i32 epoch range. so the fast-path logic is actually WRONG when applied to any year outside 1901..=2099, even without integer wrapping.

Copy link
Contributor

@caspermeijn caspermeijn Jul 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand how this could happen. That is a subtle, but important difference. Thanks for the extensive explanation!

let mut leaps: i64 = (year - 68) >> 2;
if (year - 68).trailing_zeros() >= 2 {
leaps -= 1;
Expand Down Expand Up @@ -769,6 +771,19 @@ mod tests {
"2020-06-15 00:01:02.123 +0800".parse::<Timestamp>(),
Timestamp::date_time_nanos(2020, 6, 14, 16, 1, 2, 123_000_000),
);

// Regression tests
assert_eq!(
"-11111111-z".parse::<Timestamp>(),
Err(crate::TimestampError::ParseFailure),
);
assert_eq!(
"1900-01-10".parse::<Timestamp>(),
Ok(Timestamp {
seconds: -2208211200,
nanos: 0
}),
);
}

#[test]
Expand Down