Chrome OS: Improve session_manager's SystemUtilsImpl::ProcessGroupIsGone |
|||||||
Issue description
Currently, the implementation is like
signal(SIGALRM, SIG_IGN);
...
alarm(timeout);
...
do {
waitpid(pid, ...);
} while (some_condition);
...
alarm(0);
but if the alarm fires while not in waitpid(), the signal is just ignored. As a result, waitpid() may block longer than the timeout, possibly indefinitely. To fix the issue, it'd be better to do either of the following:
1) Remove alarm(), call waitpid() with WNOHANG and a busy-loop.
-or-
2) Register a signal handler, call sigsetjmp before calling the first alarm(), call siglongjmp from the handler to handle timeout.
The former would be much simpler but may slightly delay the Chrome and/or container shutdown process. The latter doesn't have the issue but is more complicated. We should probably try 1) first since it's simpler.
,
Aug 10 2017
Have you considered using sigtimedwait? http://man7.org/linux/man-pages/man2/sigtimedwait.2.html You can register a handler for SIGCHLD and then call sigtimedwait with the timeout.
,
Aug 10 2017
I can't reassign this to Chris yet as he's not registered as a project member of Chromium, but he'll be working on it as he's also working on that function right now. Chris, please feel free to assign this back to me if you don't have time to work on.
,
Aug 11 2017
,
Aug 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/c47909ab8050eec72de21551aaf21bb725b8c0d2 commit c47909ab8050eec72de21551aaf21bb725b8c0d2 Author: Chris Morin <cmtm@google.com> Date: Tue Aug 22 05:03:58 2017 login: fix race in ProcessIsGone There was a race condition in the ProcessIsGone function where the alarm signal could be received before waitpid (however unlikely). Also, ProcessIsGone couldn't handle a timeout of 0. BUG= chromium:754076 TEST=cros_run_unit_tests --board=caroline --packages chromeos-login Change-Id: I6784e56141478490d3284f29cb68e74633a10ea2 Reviewed-on: https://chromium-review.googlesource.com/613667 Commit-Ready: Christopher Morin <cmtm@google.com> Tested-by: Christopher Morin <cmtm@google.com> Reviewed-by: Dan Erat <derat@chromium.org> Reviewed-by: Christopher Morin <cmtm@google.com> [modify] https://crrev.com/c47909ab8050eec72de21551aaf21bb725b8c0d2/login_manager/system_utils_impl.cc
,
Aug 22 2017
,
Aug 22 2017
,
Sep 5 2017
,
Jan 22 2018
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by yusukes@chromium.org
, Aug 10 2017Status: Assigned (was: Unconfirmed)