-
Notifications
You must be signed in to change notification settings - Fork 481
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
fix(prost-types): Fix date-time parsing #1096
Conversation
bc33b67
to
b3d4187
Compare
b3d4187
to
a299713
Compare
Here's another suggested fix for some weirdness that fuzzing failures highlighted: mumbleskates/bilrost@2e43e02 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution
// Fast path for years 1900 - 2038. | ||
if year as u64 <= 138 { | ||
// Fast path for years 1901 - 2038. | ||
if (1..=138).contains(&year) { |
There was a problem hiding this comment.
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 :-)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
Could you create a PR for that? Thanks. |
Two bugs revealed so far from fuzz testing in
bilrost
:parse_two_digit_numeric
does not check the length of its input before callingsplit_at
, potentially causing a panicyear_to_seconds
from the original musl implementation caused it to treat the year 1900 as a leap year, causing (i believe) every date from 1900-01-01 to 1900-03-01 to be offset one day into the past when converting fromDateTime
toTimestamp
.