Skip to content
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

Panic in parse_from_rfc2822 with overflowing date #645

Closed
5225225 opened this issue Jan 22, 2022 · 8 comments · Fixed by #686
Closed

Panic in parse_from_rfc2822 with overflowing date #645

5225225 opened this issue Jan 22, 2022 · 8 comments · Fixed by #686

Comments

@5225225
Copy link

5225225 commented Jan 22, 2022

#[test]
fn panic() {
    chrono::DateTime::parse_from_rfc2822("31 DEC 262143 23:59 -2359");
}
@djc
Copy link
Contributor

djc commented Mar 23, 2022

Sorry for the slow response -- would you be able to submit a PR for this, including this as a test?

@botahamec
Copy link
Contributor

I took a bit of a look at this. The panic seems to be caused by this function in offset/mod.rs

/// Converts the local `NaiveDateTime` to the timezone-aware `DateTime` if possible.
#[allow(clippy::wrong_self_convention)]
fn from_local_datetime(&self, local: &NaiveDateTime) -> LocalResult<DateTime<Self>> {
    self.offset_from_local_datetime(local)
        .map(|offset| DateTime::from_utc(*local - offset.fix(), offset))
}

I don't think it makes sense to return a LocalResult::None here. The function could be refactored to return an option or a result. I could also try changing Parsed::to_datetime to return OUT_OF_RANGE if the date is too large:

/// Returns a parsed timezone-aware date and time out of given fields.
///
/// This method is able to determine the combined date and time
/// from date and time fields or a single [`timestamp`](#structfield.timestamp) field,
/// plus a time zone offset.
/// Either way those fields have to be consistent to each other.
pub fn to_datetime(&self) -> ParseResult<DateTime<FixedOffset>> {
    let offset = self.offset.ok_or(NOT_ENOUGH)?;
    let datetime = self.to_naive_datetime_with_offset(offset)?;
    let offset = FixedOffset::east_opt(offset).ok_or(OUT_OF_RANGE)?;
    match offset.from_local_datetime(&datetime) {
        LocalResult::None => Err(IMPOSSIBLE),
        LocalResult::Single(t) => Ok(t),
        LocalResult::Ambiguous(..) => Err(NOT_ENOUGH),
    }
}

I'm wondering what would be preferable.

@djc
Copy link
Contributor

djc commented May 5, 2022

We can't change the public API for now. I guess we could (a) clearly document panicking in the method documentation and (b) deprecate the method in favor of an alternative that returns a Result.

@5225225
Copy link
Author

5225225 commented May 5, 2022

Well, it already returns a ParseResult, so we could do the same as what we do if it's 31 DEC 200000000 00:00 -1000 and return Err(ParseError(OutOfRange)). Maybe not strictly true, but it's more useful than panicking.

@djc
Copy link
Contributor

djc commented May 5, 2022

Oh yeah, that sounds good to me!

@botahamec
Copy link
Contributor

I got the test to pass by adding

datetime
    .checked_sub_signed(time::Duration::seconds(i64::from(offset.local_minus_utc())))
    .ok_or(OUT_OF_RANGE)?;

but that feels hacky. Maybe a better solution would be to add a FixedOffset::checked_from_local_datetime that isn't part of the TimeZone trait? If the answer is that I'm overthinking it, that's cool too. It just feels weird since it's very similar to what's already happening inside the from_local_datetime.

@djc
Copy link
Contributor

djc commented May 5, 2022

Stuff is getting reorganized in #677 -- probably pays off to keep it simple?

@botahamec
Copy link
Contributor

Alright then. I'll make a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants