`Timestamp::from_rfc4122` panics when using timestamp before unix epoch

zheland commented

2023-05-29 10:31:52 -05:00

(Migrated from github.com)

Timestamp::from_rfc4122 method panics with attempt to subtract with overflow error when passing ticks value less than UUID_TICKS_BETWEEN_EPOCHS.

I'm not sure that the intentional use of a UUID with the wrong timestamp can be considered acceptable by the standard. But even if this is considered unacceptable, a # Panics section describing that the Timestamp::from_rfc4122 method panics at certain values would be useful in method description.

The UUID crate allowed these values up to and including version 1.1.2. In UUID v1.1.2 the timestamp stored RFC4122 ticks. After UUID v1.2.1 Timestamp started to use unix time instead.

use uuid;

#[test]
fn any_timestamp_value_supported() {
    uuid::Timestamp::from_rfc4122(u64::MAX, 0); // ok
    uuid::Timestamp::from_rfc4122(122192928000000000, 0); // ok
    uuid::Timestamp::from_rfc4122(122192927999999999, 0); // panics
    uuid::Timestamp::from_rfc4122(0, 0); // panics
}

thread 'any_timestamp_value_supported' panicked at 'attempt to subtract with overflow', /playground/.cargo/registry/src/github.com-1ecc6299db9ec823/uuid-1.3.2/src/timestamp.rs:135:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Code from the UUID crate for reference:
github.com/uuid-rs/uuid@6188ecf96c/src/timestamp.rs (L25-L27)
github.com/uuid-rs/uuid@6188ecf96c/src/timestamp.rs (L75-L89)
github.com/uuid-rs/uuid@6188ecf96c/src/timestamp.rs (L133-L138)

`Timestamp::from_rfc4122` method panics with `attempt to subtract with overflow` error when passing ticks value less than `UUID_TICKS_BETWEEN_EPOCHS`. I'm not sure that the intentional use of a UUID with the wrong timestamp can be considered acceptable by the standard. But even if this is considered unacceptable, a `# Panics` section describing that the `Timestamp::from_rfc4122` method panics at certain values would be useful in method description. The UUID crate allowed these values up to and including version `1.1.2`. In UUID `v1.1.2` the timestamp stored RFC4122 ticks. After UUID `v1.2.1` Timestamp started to use unix time instead. ```rust use uuid; #[test] fn any_timestamp_value_supported() { uuid::Timestamp::from_rfc4122(u64::MAX, 0); // ok uuid::Timestamp::from_rfc4122(122192928000000000, 0); // ok uuid::Timestamp::from_rfc4122(122192927999999999, 0); // panics uuid::Timestamp::from_rfc4122(0, 0); // panics } ``` ``` thread 'any_timestamp_value_supported' panicked at 'attempt to subtract with overflow', /playground/.cargo/registry/src/github.com-1ecc6299db9ec823/uuid-1.3.2/src/timestamp.rs:135:13 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` Code from the UUID crate for reference: https://github.com/uuid-rs/uuid/blob/6188ecf96c66f06d31d84784c50da182ae5fbb64/src/timestamp.rs#L25-L27 https://github.com/uuid-rs/uuid/blob/6188ecf96c66f06d31d84784c50da182ae5fbb64/src/timestamp.rs#L75-L89 https://github.com/uuid-rs/uuid/blob/6188ecf96c66f06d31d84784c50da182ae5fbb64/src/timestamp.rs#L133-L138

KodrAus commented

2023-05-29 17:33:30 -05:00

(Migrated from github.com)

Thanks for the report @zheland. I think we've got two options here:

Enshrine the panic by making it explicit (not just debug) and add a section to the docs.
Truncate the timestamp so those before the epoch will be set to the epoch, along with a section in the docs.

Do either of those options appeal to you more than the other?

Thanks for the report @zheland. I think we've got two options here: 1. Enshrine the panic by making it explicit (not just debug) and add a section to the docs. 2. Truncate the timestamp so those before the epoch will be set to the epoch, along with a section in the docs. Do either of those options appeal to you more than the other?

zheland commented

2023-05-30 06:58:46 -05:00

(Migrated from github.com)

@KodrAus,

