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

Remove time dependency from date #4952

Merged
merged 4 commits into from
Jun 6, 2023

Conversation

wanderinglethe
Copy link
Contributor

Also upgrades humantime_to_duration to the latest version, it switched from time to chrono. Because touch still depends on time, its humantime_to_duration package version deviates from the workspace version.

Fixes #4932

Also upgrades humantime_to_duration to the latest version, it switched from time to chrono. Because touch still depends on time, its humantime_to_duration package version deviates from the workspace version.
Copy link
Member

@tertsdiepraam tertsdiepraam left a comment

Choose a reason for hiding this comment

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

That was surprisingly easy! I like the choice of letting date use the workspace dependency and change the Cargo.toml of touch.

Edit: There's a little issue with cargo deny still. In my opinion, this can be merged once that's fixed and the rest of the CI is green.

Duplicates of humantime_to_duration and time can be removed when touch
switches from time to chrono
@wanderinglethe
Copy link
Contributor Author

wanderinglethe commented Jun 5, 2023

Thank you, I have added the duplicates to skipped bans

@cakebaker
Copy link
Contributor

@tertsdiepraam I would wait with merging this PR until there is a new version of humantime_to_duration that fixes the issues shown by cargo deny.

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Jun 6, 2023

@cakebaker Is it humantime's fault? I thought the problem was just that we now have two versions of humantime (temporarily).

@wanderinglethe
Copy link
Contributor Author

wanderinglethe commented Jun 6, 2023

I thought the problem was just that we now have two versions of humantime (temporarily).

This was my understanding as well

@cakebaker
Copy link
Contributor

cakebaker commented Jun 6, 2023

