#11 Hang on zombie child process

[Grzegorz Sikorski]

The original code is doing:
1. fork child process (ssh)
2. sigprocmask to catch SIGCHLD
3. wait on signal (or terminal data) on pselect

In case ssh exits before 2. the SIGCHLD is lost and process hangs on
pselect. This was proven on large ansible playbook running on dozens of
remote hosts with ssh control persist feature enabled. In such
configuration password propmt is almost never presented and if the
operation on remote is really simple and host is under heavy load (ie.
when executing concurrent ansible tasks in dozens forks), the issue is
more likely to manifest. In other cases the race condition is too tiny.
In case ssh password prompt is sent to the client by remote, the issue
does not manifest, as the child will always wait until terminal action
occurs.

I attached a patch that fixes this issue. It was confirmed on a large ansible playbook that hangs with the original code.

[Shachar Shemesh]

Hi Grzegorz,

Thank you for the report and the patch. Upon reviewing the code, I can confirm the problem as reported. I do not, however, agree with the approach you took to fix it. I prefer that the loop not be restructured.

I am attaching my own proposal to fixing this issue, by moving the sigprocmask to before the fork. Please see if it if you have any comments before I commit it.

Thanks again,
Shachar

[Grzegorz Sikorski]

I did not want to do this, as it is affecting forked process sigmask, which may have other unexpected consequences. Are you sure it is safe to change default sigmask for ssh?

[Grzegorz Sikorski]

OK, sorry, I see you are reverting the mask to original in child. This looks fine to me.

[Shachar Shemesh]

Can you test it in your environment to make sure it indeed solves the problem?

[Shachar Shemesh]

Read my patch again. It resets the sigprocmask for the forked process before actually starting it (line 303 of the file after applying the patch).

Without this, it would, indeed, break stuff. As is, it should be perfectly fine.

[Grzegorz Sikorski]

As mentioned, it is ok. Let me test this.

[Grzegorz Sikorski]

Hi Shachar,

I have tested your fix and it is working. However after deeper review of your patch I think it affects child process somehow, as you are defaulting its mask to block all. This may not be an intention of the sshpass caller, so I propose attached solution instead. Attached patch was also successfully tested.

[Shachar Shemesh]

Au contraire. It sets the mask for the new process to all signals unblocked. It is using signmask_select, which is initialized with sigemptyset.

Since I'm starting a brand new session here, I prefer to set the mask to a known state, rather than inherit whatever crap I-don't-know-who sent my way when running sshpass.

If you say you've tested it and it works, I'm committing my version.

[Grzegorz Sikorski]

Correct, I misread the code. This does not really make much difference as subprocess should take care of signal mask itself and not rely upon what is send to it by parent. The only possible issue I can see here is you are changing the behaviour of the utility, which may affect some users.

[Przemyslaw Strzelczak]

Hi Shachar,
could you release a version with this patch? It is likely that I hit issue described here in my work.

[Przemyslaw Strzelczak]

Noticed it is already released in 1.07.