We're updating the issue view to help you get more done. 

ICU4C Out-of-Memory errors not handled in DecimalFormat

Description

When using the ICU C API for number formatting, not all Out-of-Memory (OOM) errors are correctly reported. (This was found from various internal crash dumps).

It is possible to have a half-constructed object returned from the unum_open API with *no* error code set, if OOM happens during the call.

The issue can be illustrated by the sample code in the ICU4C 'numfumt' project.

Here is some (slightly edited) sample code from the ICU4C 'numfumt' project that calls unum_open.
File: `icu4c\source\samples\numfmt\capi.c`

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 void capi() { UNumberFormat *fmt = nullptr; UErrorCode status = U_ZERO_ERROR; /* The string "987654321.123" as UChars */ UChar str[] = { 0x39, 0x38, 0x37, 0x36, 0x35, 0x34, 0x33, 0x32, 0x31, 0x30, 0x2E, 0x31, 0x32, 0x33, 0 }; double a = 0.0; /* Create a formatter for the en_US locale */ fmt = unum_open(UNUM_DECIMAL, nullptr, 0, "en_US", 0, &status); if (U_FAILURE(status)) { printf("FAIL: unum_open\n"); exit(1); } // jefgen: It is possible that Out-of-Memory can occur in the above call to the // unum_open API, and a half-valid object will be returned with no error set. // // This means that in the following API call, a crash/exception/access violation will occur. a = unum_parseDouble(fmt, str, u_strlen(str), nullptr, &status);

The call stack for the `unum_open` call looks something like this:

1 2 3 4 5 icuin62.dll!icu_62::DecimalFormat::DecimalFormat(const icu_62::DecimalFormat & source) icuin62.dll!icu_62::DecimalFormat::clone() icuin62.dll!icu_62::NumberFormat::createInstance(const icu_62::Locale &) icuin62.dll!icu_62::NumberFormat::createInstance(const icu_62::Locale & inLocale, UErrorCode & status) numfmt.exe!cppapi()

The underlying issue is that the `DecimalFormat::clone` method, and the `DecimalFormat` constructor which it later calls, have no way to report if an OOM error occurs when creating the new object.

(Alternatively, there is no way to set an internal `status` variable in the DecimalFormat object to denote that the object itself is in an "invalid" state.)

For reference here is the code for the clone and DecimalFormat methods:

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 Format* DecimalFormat::clone() const { return new DecimalFormat(*this); } ... DecimalFormat::DecimalFormat(const DecimalFormat& source) : NumberFormat(source) { fields = new DecimalFormatFields(); if (fields == nullptr) { return; } fields->properties.adoptInstead(new DecimalFormatProperties(*source.fields->properties)); fields->symbols.adoptInstead(new DecimalFormatSymbols(*source.fields->symbols)); fields->exportedProperties.adoptInstead(new DecimalFormatProperties()); if (fields->properties == nullptr || fields->symbols == nullptr || fields->exportedProperties == nullptr) { return; } touchNoError(); }

For example, if the line "` fields = new DecimalFormatFields(); `" fails to allocate memory, then `fields` member variable will be set to `nullptr`/`NULL`, and the `clone()` member function will return back a object that is in an invalid state.

This means that the later call to unum_parseDouble will eventually hit the following member function:

1 2 3 4 5 6 const numparse::impl::NumberParserImpl* DecimalFormat::getParser(UErrorCode& status) const { if (U_FAILURE(status)) { return nullptr; } // First try to get the pre-computed parser auto* ptr = fields->atomicParser.load(); // jefgen: an AV will occur on the above line as 'fields' will be nullptr.

Environment

Status

Assignee

Jeff Genovy

Reporter

Jeff Genovy

Labels

Reviewer

Shane Carr

tracCreated

Jun 21, 2018, 10:02 PM

tracProject

ICU4C

tracReporter

jefgen

tracStatus

new

Components

Fix versions

Priority

major