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

ICU-22609 Fix crash when udat_open() is passed a bogus locale name. #2747

Closed

Conversation

dbaron
Copy link

@dbaron dbaron commented Dec 15, 2023

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22609
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

This fixes a crash reported in Chromium bug 1491726. As described in that bug, the underlying problem is that when udat_open() is passed a bad locale name, the code ends up constructing a Locale object and performing operations on that object without first calling its isBogus method, which leads to a crash. This change simplifies the code in a way that I believe is equivalent for non-bogus locale values, and fixes the crash.


I was unable to figure out from the contributing documentation how to get an account to file a JIRA issue for this problem. It links to this contact page which links to a few mailing lists that all seem inappropriate and to this page on the bug tracking system that mentions the need for an account but doesn't say how to create one. If I actually go to JIRA it says "Something's gone wrong", "Looks like you've been signed out. Try logging in again.", "Log in", but clicking "Log in" just returns to the same page with the same error message, which doesn't appear to provide a mechanism for account creation.

@CLAassistant
Copy link

CLAassistant commented Dec 15, 2023

CLA assistant check
All committers have signed the CLA.

@dbaron
Copy link
Author

dbaron commented Dec 15, 2023

Well, the contributing documentation says "If your employer has already signed a CLA, then when you open your first Pull Request, an automated comment will appear that contains a link you can follow to declare your affiliation with this employer." but I don't see that link in the above automated comment.

@FrankYFTang FrankYFTang changed the title Fix crash when udat_open() is passed a bogus locale name. ICU-22609 Fix crash when udat_open() is passed a bogus locale name. Dec 15, 2023
@FrankYFTang
Copy link
Contributor

@markusicu @sffc I sent a private message about dbaron's CLA status

@dbaron dbaron force-pushed the fix-udat-open-localename-crash branch from 3d577bd to f693279 Compare December 15, 2023 19:39
@jira-pull-request-webhook
Copy link

Notice: the branch changed across the force-push!

  • icu4c/source/test/cintltst/cdattst.c is different

View Diff Across Force-Push

~ Your Friendly Jira-GitHub PR Checker Bot

@FrankYFTang
Copy link
Contributor

I do not think the proposed fix is "good enough". For example, the following case will still crash I believe:

DateFormat::createDateTimeInstance(DateFormat::kNone, DateFormat::kMedium, Locale("notalanguage")

@dbaron
Copy link
Author

dbaron commented Dec 15, 2023

It's not clear to me what the expectation is for users of Locale to check isBogus: should the caller that constructs the Locale check it, the API that takes a Locale as a parameter, or both? (I guess I see some of both in the existing code.)

@FrankYFTang
Copy link
Contributor

I put down a counter proposal fix in #2750
we also need to make sure the C++ API will not crash

@FrankYFTang FrankYFTang self-requested a review December 15, 2023 22:49
@srl295
Copy link
Member

srl295 commented Dec 15, 2023

Well, the contributing documentation says "If your employer has already signed a CLA, then when you open your first Pull Request, an automated comment will appear that contains a link you can follow to declare your affiliation with this employer." but I don't see that link in the above automated comment.

@dbaron it looks like there are some misleading aspects about how the CLA is displayed. As a result of this we're going to file a ticket upstream with the CLA assistant, and look at if we should update our wording for clarity.

Meanwhile, please click the 'Login to sign…' button. it won't actually sign until you click the Agree button. After logging in, it will as you for your affiliation, choose My employer has already signed the CLA. if that is appropriate.

As to the JIRA login, I'm not sure what's happening there. What do you get when you try to login to https://id.atlassian.com ?

@FrankYFTang
Copy link
Contributor

obsoleted by #2750

@srl295
Copy link
Member

srl295 commented Dec 18, 2023

Well, the contributing documentation says "If your employer has already signed a CLA, then when you open your first Pull Request, an automated comment will appear that contains a link you can follow to declare your affiliation with this employer." but I don't see that link in the above automated comment.

@dbaron it looks like there are some misleading aspects about how the CLA is displayed. As a result of this we're going to file a ticket upstream with the CLA assistant, and look at if we should update our wording for clarity.

Meanwhile, please click the 'Login to sign…' button. it won't actually sign until you click the Agree button. After logging in, it will as you for your affiliation, choose My employer has already signed the CLA. if that is appropriate.

As to the JIRA login, I'm not sure what's happening there. What do you get when you try to login to https://id.atlassian.com ?

I've filed cla-assistant/cla-assistant#1034 to improve the wording in the CLA assistant, please comment on that PR if you have opinions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants