1159081 - Bad PR_SetEnv usage for Gonk

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Brian R. Bondy [:bbondy]]

I think the PR_SetEnv usage for this Gonk related code is wrong:

https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/nsUpdateDriver.cpp#872

> nsPrintfCString prioEnv("MOZ_UPDATER_PRIO=%d/%d/%d/%d",
>                           prioVal, oomScoreAdj, ioprioClass, ioprioLevel);
> PR_SetEnv(prioEnv.get());


From the PR_SetEnv documentation it says that the buffer passed can be used directly (the value is not copied in somewhere else) and so should not be stack allocated. I think that once prioVal goes out of scope the memory referenced will be freed.

Comment 1

[Robert Strong (they/them - no direct email)]

needinfo'ing Dave and Ehsan since this was added as part of bug 802423.

Comment 2

[Gabriele Svelto [:gsvelto]]

One could also use hal::SetProcessPriority() directly on the PID of the spawned task: http://hg.mozilla.org/mozilla-central/file/caf25344f73e/hal/Hal.h#l476

Bonus points if somebody adds optional I/O prioritization support to that function; we could use it in FxOS too.

Comment 3

[(no longer active)]

Yeah, PR_SetEnv is a very thin wrapper around putenv on Linux, which is the suckiest API on the planet.  It seems like the prevalent pattern for these cases is to use ToNewCString() to copy the string into the heap and leak the buffer.

Comment 4

[(no longer active)]

Attachment: Fix the PR_SetEnv usage for the MOZ_UPDATER_PRIO environment variable on gonk

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 8598603 [details] [diff] [review]
Fix the PR_SetEnv usage for the MOZ_UPDATER_PRIO environment variable on gonk

Looks fine to me. Let's make sure that Dave is also ok with this just in case this causes issues for gonk.

Comment 6

[Dave Hylands [:dhylands]]

It looks like technically this isn't an issue for gonk, but it is an issue for API correctness.

PR_SetEnv winds up calling putenv, which does a strdup of its argument:
https://android.googlesource.com/platform/bionic/+/ics-mr1-release/libc/stdlib/putenv.c

Comment 7

[(no longer active)]

This doesn't need a try run.

Comment 8

[Robert Strong (they/them - no direct email)]

Pushed to mozilla-inbound
https://hg.mozilla.org/integration/mozilla-inbound/rev/db13f4410450

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/db13f4410450