If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

uprv_convertToPosix has a pointer to a stack local from a destroyed scope

RESOLVED FIXED in Firefox 56

Status

()

Core
JavaScript: Internationalization API
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: dmajor, Assigned: André Bargull)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

3 months ago
This causes Windows ASan builds to crash soon after startup.

https://dxr.mozilla.org/mozilla-central/rev/fe809f57bf2287bb937c3422ed03a63740b3448b/intl/icu/source/common/locmap.c#1053

Line 1053 sets pPosixID = locName. Then locName goes out of scope, but the function continues to use pPosixID.

With Waldo away, I'm not sure who to talk to about ICU stuff. André, perhaps you might be able to help get this fixed?
Flags: needinfo?(andrebargull)
(Reporter)

Updated

3 months ago
Blocks: 1323254
(Reporter)

Updated

3 months ago
Blocks: 1299615
No longer blocks: 1323254
(Assignee)

Comment 1

3 months ago
(In reply to David Major [:dmajor] from comment #0)
> With Waldo away, I'm not sure who to talk to about ICU stuff. André, perhaps
> you might be able to help get this fixed?

I can try to fix this issue after bug 1353650 (to avoid creating two different ICU patches). 

The general procedure when we need to patch our local ICU copy is as follows:
- We create a bug report upstream (http://bugs.icu-project.org/trac/newticket), so the ICU team gets informed about a potential problem.
- We create the patch and add it to the intl/icu-patches folder (http://searchfox.org/mozilla-central/source/intl/icu-patches).
- Then we apply the patch to our ICU copy.
- And if necessary, we recompile the precompiled ICU data file (but this step is only needed when ICU's data processing was modified, so it's not relevant to this issue).
Flags: needinfo?(andrebargull)
(Assignee)

Updated

3 months ago
Flags: needinfo?(andrebargull)
(Assignee)

Comment 2

3 months ago
Created attachment 8882219 [details] [diff] [review]
bug1373763.patch

This change should fix the ASan issue, right?
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Flags: needinfo?(andrebargull)
Attachment #8882219 - Flags: review?(dmajor)
(Reporter)

Comment 3

3 months ago
Comment on attachment 8882219 [details] [diff] [review]
bug1373763.patch

Thanks!
Attachment #8882219 - Flags: review?(dmajor) → review+
(Assignee)

Comment 4

3 months ago
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b544c86b01da61b95c3903207d016306efff2afb
Keywords: checkin-needed
(Assignee)

Comment 5

3 months ago
Clearing checkin-needed to amend the patch to match upstream.
Keywords: checkin-needed
(Assignee)

Updated

3 months ago
(Assignee)

Comment 6

3 months ago
Created attachment 8884532 [details] [diff] [review]
bug1373763.patch

Updated patch to match upstream, carrying r+ from dmajor.

Only difference compared to the previous patch: The local variable is now explicitly initialized.
Attachment #8882219 - Attachment is obsolete: true
Attachment #8884532 - Flags: review+
(Assignee)

Updated

3 months ago
Keywords: checkin-needed

Comment 7

2 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8ffdd05415
Extend scope for local variable in uprv_convertToPosix. r=dmajor
Keywords: checkin-needed

Comment 8

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3a8ffdd05415
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.