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.

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

Jump to bottom

Fix panic in DateTime::checked_add_days #941

Closed
wants to merge 1 commit into from
Closed

Fix panic in DateTime::checked_add_days #941

wants to merge 1 commit into from

Conversation

Ekleog
Copy link
Contributor

@Ekleog Ekleog commented Jan 22, 2023

Incidentally, add TimeDelta::try_* builders so that it's possible to build them without risking a panic.

This issue was found thanks to cargo-bolero-fuzzing my web server, without actually trying to fuzz chrono itself.

@Ekleog Ekleog mentioned this pull request Jan 22, 2023
Closed
@Ekleog
Fix panic in DateTime::checked_add_days
9262c43 
Incidentally, add TimeDelta::try_* builders so that it's possible to
build them without risking a panic.
@Ekleog Ekleog force-pushed the fix-days-addition-panic branch from 0f1a635 to 9262c43 Compare January 22, 2023 19:04
Ekleog added a commit to Ekleog/chrono that referenced this pull request Jan 22, 2023
@Ekleog
Fix panic in DateTime::checked_add_days
6e7034f 
This is a backport of chronotope#941, except it needs to work around the fact that
we can't modify the `time` crate.
@Ekleog Ekleog mentioned this pull request Jan 22, 2023
Merged
Ekleog added a commit to Ekleog/chrono that referenced this pull request Jan 22, 2023
@Ekleog
Fix panic in DateTime::checked_add_days
c947588 
This is a backport of chronotope#941, except it needs to work around the fact that
we can't modify the `time` crate.
Ekleog added a commit to Ekleog/chrono that referenced this pull request Jan 22, 2023
@Ekleog
Fix panic in DateTime::checked_add_days
425d723 
This is a backport of chronotope#941, except it needs to work around the fact that
we can't modify the `time` crate.
@djc
Copy link
Member

djc commented Jan 23, 2023

Going to close this. We'll review (and probably merge) #942 (the 0.4.x version of this change) and then eventually that change will make it to the main branch as part of a regular merge of 0.4.x work into main.

@djc djc closed this Jan 23, 2023
@Ekleog
Copy link
Contributor Author

Ekleog commented Jan 23, 2023

Hmm so I had opened both because this fix is IMO better than the one in #941 , as it can actually add some API surface to TimeDelta and doesn't need to rely on knowledge of Duration's internals. Now you're the one maintaining chrono so if the fix in #941 is good enough for you for 0.5 then as you wish, I just find it pretty ugly :)

@djc
Copy link
Member

djc commented Jan 23, 2023

Ahh, sorry, had missed that context. Will open this back up, then we can review it after the other change has made its way into main.

@djc djc reopened this Jan 23, 2023
@Ekleog
Copy link
Contributor Author

Ekleog commented Jan 28, 2023

Awesome thanks! :)

djc pushed a commit that referenced this pull request Mar 3, 2023
@Ekleog @djc
Fix panic in DateTime::checked_add_days
f9f3c78 
This is a backport of #941, except it needs to work around the fact that
we can't modify the `time` crate.
@djc
Copy link
Member

djc commented Mar 17, 2023

(Merge is pending in #986.)

@djc
Copy link
Member

djc commented Mar 17, 2023

Can you rebase this onto main? Should hopefully not be too painful.

esheppa
esheppa reviewed Mar 18, 2023
View reviewed changes
Copy link
Collaborator

@esheppa esheppa left a comment

Choose a reason for hiding this comment

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

Looks good to me, pending rebase

jtmoon79
jtmoon79 suggested changes Apr 5, 2023
View reviewed changes
Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

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

Can you add test cases for the before and after for all changed functions?
e.g. test_diff_days, test_weeks, test_try_weeks, etc.

Specifically, two commits

  1. a commit with new test cases
  2. a commit of these changes + adjustments to those test cases (if needed)

@Ekleog
Copy link
Contributor Author

Ekleog commented May 13, 2023

Sorry for the delay. I have basically not done any non-work software development for a few months, and have a few personal projects that will be higher priority for me than this PR, so I don't think I'll actually come back to this anytime soon.

Do you prefer if I leave the PR open so that someone else can easily notice it and take it over (and so that it tracks the fact that the panic is currently reachable on main), or should I just close it?

@pitdicker
Copy link
Collaborator

@Ekleog That is very reasonable. If it is ok with you I can take over in a couple of days.

@Ekleog
Copy link
Contributor Author

Ekleog commented Jun 5, 2023

That'd be perfect, thank you! :)

@pitdicker pitdicker mentioned this pull request Jun 8, 2023
Closed
@pitdicker
Copy link
Collaborator

The new methods are included in #1137 together with other features for Duration. They are not yet merged however.

@pitdicker pitdicker closed this Jul 17, 2023
@pitdicker pitdicker mentioned this pull request Sep 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@esheppa esheppa esheppa left review comments

@jtmoon79 jtmoon79 jtmoon79 requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ekleog @djc @pitdicker @jtmoon79 @esheppa