Closed Bug 1373763 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript: Internationalization API, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: dmajor, Assigned: anba)

References

Details

Attachments

(1 file, 1 obsolete file)

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)
Blocks: 1323254
Blocks: 1299615
No longer blocks: 1323254
(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)
Flags: needinfo?(andrebargull)
Attached patch bug1373763.patch (obsolete) — Splinter Review
This change should fix the ASan issue, right?
Assignee: nobody → andrebargull
Status: NEW → ASSIGNED
Flags: needinfo?(andrebargull)
Attachment #8882219 - Flags: review?(dmajor)
Comment on attachment 8882219 [details] [diff] [review]
bug1373763.patch

Thanks!
Attachment #8882219 - Flags: review?(dmajor) → review+
Clearing checkin-needed to amend the patch to match upstream.
Keywords: checkin-needed
Attached patch bug1373763.patchSplinter Review
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+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/3a8ffdd05415
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.