Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed possible SIGCHILD signal lost #2146

Merged
merged 1 commit into from
Feb 10, 2022
Merged

Conversation

zbstao
Copy link
Contributor

@zbstao zbstao commented Feb 10, 2022

When multiple(eg. 20) xrdp connections are disconnected at the same time(eg. close all rdp client at the same time), zombie process will be spawned.
use xorgrdp session and set the parameter KillDisconnected to true.

I found similar code in file chansrv.c

void
child_signal_handler(int sig)
{
    int pid;

    LOG_DEVEL(LOG_LEVEL_INFO, "child_signal_handler:");
    do
    {
        pid = g_waitchild();
        LOG_DEVEL(LOG_LEVEL_INFO, "child_signal_handler: child pid %d", pid);
        if ((pid == g_exec_pid) && (pid > 0))
        {
            LOG_DEVEL(LOG_LEVEL_INFO, "child_signal_handler: found pid %d", pid);
            //shutdownx();
        }
    }
    while (pid >= 0);
}

Copy link
Member

@matt335672 matt335672 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @zbstao,

Thanks for your useful contribution. I've got a couple of comments relating to the coding style we use, but the functionality looks fine to me.

If you address the comments I'll enable the CI pipeline for the PR.

Thanks.

xrdp/xrdp.c Outdated
@@ -163,11 +163,8 @@ xrdp_shutdown(int sig)
static void
xrdp_child(int sig)
{
int safety;
while (g_waitchild() >= 0) ;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The coding style requires spaces for indents rather than tabs, and also needs braces for the empty loop (e.g.):-

while(...)
{
}

I'm happy with the removal of the safety variable. It was introduced in f763cb3 but doesn't seem to be necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry didn't say the coding style page, the code needs to be modified.

sesman/sig.c Outdated
{
session_kill(pid);
}
}while (pid >= 0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The while on the same line matches the coding style, but will be rejected by our use of astyle in the CI pipeline. Please can you place the while on a separate line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

Copy link
Member

@matt335672 matt335672 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nearly there, but you've used a tab instead of 4 spaces in both locations. The CI still won't like that.

Apologies if I wasn't clear.

@matt335672
Copy link
Member

Looks great.

Can you squash your commits? I'll merge it in then.

When multiple(eg. 20) xrdp connections are disconnected at the same time(eg.  close all rdp client at the same time), zombie process may be spawned.
@zbstao
Copy link
Contributor Author

zbstao commented Feb 10, 2022

hi @matt335672
Done. In addition I will read the coding style asap.

@matt335672
Copy link
Member

Thanks for your contribution @zbstao

matt335672 added a commit that referenced this pull request Feb 16, 2022
Fixed possible infinite loop (regression on #2146)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants