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

replace chrono with newer time #39

Merged
merged 2 commits into from
Feb 10, 2022

Conversation

JanZerebecki
Copy link
Contributor

@JanZerebecki JanZerebecki commented Jan 13, 2022

Cargo.toml Outdated Show resolved Hide resolved
@JanZerebecki
Copy link
Contributor Author

Ah I missed that time was already removed in #37 , this just needs a new release.

@JanZerebecki
Copy link
Contributor Author

Also can you please push the missing tags since 2.5.0?

@ShellWowza
Copy link

It's still useful to remove chrono as a dependency, so that cargo audit does not flag this crate under RUSTSEC-2020-0159. What needs to happen for this to merge and release?

@Techcable
Copy link
Member

Techcable commented Jan 27, 2022

In practice, it looks like thos CVE is of moderate severity. It requires an attacker to control environment variables and not just user input.

I'm not saying it's not important, I'm just saying this is not an emmegency. The most obvious option is to release a patch release (2.x.y) of this crate with only the relevant hotfix (79bfa8c).

If you are concerned about triggering this (somewhat rare) vulnerability in your code I would suggest applying the fix yourself via a git dependency.

If you are more concerned with security best practices, then I agree this is a concern, but definitely not an emergency.

As some context there has been some broader activity around the main slog crate. This includes updating to rust 2018 and bug fixes for optional featuree/no_std. There is nothing with default features yet. This is a good thing (yay for more maitnence) but it may mean a little bit longer wait for a formal release on crates.io 😦

@dreamer dreamer mentioned this pull request Feb 2, 2022
@Techcable
Copy link
Member

Does release 2.8.1 addresses your problem with chrono sufficiently? I would think 79bfa8c addresses the security concerns 😉

Besides this specific security issue (which is now fixed), are there any other advantages in switching from chrono to time?

@Techcable Techcable added C-Cleanup Category: Internal code cleanup (not affecting API) P-low Low priority labels Feb 9, 2022
@JanZerebecki
Copy link
Contributor Author

2.8.1 does avoid https://rustsec.org/advisories/RUSTSEC-2020-0071 . But from my quick peek there is no version of chrono available that has https://rustsec.org/advisories/RUSTSEC-2020-0159 fixed and there is no useful feature set of chrono that avoids both. Switching to time does avoid both. An alternative would be to fix the chrono issue, but that is not a quick thing for me. A downside of this PR might be that it uses time 0.2.x while it is already at 0.3.7, but finding the same functionality in the newer version of time was also not quick.

@Techcable
Copy link
Member

Blegh. Sounds like chrono is not well maintained right now 😦

I guess this probably should be a priority for us.

@Techcable Techcable added P-medium Medium Priority and removed P-low Low priority labels Feb 9, 2022
@Techcable
Copy link
Member

Interestingly enough, when using time = "0.3" the average binary sizes of the examples actually decrease compared to the latest master.

Binary sizes with time: 0.3: 798KB
Binary sizes with master (using chrono): 816KB

@Techcable
Copy link
Member

Techcable commented Feb 9, 2022

Okay. I updated to time 0.3 and fixed the exception.

Before this upgrade, cargo run --example "full" threw an exception for me.

thread '' panicked at 'Invalid specifier .', /Users/techcable/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/time-0.2.27/src/format/parse_items.rs:11:21

Also I accidentally force-pushed invalidating all the prior comments (sorry) 😉

JanZerebecki and others added 2 commits February 9, 2022 16:18
Binary sizes actually shrink compared to master, so
this is strictly an improvement ;)

Binary sizes with time: 0.3: 798KB
Binary sizes with master (using chrono): 816KB

Also this corrects the TIMESTAMP_FORMAT to work with time instead of chrono.

This is specific error caused by the old value of TIMESTAMP_FORMAT ("%b %d %H:%M:%S%.3f"):
> thread '' panicked at 'Invalid specifier .', /Users/techcable/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/time-0.2.27/src/format/parse_items.rs:11:21

when doing `cargo run --example "full-color"`
@Techcable
Copy link
Member

Techcable commented Feb 9, 2022

@JanZerebecki let me know what you think about the upgrade to time 0.3 and the new TIMESTAMP_FORMAT specifier.

Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@JanZerebecki
Copy link
Contributor Author

Thanks. Looks good.

@Techcable Techcable merged commit 769421c into slog-rs:master Feb 10, 2022
@JanZerebecki JanZerebecki deleted the replace-chrono-with-time branch February 14, 2022 11:17
Techcable added a commit that referenced this pull request Feb 20, 2022
Right now this is Rust 1.53

This commit doesn't change anything, it just documents what already exists.

This was auto-detected by the `cargo msrv` command.

Accepting #39 bumped this from 1.38 to 1.53
Techcable added a commit that referenced this pull request Feb 20, 2022
- Switch from chrono to time (PR #39)
- Bump MSRV
- Switch from Travis to Github Actions (this PR)
Techcable added a commit that referenced this pull request Feb 20, 2022
Right now this is Rust 1.53

This commit doesn't change anything, it just documents what already exists.

This was auto-detected by the `cargo msrv` command.

Accepting #39 bumped this from 1.38 to 1.53
Techcable added a commit that referenced this pull request Feb 20, 2022
- Switch from chrono to time (PR #39)
- Bump MSRV
- Switch from Travis to Github Actions (this PR)
dpc pushed a commit that referenced this pull request Feb 21, 2022
Right now this is Rust 1.53

This commit doesn't change anything, it just documents what already exists.

This was auto-detected by the `cargo msrv` command.

Accepting #39 bumped this from 1.38 to 1.53
dpc pushed a commit that referenced this pull request Feb 21, 2022
- Switch from chrono to time (PR #39)
- Bump MSRV
- Switch from Travis to Github Actions (this PR)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-Cleanup Category: Internal code cleanup (not affecting API) P-medium Medium Priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants