ICU-23448 Fix Invalid-enum-value crash in udat_open by FrankYFTang · Pull Request #4056 · unicode-org/icu

FrankYFTang · 2026-06-30T16:53:01Z

Checklist

Required: Issue filed: ICU-23448
Required: The PR title must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
Required: Each commit message must be prefixed with a JIRA Issue number. Example: "ICU-NNNNN Fix xyz"
Issue accepted (done by Technical Committee after discussion)
Tests included, if applicable
API docs and/or User Guide docs changed or added, if applicable
Approver: Feel free to merge on my behalf

Validate timeStyle and dateStyle in udat_open to prevent passing invalid styles to C++ APIs, which causes Undefined Behavior (load of invalid enum value) flagged by UBSAN.

Also update date_format_fuzzer.cpp to avoid passing invalid enums to C++ APIs directly, and instead test udat_open validation with raw bytes.
Add a unit test in cdattst.c to verify.

Bug: https://issues.oss-fuzz.com/issues/477633331

See unicode-org#4056

jira-pull-request-webhook · 2026-06-30T18:07:57Z

Hooray! The files in the branch are the same across the force-push. 😃

~ Your Friendly Jira-GitHub PR Checker Bot

Validate timeStyle and dateStyle in udat_open to prevent passing invalid styles to C++ APIs, which causes Undefined Behavior (load of invalid enum value) flagged by UBSAN. Also update date_format_fuzzer.cpp to avoid passing invalid enums to C++ APIs directly, and instead test udat_open validation with raw bytes. Add a unit test in cdattst.c to verify. Bug: https://issues.oss-fuzz.com/issues/477633331

These uninitialized members were causing crashes in tests under UBSAN.

Prevent invalid styles from causing integer overflow and other issues. Added unit test TestInvalidStyles.

jira-pull-request-webhook · 2026-06-30T19:15:03Z

Notice: the branch changed across the force-push!

icu4c/source/i18n/datefmt.cpp is different
icu4c/source/i18n/udat.cpp is different
icu4c/source/test/fuzzer/date_format_fuzzer.cpp is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

richgillam

LOKTM, but a couple things didn't completely make sense to me...

richgillam · 2026-06-30T20:11:02Z

+   static_assert(sizeof(dateStyle) == sizeof(dateStyleInt), "sizes must match");
+   static_assert(sizeof(timeStyle) == sizeof(timeStyleInt), "sizes must match");
+   std::memcpy(&dateStyleInt, &dateStyle, sizeof(dateStyle));
+   std::memcpy(&timeStyleInt, &timeStyle, sizeof(timeStyle));


A couple dumb questions: Why do you need to convert it into an int and back here? Is this necessary to check if the input is a valid enum value?

And would it make sense to just treat invalid enum values are equivalent to kNone rather than just failing on construction?

Converting to int via memcpy is necessary to avoid UBSAN (Undefined Behavior Sanitizer) warnings. In C++, casting an arbitrary invalid integer to an enum and then using it (e.g. in comparison) is undefined behavior. memcpy allows us to inspect the raw bits safely without triggering UB.

Returning nullptr (failing on construction) is safer than treating invalid values as kNone. Treating invalid values as kNone might hide bugs in user code and lead to unexpected formatting behavior instead of a clear failure.

It's incredibly ugly, but if we have to do it that way, I guess we have to do it that way. I do think it'd help to have some comments in there explaining why we have to do it that way, though.

memcpy() is unnecessary. Assigning an enum to an int is safe and conventional.

Also, the static_asserts+memcpy could be a portability problem: There is no guarantee that an enum is always the same size as an int32_t. (More likely same as an int.)

Please just use

int32_t dateStyleInt = dateStyle; int32_t timeStyleInt = timeStyle;

Done. Replaced memcpy and static_assert with simple assignment. Verified that it compiles and passes tests under UBSAN/ASAN (even with -O3).

Reverted back to memcpy (with static_assert size checks) because simple assignment actually triggered UBSan errors in CI (e.g. runtime error: load of value 100000, which is not a valid value for type 'EStyle'). In C++, loading an invalid value as an enum is undefined behavior, and UBSan catches this during the assignment. memcpy avoids loading it as enum type.

markusicu · 2026-06-30T23:40:54Z


 #include "cstring.h"
 #include "windtfmt.h"
+#include <cstring>


We usually include

standard C headers

standard C++ headers (like this one)

unicode/utypes.h

other public ICU headers

internal ICU headers

Done. Removed the unnecessary <cstring> include and moved <typeinfo> to the top of the file.

markusicu · 2026-06-30T23:44:42Z

+   static_assert(sizeof(dateStyle) == sizeof(dateStyleInt), "sizes must match");
+   static_assert(sizeof(timeStyle) == sizeof(timeStyleInt), "sizes must match");
+   std::memcpy(&dateStyleInt, &dateStyle, sizeof(dateStyle));
+   std::memcpy(&timeStyleInt, &timeStyle, sizeof(timeStyle));


memcpy() is unnecessary. Assigning an enum to an int is safe and conventional.

Also, the static_asserts+memcpy could be a portability problem: There is no guarantee that an enum is always the same size as an int32_t. (More likely same as an int.)

Please just use

int32_t dateStyleInt = dateStyle; int32_t timeStyleInt = timeStyle;

markusicu · 2026-06-30T23:46:17Z

+static UBool isValidStyle(int32_t style) {
+    return (style >= DateFormat::kFull && style <= DateFormat::kShort) ||
+           (style >= DateFormat::kFullRelative && style <= DateFormat::kShortRelative);
+}
+
+static UBool isValidTimeStyle(int32_t style) {
+    return (style >= DateFormat::kFull && style <= DateFormat::kShort);
+}


Please put these into an anonymous namespace and remove static.

Done. Moved the helper functions into an anonymous namespace and removed static in both datefmt.cpp and udat.cpp.

markusicu · 2026-06-30T23:47:46Z

+       dateStyleInt = dateStyleInt + kDateOffset;
   }
-   return create(timeStyle, dateStyle, aLocale);
+   return create(static_cast<EStyle>(timeStyleInt), static_cast<EStyle>(dateStyleInt), aLocale);


the timeStyle is unchanged, so please just use it, rather than casting back from the int

Done. Used timeStyle directly in the create call.

…, include ordering, use unchanged param)

FrankYFTang · 2026-07-01T01:02:12Z

PTAL

richgillam

Your changes and explanations are good enough for me; I hope they'll also be good enough for Markus.

… to memcpy with size check)

FrankYFTang · 2026-07-01T21:07:38Z

"int32_t dateStyleInt = dateStyle;" trigger ubsan while the value in dateStyle is not a valid enum, change back to memcpy

PTAL

… UBSan

markusicu

thanks!

markusicu · 2026-07-01T22:20:23Z

"int32_t dateStyleInt = dateStyle;" trigger ubsan while the value in dateStyle is not a valid enum

Making this kind of code "undefined behavior" makes C++ nearly impossible to work with.
Thanks for agreeing to suppress UBSan here instead, so that we can write sane code.

FrankYFTang added a commit to FrankYFTang/icu that referenced this pull request Jun 30, 2026

ICU-23448 Fix Invalid-enum-value crash in udat_open

4acb852

See unicode-org#4056

FrankYFTang force-pushed the fix-udat-open-invalid-enum branch from 44d3718 to 4acb852 Compare June 30, 2026 18:07

FrankYFTang requested review from richgillam and sffc and removed request for sffc June 30, 2026 18:08

FrankYFTang added 3 commits June 30, 2026 19:05

ICU-23448 Fix UBSAN errors due to uninitialized members in numparse

59ca4a3

These uninitialized members were causing crashes in tests under UBSAN.

ICU-23448 Validate styles in DateFormat::createDateTimeInstance

d2f04f4

Prevent invalid styles from causing integer overflow and other issues. Added unit test TestInvalidStyles.

FrankYFTang force-pushed the fix-udat-open-invalid-enum branch from 4acb852 to d2f04f4 Compare June 30, 2026 19:14

richgillam reviewed Jun 30, 2026

View reviewed changes

FrankYFTang requested a review from richgillam June 30, 2026 22:01

richgillam previously approved these changes Jun 30, 2026

View reviewed changes

markusicu requested changes Jun 30, 2026

View reviewed changes

markusicu self-assigned this Jun 30, 2026

ICU-23448 Add comments explaining memcpy usage to avoid UB

51e81e1

FrankYFTang dismissed richgillam’s stale review via 51e81e1 July 1, 2026 00:02

FrankYFTang added 2 commits July 1, 2026 00:26

ICU-23448 Use assignment instead of memcpy for style validation

ae2aef0

ICU-23448 Address review comments from markusicu (anonymous namespace…

fb5e6b1

…, include ordering, use unchanged param)

FrankYFTang requested review from markusicu and richgillam July 1, 2026 01:02

richgillam previously approved these changes Jul 1, 2026

View reviewed changes

ICU-23448 Fix UBSan errors due to loading invalid enum values (revert…

5584a29

… to memcpy with size check)

FrankYFTang dismissed richgillam’s stale review via 5584a29 July 1, 2026 20:33

ICU-23448 Add comments explaining memcpy usage for UBSan

2d3986a

ICU-23448 Revert memcpy and use UPRV_NO_SANITIZE_UNDEFINED to silence…

93b71e3

… UBSan

markusicu approved these changes Jul 1, 2026

View reviewed changes

Uh oh!

Uh oh!

Conversation

FrankYFTang commented Jun 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

jira-pull-request-webhook Bot commented Jun 30, 2026

Uh oh!

jira-pull-request-webhook Bot commented Jun 30, 2026

Uh oh!

richgillam left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FrankYFTang commented Jul 1, 2026

Uh oh!

richgillam left a comment

Choose a reason for hiding this comment

Uh oh!

FrankYFTang commented Jul 1, 2026

Uh oh!

markusicu left a comment

Choose a reason for hiding this comment

Uh oh!

markusicu commented Jul 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

FrankYFTang commented Jun 30, 2026 •

edited

Loading