Closed Bug 1382840 Opened 2 years ago Closed 2 years ago

nsRFPService::UpdatePref() needs to copy the string it passes to PR_SetEnv

Categories

(Core :: General, defect)

defect
Not set

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)

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)
Thanks for looking into this. I will fix this.
Assignee: nobody → tihuang
Flags: needinfo?(tihuang)
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+
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
https://hg.mozilla.org/mozilla-central/rev/80b3b7fa1e23
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Please request Beta approval on this.
Flags: needinfo?(tihuang)
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 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+
(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.