Valgrind failures in dtptngen.cpp: getDefaultHourCycle can call UPRV_UNREACHABLE on an empty instance

Description

Running the icu4c tests under valgrind using the master branch hits a number of failures.

I filed two tickets for these failures:

  • One for making fixes to get the tests to run clean. (See ).

  • This ticket for issues found in dtptngen.cpp specifically though.

Below is the error output from valgrind:

It appears this is a possible regression introduced by:

This is the problematic line that valgrind is complaining about:

The variable fDefaultHourFormatChar is only set by the initData method, or the getAllowedHourFormats method, as it depends on a given input “locale”.

The test case that hits this uses the DateTimePatternGenerator class like this (with no locale at all):

Since there is no locale ever set, initData isn't run, and/or getAllowedHourFormats isn’t called, so fDefaultHourFormatChar contains random/uninitialized/garbage data.

 

I think setting fDefaultHourFormatChar to 0 in the constructor might work for this usage, as it appears the intent is to only change the hour-cycle to what is preferred by the locale. (If there is no locale, then it presumably doesn't need to adjusted.)

 

However there is another issue as well (which is what this ticket is for):

Right now if you call the [new in 67] API DateTimePatternGenerato::getDefaultHourCycle on an empty instance you will either get back a random answer (since fDefaultHourFormatChar contains random memory), or will more likely end up calling abort (via UPRV_UNREACHABLE).

The method getDefaultHourCycle does (fortunately) have an error code, even though it is not used.

However, the documentation for getDefaultHourCycle doesn’t specify if it is supposed to “work” on an empty instance or not, so it isn’t clear to me what kind of error should be returned (perhaps U_UNSUPPORTED_ERROR?), and it also isn’t clear what UDateFormatHourCycle should be returned (since there isn’t a “default” or “unknown” value for the enum).

That said, we should probably document that a locale must be set first before calling it.

So there’s likely two PRs needed for this:

  • One to fix the uninitialized variable fDefaultHourFormatChar.

    • I’ll do this as part of

  • One to fix the API DateTimePatternGenerator::getDefaultHourCycle.

    • This might need some more discussion.

Activity

Show:
Jeff Genovy
March 10, 2020, 12:35 AM
Jeff Genovy
March 4, 2020, 7:35 PM

From discussion in ICU-TC on 2020-03-04, I’ll take this ticket with Frank and Peter as reviewers.

Will return an error instead of aborting.

Fixed
Your pinned fields
Click on the next to a field label to start pinning.

Assignee

Jeff Genovy

Reporter

Jeff Genovy

Components

Reviewer

Frank Yung-Fong Tang

Priority

major

Fix versions