Bug 1349 - race condition with ControlMaster=auto
Summary: race condition with ControlMaster=auto
Status: CLOSED FIXED
Alias: None
Product: Portable OpenSSH
Classification: Unclassified
Component: ssh (show other bugs)
Version: 5.0p1
Hardware: All All
: P2 normal
Assignee: Assigned to nobody
URL:
Keywords:
Depends on: 1329
Blocks: V_5_1
  Show dependency treegraph
 
Reported: 2007-08-05 04:18 AEST by Tony Finch
Modified: 2008-07-22 12:19 AEST (History)
4 users (show)

See Also:


Attachments
Fix race in ControlMaster=auto (2.28 KB, patch)
2007-08-05 04:18 AEST, Tony Finch
no flags Details | Diff
updated patch (3.77 KB, patch)
2008-03-06 23:43 AEDT, David Woodhouse
no flags Details | Diff
fixed patch (2.47 KB, patch)
2008-03-07 00:59 AEDT, David Woodhouse
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Finch 2007-08-05 04:18:14 AEST
Created attachment 1335 [details]
Fix race in ControlMaster=auto

There is a race in the setup of the ControlMaster socket in auto mode, as illustrated by the following command line:

ssh -oControlMaster=auto -oControlPath=sock localhost 'sleep 1; echo 1' &
ssh -oControlMaster=auto -oControlPath=sock localhost 'sleep 2; echo 2' &

Both of the commands will try to start up as a control client, find that sock does not exist, and switch into control master mode. One will succeed in creating the control master socket and the other will fail and bomb.

The attached patch eliminates this race by trying to create a control master socket first, and falling back to control client mode if master mode fails.
Comment 1 David Woodhouse 2008-03-06 23:40:08 AEDT
You don't seem to use the new 'test' argument to ssh_control_listener()

You also new use it before it's declared, leading to the following error:

ssh.c: In function ‘ssh_session’:
ssh.c:997: warning: implicit declaration of function ‘ssh_control_listener’
ssh.c: At top level:
ssh.c:1072: error: static declaration of ‘ssh_control_listener’ follows non-static declaration
ssh.c:997: error: previous implicit declaration of ‘ssh_control_listener’ was here
Comment 2 David Woodhouse 2008-03-06 23:43:32 AEDT
Created attachment 1461 [details]
updated patch
Comment 3 David Woodhouse 2008-03-06 23:45:25 AEDT
I verified that in combination with my patch for bug #1329, this still copes correctly with stale sockets.
Comment 4 Tony Finch 2008-03-07 00:42:19 AEDT
Thanks for looking at this patch.

My original patch doesn't call ssh_control_listener() from ssh_session() so I think you introduced that error. I believe the control socket doesn't work with protocol version 1 because the latter doesn't support arbitrary multiplexed streams. If so it's wrong to add the call and wrong to re-arrange the code to make the call compile.

Thanks for spotting that the test argument is redundant.
Comment 5 David Woodhouse 2008-03-07 00:59:36 AEDT
Created attachment 1462 [details]
fixed patch

(In reply to comment #4)
> My original patch doesn't call ssh_control_listener() from
> ssh_session() so I think you introduced that error.

So I did; sorry. I was applying this patch after the patch in bug #1330 to provide 'ControlPersist' support, and had to fix up a reject by hand -- incompetently, it would seem.

This one puts it back where it should be. But also applies _after_ Alan Barrett's patch in bug #1330, which is hopefully also ready to be applied now.
Comment 6 Damien Miller 2008-06-12 16:57:13 AEST
Fallback and recovery from mux socket errors is implemented in the latest patch to bug #1329.
Comment 7 Damien Miller 2008-06-13 10:19:59 AEST
the patch in attachment #1309 [details] has been applied applied; will be in openssh-5.1
Comment 8 Damien Miller 2008-07-22 12:19:26 AEST
Mass update RESOLVED->CLOSED after release of openssh-5.1