I don't like panic variant. I do not think that generating UUID with a time value smaller than unix epoch is a good reason for a panic. It seems to me that such things should be forgiven quietly in most UUID generation usecases.
Truncating seems much better to me, but with an incorrect time value it may result in a very similar generated UUIDs for UUIDv1 and UUIDv6. However, if considering that the use of incorrect time is rather wrong, then this option is basically completely fine.
Alternatively, I might suggest using wrapping operations when converting between rfc4122 and unix time. This will make it possible to obtain some unix time value from any rfc4122. And if necessary convert it back to the same rfc4122 timestamp.
Pros: UUIDv1 will support any valid rfc4122 timestamp and UUIDv7 will support any valid unix timestamp.
Cons: Possible silent wrapping operations when using invalid unix time to generate UUIDv1 or silent wrapping operations when using invalid rfc4122 to generate UUIDv7. If mentioned in the documentation it looks quite tolerable behavior.

// draft example, I have not checked the implementation in detail

pub const UUID_TICKS_BETWEEN_EPOCHS: u64 = 0x01B2_1DD2_1381_4000;
pub const UUID_SECS_BETWEEN_EPOCHS: u64 = UUID_TICKS_BETWEEN_EPOCHS / 10_000_000;

const fn unix_to_rfc4122_ticks(seconds: u64, nanos: u32) -> u64 {
    seconds
        .wrapping_add(UUID_SECS_BETWEEN_EPOCHS)
        .wrapping_mul(10_000_000)
        .wrapping_add(nanos as u64 / 100)
}

const fn rfc4122_to_unix(ticks: u64) -> (u64, u32) {
    (
        (ticks / 10_000_000).wrapping_sub(UUID_SECS_BETWEEN_EPOCHS),
        (ticks % 10_000_000) as u32 * 100,
    )
}

#[test]
fn any_rfc4122_can_be_mapped_to_the_corresponding_unixtime_and_back() {
    for (rfc4122, unix) in [
        (u64::MAX, (0x0000_01AA_A6D6_0F4A, 955_161_500)),
        (u64::MAX / 2 + 1, (0x0000_00D3_E741_3965, 477_580_800)),
        (UUID_TICKS_BETWEEN_EPOCHS, (0, 0)),
        (UUID_TICKS_BETWEEN_EPOCHS - 1, (u64::MAX, 999_999_900)),
        (10_000_000, (0xFFFF_FFFD_27AC_6381, 0)),
        (1, (0xFFFF_FFFD_27AC_6380, 100)),
        (0, (0xFFFF_FFFD_27AC_6380, 0)),
    ] {
        assert_eq!(rfc4122_to_unix(rfc4122), unix);
        assert_eq!(unix_to_rfc4122_ticks(unix.0, unix.1), rfc4122);
    }
}

#[test]
fn not_any_unixtime_can_be_mapped_to_the_corresponding_rfc4122() {
    assert_eq!(unix_to_rfc4122_ticks(0, 0), UUID_TICKS_BETWEEN_EPOCHS);
    assert_eq!(unix_to_rfc4122_ticks(0, 99), UUID_TICKS_BETWEEN_EPOCHS);
    assert_eq!(
        unix_to_rfc4122_ticks(0xFE00_0000_0000_0000, 0),
        UUID_TICKS_BETWEEN_EPOCHS
    );
}

