search

mdsteele / rust-cab

Rust library for encoding/decoding Windows cabinet (.cab) files
MIT License
16 stars 9 forks source link

Replace chrono with time #13

Closed Jake-Shadle closed 2 years ago

Jake-Shadle commented 2 years ago

This PR fixes 2 security advisories

These are both caused by chrono, as one is directly on chrono, and the other is due to it having a dependency on an ancient version of time which has a similar advisory that has been fixed in later versions. chrono is essentially unmaintained and has not seen a release in almost 2 years and the time crate includes all of the functionality needed by this crate. Thanks to the tests it was easy to use time instead and verify it works since this crate wasn't using any chrono features that don't have exact parallels in time, namely format strings.

This is however a breaking change since chrono was part of the public API, but I don't think it will be disruptive to users. One behavior change however was also needed in FileBuilder as it was using local time as the default value if not set by the user. Use of local time on unix platforms (not Windows) was the root cause of both of the above advisories. Unfortunately, the way the time crate worked around this deficiency of local time was to actually check the number of active threads at runtime to ensure there was only 1 thread running, or require that the user use --cfg unsound_local_offset at build time to opt-in to the unsound use of local time. Both of these restrictions are IMO unacceptable as they severely restrict downstream usage, so the introduced behavior change is that the default is now no longer local time, but UTC time. I considered this fine since, as the comment on set_datetime states, the CAB spec says "the actual definition is application-defined", and IMO UTC time is superior since it is consistent for every reader and writer, as, presumably, the local offset would need to be communicated in some way as otherwise a reader in a different time offset would get an incorrect time if it is assumed to be local.

This also updates some outdated dev-dependencies, namely winapi, as winapi 0.3 uses feature flags for all of the functionality in the crate now, so this only enables the few used by the tests, which should give noticeable improvements to build times. FWIW windows-sys includes the functions that are manually bound in the tests (eg CreateCompressor), but given this crate already works, and windows-sys has some issues I figured it was better to just bump winapi instead of replacing it.

mdsteele commented 2 years ago

Thanks very much for the fix and the detailed writeup! Switching to time, and using UTC instead of local time by default, both seem like reasonable breaking changes to me, under the circumstances.

Looks like the build is failing on Windows. At a glance, I think the use self::winapi declarations in mszip.rs need updating now that the extern crate line is removed?

Jake-Shadle commented 2 years ago

Oops, should have compiled on Windows before pushing, sorry about that. This also fixed a few cases where MaybeUninit was not being used correctly that clippy flagged.

mdsteele commented 2 years ago

Looks good, thanks again!