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

RESOLVED FIXED in Firefox 56

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: dmajor, Assigned: anba)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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)
Posted 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
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: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.