ICU-23291 prevent icu4j stack overflow when a resource file is missing during ULocale initialization#3796
ICU-23291 prevent icu4j stack overflow when a resource file is missing during ULocale initialization#3796eranl wants to merge 3 commits intounicode-org:mainfrom
Conversation
|
|
sffc
left a comment
sffc
left a comment
There was a problem hiding this comment.
@markusicu I don't understand very well how the bundle instantiation and caching works here; would you like to take a look?
| && !localeIDStartsWithLangSubtag(defaultID, localeName)) { | ||
| && !localeIDStartsWithLangSubtag(defaultID, localeName) | ||
| && (!Objects.equals(localeID, defaultID) || origLocaleID != null)) { |
There was a problem hiding this comment.
Please match the indentation of the file you are editing
| && !localeIDStartsWithLangSubtag(defaultID, localeName)) { | ||
| && !localeIDStartsWithLangSubtag(defaultID, localeName) | ||
| && (!Objects.equals(localeID, defaultID) | ||
| || origLocaleID != null)) { |
There was a problem hiding this comment.
Not completely sure I understand what you're doing here. Can you talk a little about the actual problem (and what conditions cause it to happen) and how your change solves the problem?
If I'm reading this right, the issue is that if the locale ID the caller is asking for doesn't have any resource files, we can get into an endless loop when we're in a lookup mode that includes the default locale. Is the problem only when the system default locale is the same as the locale he caller asked for, or where it also doesn't have any resource files? Or is the problem broader than that?
There was a problem hiding this comment.
As I described in ICU-23291, when a resource file is missing, the existing code results in infinite recursion (see stack trace there). The reason seems to be a missing stopping condition at the base case. That's what I tried to add here. The conditions I added make sure that we stop recursing if the next call's arguments would be identical to the current one's.
There was a problem hiding this comment.
Sorry for the long delay in responding; I was on vacation. I just realized that this PR doesn't include any test code, and that might be the best way to clarify the intent of the PR and the actual problem you're fixing. Can you add a unit test? If that's impractical, can you add a comment with some sample code and a narrative description of how to reproduce the problem? I'm particularly interested in things like which resources you're asking for, which locale you're asking for, whether the resource-open type is OpenType.LOCALE_DEFAULT_ROOT or something else, and what the system default locale is.
There was a problem hiding this comment.
I dove a little deeper on this, and updated ICU-23291 accordingly.
Should the fix be more specific to the ULocale initialization case?
Is there a way to emulate a missing resource file needed for ULocale initialization, such as timezoneTypes.res, in a unit test?
There was a problem hiding this comment.
@richgillam, can you please take another look?
There was a problem hiding this comment.
I'm sorry; I still don't get what we're talking about here.
The ticket says "When icu4j fails to load a resource file during ULocale initialization..." What does that mean? What is your code actually doing? You're saying you get this infinite recursion when you instantiate a ULocale? Or do you mean something else?
The ticket says "Here’s a sample stack trace when timezoneTypes.res is missing..." Why is timezoneTypes.res missing? That's one of the files that's bundled with ICU; it shouldn't be missing. You can't pick and choose this stuff. In fact, it should be inside a .jar file that we ship with ICU4J.
So again I ask, what are you actually doing? I asked for test code and/or full reproduction steps earlier. That might help clarify. Right now I still have no information on what problem you're trying to solve and why this PR is the solution.
There was a problem hiding this comment.
The context is, I was trying to build a slim version of the library. Without tracing, it was a matter of trial and error. That's how I found this issue.
I'm not suggesting the library should function without essential resources, just that it fails gracefully, as opposed to a stack overflow.
Now that tracing is available, this is less of an issue, but it still seems valid to me.
eranl
changed the title
ULocale initialization
eranl
changed the title
ULocale initialization
|
@markusicu , WDYT? |
4 participants
Fixes ICU-23291. Issue found while testing #3795.