@tertsdiepraam yes, humantime 0.3.0 introduces some unnecessary dependencies, among them a "bad" version of time with a security vulnerability (see https://github.com/uutils/coreutils/actions/runs/5177082517/jobs/9326736548?pr=4936). It's fixed in the humantime repo and there shouldn't be any changes needed to deny.toml.

@wanderinglethe
Copy link
Contributor Author

wanderinglethe commented Jun 6, 2023

humantime 0.3.0 introduces some unnecessary dependencies, among them a "bad" version of time with a security vulnerability

It is actually chrono that still depends on time, it looks like they are talking about removing it
chronotope/chrono#1073

@wanderinglethe
Copy link
Contributor Author

I'm sorry, I see this is fixable by removing the old-time feature from chrono.
Which has also been done in humantime uutils/parse_datetime#14

@tertsdiepraam
Copy link
Member

Excellent! We'll wait a bit for the next release then. Thanks @cakebaker!

@sylvestre
Copy link
Contributor

I pushed a new reelease of this crate (0.3.1)

Latest patch fixes the transitive dependency on time. Which can now
also be removed from the deny skipped bans.
@wanderinglethe
Copy link
Contributor Author

I have upgraded humantime_to_duration and indeed we do not have a transitive dependency on time anymore. Thank you @cakebaker

Terts, could you take another look?

@uutils uutils deleted a comment from github-actions bot Jun 6, 2023
@sylvestre
Copy link
Contributor

Fuzzing failed because of -d1111111111111111m-m:


INFO: Loaded 1 modules   (348311 inline 8-bit counters): 348311 [0x55c67e42f9e0, 0x55c67e484a77),
INFO: Loaded 1 PC tables (348311 PCs): 348311 [0x55c67e484a78,0x55c67e9d53e8),
target/x86_64-unknown-linux-gnu/release/fuzz_date: Running 1 inputs 1 time(s) each.
Running: artifacts/fuzz_date/minimized-from-e8f0af57f8497535ea1d2aaee3e99d8798145651
thread '<unnamed>' panicked at 'Duration::seconds out of bounds', /home/sylvestre/.cargo/registry/src/index.crates.io-6f17d22bba15001f/chrono-0.4.26/src/oldtime.rs:121:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
==2970437== ERROR: libFuzzer: deadly signal
    #0 0x55c67ceec581 in __sanitizer_print_stack_trace /rustc/llvm/src/llvm-project/compiler-rt/lib/asan/asan_stack.cpp:87:3
    #1 0x55c67df3423a in fuzzer::PrintStackTrace() (/home/sylvestre/dev/debian/coreutils/fuzz/target/x86_64-unknown-linux-gnu/release/fuzz_date+0x1a5d23a) (BuildId: 167227b0a3d3b402545f11d986e00e206ea74582)
    #2 0x55c67df1b835 in fuzzer::Fuzzer::CrashCallback() (/home/sylvestre/dev/debian/coreutils/fuzz/target/x86_64-unknown-linux-gnu/release/fuzz_date+0x1a44835) (BuildId: 167227b0a3d3b402545f11d986e00e206ea74582)
    #3 0x7f4fedb2bf8f  (/lib/x86_64-linux-gnu/libc.so.6+0x3bf8f) (BuildId: 0401bd8da6edab3e45399d62571357ab12545133)
    #4 0x7f4fedb7accb  (/lib/x86_64-linux-gnu/libc.so.6+0x8accb) (BuildId: 0401bd8da6edab3e45399d62571357ab12545133)
    #5 0x7f4fedb2bef1 in raise (/lib/x86_64-linux-gnu/libc.so.6+0x3bef1) (BuildId: 0401bd8da6edab3e45399d62571357ab12545133)
    #6 0x7f4fedb16471 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x26471) (BuildId: 0401bd8da6edab3e45399d62571357ab12545133)
    #7 0x55c67df86216 in std::sys::unix::abort_internal::h7bb3497fe3ded23f /rustc/0c61c7a978fe9f7b77a1d667c77d2202dadd1c10/library/std/src/sys/unix/mod.rs:350:14
    #8 0x55c67ce46836 in std::process::abort::h1fe2a5b3a1eea0b9 /rustc/0c61c7a978fe9f7b77a1d667c77d2202dadd1c10/library/std/src/process.rs:2138:5
    #9 0x55c67def0b14 in libfuzzer_sys::initialize::_$u7b$$u7b$closure$u7d$$u7d$::hc7f98b2e7c51d196 (/home/sylvestre/dev/debian/coreutils/fuzz/target/x86_64-unknown-linux-gnu/release/fuzz_date+0x1a19b14) (BuildId: 167227b0a3d3b402545f11d986e00e206ea74582)
    #10 0x55c67df7ab33 in _$LT$alloc..boxed..Box$LT$F$C$A$GT$$u20$as$u20$core..ops..function..Fn$LT$Args$GT$$GT$::call::ha85a7263ce5c1565 /rustc/0c61c7a978fe9f7b77a1d667c77d2202dadd1c10/library/alloc/src/boxed.rs:2002:9
    #11 0x55c67df7ab33 in std::panicking::rust_panic_with_hook::hfd9594253b2174d9 /rustc/0c61c7a978fe9f7b77a1d667c77d2202dadd1c10/library/std/src/panicking.rs:696:13
    #12 0x55c67df7a861 in std::panicking::begin_panic_handler::_$u7b$$u7b$closure$u7d$$u7d$::h0cbfa1758f851517 /rustc/0c61c7a978fe9f7b77a1d667c77d2202dadd1c10/library/std/src/panicking.rs:581:13
    #13 0x55c67df77d95 in std::sys_common::backtrace::__rust_end_short_backtrace::ha6a4a6b805e2aab8 /rustc/0c61c7a978fe9f7b77a1d667c77d2202dadd1c10/library/std/src/sys_common/backtrace.rs:150:18
    #14 0x55c67df7a601 in rust_begin_unwind /rustc/0c61c7a978fe9f7b77a1d667c77d2202dadd1c10/library/std/src/panicking.rs:579:5
    #15 0x55c67ce4a1a2 in core::panicking::panic_fmt::h46ad3c1c4689a685 /rustc/0c61c7a978fe9f7b77a1d667c77d2202dadd1c10/library/core/src/panicking.rs:67:14
    #16 0x55c67cfc11bd in chrono::oldtime::Duration::minutes::h9e9cbb2f12345236 (/home/sylvestre/dev/debian/coreutils/fuzz/target/x86_64-unknown-linux-gnu/release/fuzz_date+0xaea1bd) (BuildId: 167227b0a3d3b402545f11d986e00e206ea74582)
    #17 0x55c67cfca48e in humantime_to_duration::from_str_at_date::h00d92ac683b6c7ee (/home/sylvestre/dev/debian/coreutils/fuzz/target/x86_64-unknown-linux-gnu/release/fuzz_date+0xaf348e) (BuildId: 167227b0a3d3b402545f11d986e00e206ea74582)
    #18 0x55c67cfc8f30 in humantime_to_duration::from_str::h3d7abccfaf481076 (/home/sylvestre/dev/debian/coreutils/fuzz/target/x86_64-unknown-linux-gnu/release/fuzz_date+0xaf1f30) (BuildId: 167227b0a3d3b402545f11d986e00e206ea74582)


@sylvestre
Copy link
Contributor

landing as it is from humantime_to_duration

@sylvestre sylvestre merged commit 6d6966c into uutils:main Jun 6, 2023
@sylvestre
Copy link
Contributor

opened here:
uutils/parse_datetime#18

@wanderinglethe
Copy link
Contributor Author

Thank you sylvestre

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 this pull request may close these issues.

date should use the chrono crate and not time
4 participants