Don't use the setenv function after fork
ClosedPublic

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

Details

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.May 17 2020, 5:52 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptMay 17 2020, 5:52 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
jpalecek requested review of this revision.May 17 2020, 5:52 PM
jpalecek edited the summary of this revision. (Show Details)May 17 2020, 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.May 17 2020, 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.May 17 2020, 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.May 17 2020, 11:18 PM
apol added inline comments.
src/kcrash.cpp
691

Use {} instead of ; for readability.

jpalecek updated this revision to Diff 83137.May 24 2020, 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.May 24 2020, 1:31 AM
jpalecek added inline comments.
src/kcrash.cpp
691

Done that.

bruns added a subscriber: bruns.May 24 2020, 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.May 27 2020, 12:20 AM
jpalecek marked an inline comment as done.

Change remove_if and copy to copy_if, per suggestion

jpalecek added inline comments.May 27 2020, 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.May 27 2020, 9:21 PM
This revision is now accepted and ready to land.May 27 2020, 9:21 PM
dfaure closed this revision.Jun 7 2020, 9:24 AM

I see. It needs the declaration of environ, which is only provided on GNU. Should I amend it here?

I see. It needs the declaration of environ, which is only provided on GNU. Should I amend it here?

This has been committed already, create a new review request. (Note that KDE has moved to Gitlab at https://invent.kde.org, best if you create a merge request there; Phabricator still works, but it's planned to become read-only). :)

NOTICE OF INTENTION TO REVERT

Due to this commit introducing a FBTFS on FreeBSD, this commit will be automatically reverted in 24 hours unless https://invent.kde.org/frameworks/kcrash/-/merge_requests/3 has been merged and the FTBFS condition corrected.