Closed
Bug 1382840
Opened 7 years ago
Closed 7 years ago
nsRFPService::UpdatePref() needs to copy the string it passes to PR_SetEnv
Categories
(Core :: General, defect)
Core
General
Tracking
()
RESOLVED
FIXED
mozilla56
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox54 | --- | unaffected |
firefox55 | --- | fixed |
firefox56 | --- | fixed |
People
(Reporter: mstange, Assigned: timhuang)
References
Details
(Keywords: regression)
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
ehsan.akhgari
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
There's this code at http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/components/resistfingerprinting/nsRFPService.cpp#151 : > if (!mInitialTZValue.IsEmpty()) { > nsAutoCString tzValue = NS_LITERAL_CSTRING("TZ=") + mInitialTZValue; > PR_SetEnv(tzValue.get()); > } else { The value string that's passed to PR_SetEnv goes out of scope immediately, so the pointer that PR_SetEnv now holds on to is no longer valid. We have other callers of PR_SetEnv with dynamic strings in the tree, and those use ToNewCString in order to copy and leak the string, e.g. here: http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/crashreporter/nsExceptionHandler.cpp#2504 And then there are a few valgrind annotations for these intentional leaks at http://searchfox.org/mozilla-central/source/build/valgrind/cross-architecture.sup#8 .
Flags: needinfo?(tihuang)
Assignee | ||
Comment 1•7 years ago
|
||
Thanks for looking into this. I will fix this.
Assignee: nobody → tihuang
Flags: needinfo?(tihuang)
Comment hidden (mozreview-request) |
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8888653 [details] Bug 1382840 - Making the nsRFPService::UpdatePref() to copy the string which been passed to PR_SetEnv(). https://reviewboard.mozilla.org/r/159686/#review165252 ::: toolkit/components/resistfingerprinting/nsRFPService.cpp:157 (Diff revision 1) > + > + // If the tz has been set before, we free it first since it will be allocated > + // a new value later. > + if (tz) { > + free(tz); > + tz = nullptr; No point in nulling this out here.
Attachment #8888653 -
Flags: review?(ehsan) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/80b3b7fa1e23 Making the nsRFPService::UpdatePref() to copy the string which been passed to PR_SetEnv(). r=Ehsan
Keywords: checkin-needed
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/80b3b7fa1e23
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Comment 7•7 years ago
|
||
Please request Beta approval on this.
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(tihuang)
Assignee | ||
Comment 8•7 years ago
|
||
Comment on attachment 8888653 [details] Bug 1382840 - Making the nsRFPService::UpdatePref() to copy the string which been passed to PR_SetEnv(). Approval Request Comment [Feature/Bug causing the regression]: 1330890 [User impact if declined]: Users might get wrong timezone when they flip off the pref 'privacy.resistFingerprinting' if they have set the TZ environment value by themselves. [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: No [Is the change risky?]: Low risky, because of this pref is set off by default, so the normal users will not encounter this issue. [Why is the change risky/not risky?]: [String changes made/needed]:
Flags: needinfo?(tihuang)
Attachment #8888653 -
Flags: approval-mozilla-beta?
Comment 9•7 years ago
|
||
Comment on attachment 8888653 [details] Bug 1382840 - Making the nsRFPService::UpdatePref() to copy the string which been passed to PR_SetEnv(). makes sense, seems worth getting in 55.0b13
Attachment #8888653 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 10•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/459b23d30228
Comment 11•7 years ago
|
||
(In reply to Tim Huang[:timhuang] from comment #8) > [Is this code covered by automated tests?]: No > [Has the fix been verified in Nightly?]: Yes > [Needs manual test from QE? If yes, steps to reproduce]: No Setting qe-verify- based on Tim's assessment on manual testing needs.
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•