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

Panic while formatting date in beta and nightly compilers #5039

Closed
Nerixyz opened this issue Jun 11, 2024 · 6 comments · Fixed by #5049
Closed

Panic while formatting date in beta and nightly compilers #5039

Nerixyz opened this issue Jun 11, 2024 · 6 comments · Fixed by #5049
Assignees
Labels
S-medium Size: Less than a week (larger bug fix or enhancement) T-bug Type: Bad behavior, security, privacy

Comments

@Nerixyz
Copy link

Nerixyz commented Jun 11, 2024

The following code panics while handling a panic in beta and nightly versions of Rust:

main.rs:

use std::str::FromStr;

use icu_calendar::Date;
use icu_datetime::{options::length, DateFormatter};
use icu_locid::Locale;

fn main() {
    let locale = Locale::from_str("en-u-ca-japanese").unwrap();
    let formatter = DateFormatter::try_new_with_length(&locale.into(), length::Date::Full).unwrap();
    let date = Date::try_new_iso_date(2020, 5, 30).unwrap().to_any();
    dbg!(formatter.format_to_string(&date).unwrap());
}

cargo.toml

[package]
name = "bar"
version = "0.1.0"
edition = "2021"

[dependencies]
icu_datetime = { version = "1.5.1", features = ["std"] }
icu_calendar = { version = "1.5.1", features = ["std"] }
icu_locid = { version = "1.5.0", features = ["std"] }

output:

panicked at library\core\src\fmt\mod.rs:2431:38:
thread panicked while processing panic. aborting.
error: process didn't exit successfully: `target\debug\bar.exe` (exit code: 0xc0000409, STATUS_STACK_BUFFER_OVERRUN)

(library\core\src\fmt\mod.rs:2431:38)

It's not really easy to debug, but it seems like the original panic is because of this debug_assert failing and the next panic seems to happen while trying to format icu_calendar::Era - so the original error was probably DateTimeWriteError::MissingEraSymbol. In release mode, this causes invalid UTF-8 to be written, because the era is wrong (from debugging I noticed it seems to be garbage - at least until the next nul).


test_japanese panics on nightly as well (when changing the toolchain in rust-toolchain.toml to nightly).


Update: It seems like rustc decided to swap the order of the tuple elements inside dates_to_eras, so it thinks the string comes first, but that one doesn't contain valid UTF-8.

@sffc sffc added this to the ICU4X 2.0 ⟨P1⟩ milestone Jun 13, 2024
@sffc sffc added T-bug Type: Bad behavior, security, privacy S-medium Size: Less than a week (larger bug fix or enhancement) labels Jun 13, 2024
@Manishearth
Copy link
Member

I'm surprised this wasn't caught by our CI

The test I've been using to check this is

use icu::calendar::{AnyCalendar, DateTime, Time};
use icu::datetime::{options::length, DateTimeFormatter};
use icu::locale::locale;
use writeable::Writeable;

fn main() {
    let locale = locale!("en-u-ca-japanese").into(); // English with the Japanese calendar

    let calendar = AnyCalendar::new_for_locale(&locale);

    let iso = DateTime::try_new_iso_datetime(2020, 10, 5, 13, 33, 15).unwrap();
    let datetime = iso.to_calendar(calendar);
    let options = length::Bag::from_date_time_style(length::Date::Full, length::Time::Short);

    let dtf = DateTimeFormatter::try_new(&locale, options.into())
        .expect("Failed to create DateTimeFormatter instance.");
    let manual_value = dtf
        .format(&datetime)
        .expect("Calendars should match");
    let s = manual_value.write_to_string();
    println!("{:?}", s);

    assert_eq!(s, "Monday, October 5, 2 Reiwa, 1:33 PM")
}

@Manishearth
Copy link
Member

test_japanese panics on nightly as well (when changing the toolchain in rust-toolchain.toml to nightly).

@robertbastian would you be able to verify why the nightly CI didn't catch this?

@Manishearth
Copy link
Member

[cargo-make] INFO - Running Task: set-ci-toolchain
forced-toolchain environment not required 

in both scheduled CI jobs https://github.com/unicode-org/icu4x/actions/runs/9501131062/job/26185924275 , https://github.com/unicode-org/icu4x/actions/runs/9501092498/job/26185787376

I know we tweaked how the nightly toolchain got set a bunch of times, I suspect we broke it?

Manishearth added a commit to Manishearth/icu4x that referenced this issue Jun 19, 2024
Fixes unicode-org#5039



Caused by rust-lang/rust#125360. We were
assuming that `packed` meant `C, packed` already. This is an assumption
I've seen throughout the Rust ecosystem so there may be reasons to
revert.
Manishearth added a commit to Manishearth/icu4x that referenced this issue Jun 19, 2024
Fixes unicode-org#5039

Caused by rust-lang/rust#125360. We were
assuming that `packed` meant `C, packed` already. This is an assumption
I've seen throughout the Rust ecosystem so there may be reasons to
revert.
Manishearth added a commit to Manishearth/icu4x that referenced this issue Jun 20, 2024
Fixes unicode-org#5039



Caused by rust-lang/rust#125360. We were
assuming that `packed` meant `C, packed` already. This is an assumption
I've seen throughout the Rust ecosystem so there may be reasons to
revert.
Manishearth added a commit to Manishearth/icu4x that referenced this issue Jun 20, 2024
Fixes unicode-org#5039

Caused by rust-lang/rust#125360. We were
assuming that `packed` meant `C, packed` already. This is an assumption
I've seen throughout the Rust ecosystem so there may be reasons to
revert.
Manishearth added a commit to Manishearth/icu4x that referenced this issue Jun 20, 2024
Fixes unicode-org#5039

Caused by rust-lang/rust#125360. We were
assuming that `packed` meant `C, packed` already. This is an assumption
I've seen throughout the Rust ecosystem so there may be reasons to
revert.
@Manishearth
Copy link
Member

June 27

    • Filing rustsec issues triggers linting for clients. The nature of the issue in zerovec is of the same nature as other rustsec type issues.
  • @sffc - I agree, it's the right thing to do and shows that we are good stewards
    • OK

LGTM: @sffc, @robertbastian , @Manishearth

@robertbastian robertbastian reopened this Jul 2, 2024
@robertbastian
Copy link
Member

Tracking rustsec

@robertbastian
Copy link
Member

rustsec/advisory-db#1990

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-medium Size: Less than a week (larger bug fix or enhancement) T-bug Type: Bad behavior, security, privacy
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants