Don't use the setenv function after fork
AcceptedPublic

Authored by jpalecek on Sun, May 17, 5:52 PM.

Details

Reviewers
dfaure
Group Reviewers
Frameworks
Summary

Ideally, you should not use any non-async-signal-safe functions after fork(), because the situation after fork is very similar to the situation in the signal handler. In kcrash, we are in signal handler before fork, so we shouldn't use setenv even before fork. This patch however emulates setenv using static memory so should be safe.

Depends on D29809
Signed-off-by: Jiří Paleček <jpalecek@web.de>

Diff Detail

Repository
R285 KCrash
Branch
for-upstream
Lint
Lint OK
Unit
No Unit Test Coverage
jpalecek created this revision.Sun, May 17, 5:52 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptSun, May 17, 5:52 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
jpalecek requested review of this revision.Sun, May 17, 5:52 PM
jpalecek edited the summary of this revision. (Show Details)Sun, May 17, 5:58 PM
jpalecek added a reviewer: Frameworks.
jpalecek retitled this revision from t/dont-use-setenv-after-fork to Don't use setenv after fork.Sun, May 17, 7:36 PM
jpalecek edited the summary of this revision. (Show Details)
anthonyfieroni added inline comments.
src/kcrash.cpp
693

Just use vector, get the size by environ_end - environ

jpalecek added inline comments.Sun, May 17, 11:01 PM
src/kcrash.cpp
693

No, that would be dynamic allocation, which is not safe in signal handlers. The whole purpose was to avoid it.

apol added a subscriber: apol.Sun, May 17, 11:18 PM
apol added inline comments.
src/kcrash.cpp
691

Use {} instead of ; for readability.

jpalecek updated this revision to Diff 83137.Sun, May 24, 1:26 AM
jpalecek retitled this revision from Don't use setenv after fork to Don't use the setenv function after fork.
jpalecek edited the summary of this revision. (Show Details)

Change the code a bit for readability

jpalecek marked an inline comment as done.Sun, May 24, 1:31 AM
jpalecek added inline comments.
src/kcrash.cpp
691

Done that.

bruns added a subscriber: bruns.Sun, May 24, 1:55 AM
bruns added inline comments.
src/kcrash.cpp
689

move this to the lambda scope, you can also make the var name shorter then.

698

std::copy_if, instead of a separate remove_if pass.

703

the part after the && can be removed when you include the '=' in the static string

jpalecek updated this revision to Diff 83160.Wed, May 27, 12:20 AM
jpalecek marked an inline comment as done.

Change remove_if and copy to copy_if, per suggestion

jpalecek added inline comments.Wed, May 27, 12:25 AM
src/kcrash.cpp
698

Thanks for that suggestion, updated accordingly. I admit I'm still more accustomed to the old ways of remove_if.

dfaure accepted this revision.Wed, May 27, 9:21 PM
This revision is now accepted and ready to land.Wed, May 27, 9:21 PM