@KodrAus, 1. I don't like panic variant. I do not think that generating UUID with a time value smaller than unix epoch is a good reason for a panic. It seems to me that such things should be forgiven quietly in most UUID generation usecases. 2. Truncating seems much better to me, but with an incorrect time value it may result in a very similar generated UUIDs for `UUIDv1` and `UUIDv6`. However, if considering that the use of incorrect time is rather wrong, then this option is basically completely fine. 3. Alternatively, I might suggest using wrapping operations when converting between rfc4122 and unix time. This will make it possible to obtain some unix time value from any rfc4122. And if necessary convert it back to the same rfc4122 timestamp. **Pros**: `UUIDv1` will support any valid **rfc4122** timestamp and `UUIDv7` will support any valid **unix** timestamp. **Cons**: Possible silent wrapping operations when using invalid **unix** time to generate `UUIDv1` or silent wrapping operations when using invalid **rfc4122** to generate `UUIDv7`. If mentioned in the documentation it looks quite tolerable behavior. ```rust // draft example, I have not checked the implementation in detail pub const UUID_TICKS_BETWEEN_EPOCHS: u64 = 0x01B2_1DD2_1381_4000; pub const UUID_SECS_BETWEEN_EPOCHS: u64 = UUID_TICKS_BETWEEN_EPOCHS / 10_000_000; const fn unix_to_rfc4122_ticks(seconds: u64, nanos: u32) -> u64 { seconds .wrapping_add(UUID_SECS_BETWEEN_EPOCHS) .wrapping_mul(10_000_000) .wrapping_add(nanos as u64 / 100) } const fn rfc4122_to_unix(ticks: u64) -> (u64, u32) { ( (ticks / 10_000_000).wrapping_sub(UUID_SECS_BETWEEN_EPOCHS), (ticks % 10_000_000) as u32 * 100, ) } #[test] fn any_rfc4122_can_be_mapped_to_the_corresponding_unixtime_and_back() { for (rfc4122, unix) in [ (u64::MAX, (0x0000_01AA_A6D6_0F4A, 955_161_500)), (u64::MAX / 2 + 1, (0x0000_00D3_E741_3965, 477_580_800)), (UUID_TICKS_BETWEEN_EPOCHS, (0, 0)), (UUID_TICKS_BETWEEN_EPOCHS - 1, (u64::MAX, 999_999_900)), (10_000_000, (0xFFFF_FFFD_27AC_6381, 0)), (1, (0xFFFF_FFFD_27AC_6380, 100)), (0, (0xFFFF_FFFD_27AC_6380, 0)), ] { assert_eq!(rfc4122_to_unix(rfc4122), unix); assert_eq!(unix_to_rfc4122_ticks(unix.0, unix.1), rfc4122); } } #[test] fn not_any_unixtime_can_be_mapped_to_the_corresponding_rfc4122() { assert_eq!(unix_to_rfc4122_ticks(0, 0), UUID_TICKS_BETWEEN_EPOCHS); assert_eq!(unix_to_rfc4122_ticks(0, 99), UUID_TICKS_BETWEEN_EPOCHS); assert_eq!( unix_to_rfc4122_ticks(0xFE00_0000_0000_0000, 0), UUID_TICKS_BETWEEN_EPOCHS ); } ```

KodrAus commented

2023-05-30 07:33:24 -05:00

(Migrated from github.com)

I’m also more inclined to a non-panicking variant here. For anybody that’s concerned about quietly changing behaviour through wrapping or truncation we can offer a checked_from_rfc4122 that returns an Option.

I do like that the wrapping variant gives you a more uniquely useful timestamp. I’d be happy with an implementation that did that. It might also be worth checking the current draft spec that introduces v6-v8 and see if it recommends any behaviour converting RRC4122 timestamps to Unix time.

I’m also more inclined to a non-panicking variant here. For anybody that’s concerned about quietly changing behaviour through wrapping or truncation we can offer a `checked_from_rfc4122` that returns an `Option`. I do like that the wrapping variant gives you a more uniquely useful timestamp. I’d be happy with an implementation that did that. It might also be worth checking the current draft spec that introduces v6-v8 and see if it recommends any behaviour converting RRC4122 timestamps to Unix time.

KodrAus commented

2023-06-26 20:24:01 -05:00

(Migrated from github.com)

Thanks @zheland 👍 I've opened #691 that addresses this by explicitly wrapping, basically as you've suggested here. All we're guaranteeing is that those methods won't panic, not that they'll wrap to any particular value, so I've reflected that in the test by only ensuring overflowing inputs don't panic.

Thanks @zheland :+1: I've opened #691 that addresses this by explicitly wrapping, basically as you've suggested here. All we're guaranteeing is that those methods won't panic, not that they'll wrap to any particular value, so I've reflected that in the test by only ensuring overflowing inputs don't panic.

Rows
Columns

Timestamp::from_rfc4122 panics when using timestamp before unix epoch #680

`Timestamp::from_rfc4122` panics when using timestamp before unix epoch #680