PS One drawback of synchronizing on either "this" or on Session.class is, that these objects are publicly available to clients.
So, a malicious client who also synchronizes on the Session object or Session class at a wrong time might find an incorrect ordering of locks which might lead to deadlock eventually (which would not be the fault of JSch since the code inside your synchronized blocks is guaranteed to complete without external interaction; it would be the client's fault, but it would influence JSch). As a rule of thumb, I've been told that it is usually better to synchronize on local private Objects, which the clients can not access. So, instead of synchronizing on Session.class, I think you should synchronize on any local Object of your choice. Cheers, -- Martin Oberhuber, Senior Member of Technical Staff, Wind River Target Management Project Lead, DSDP PMC Member http://www.eclipse.org/dsdp/tm > -----Original Message----- > From: Oberhuber, Martin > Sent: Wednesday, July 16, 2008 4:02 PM > To: 'Atsuhiko Yamanaka' > Cc: [email protected] > Subject: RE: [JSch-users] JSch-0.1.37: NPE in > Session.disconnect() when calledbefore authenticated > > Hello Atsuhiko, > > yes this patch looks like a good solution that should > fix the problem safely. > > But why do you synchronize on Session.class and not on > this ? It seems that this would unnecessarily influence > other Sessions that have absolutely nothing to do with > this one. > > Also, synchronizing on this seems to have the added value > that synchronized methods "setConfig" and "_write" can > not run concurrently. > > Cheers, > -- > Martin Oberhuber, Senior Member of Technical Staff, Wind River > Target Management Project Lead, DSDP PMC Member > http://www.eclipse.org/dsdp/tm > > > > > -----Original Message----- > > From: Atsuhiko Yamanaka [mailto:[EMAIL PROTECTED] > > Sent: Monday, July 14, 2008 9:13 AM > > To: Oberhuber, Martin > > Cc: [email protected] > > Subject: Re: [JSch-users] JSch-0.1.37: NPE in > > Session.disconnect() when calledbefore authenticated > > > > Hi, > > > > +-From: "Oberhuber, Martin" <[EMAIL PROTECTED]> -- > > |_Date: Fri, 11 Jul 2008 17:14:36 +0200 _______________________ > > | > > |In JSch-0.1.37, Session.connect(int) sets > > |isConnected=true in line 208, but the > > |connectThread is only created later after > > |Authentication is done. > > ... > > |I'm not sure how to best address this, but one > > |option might be that disconnect() does this: > > |if(connectThread==null) { > > | //Not yet authenticated: Signal cancellation > > | synchronized(this) { > > | isConnected = false; > > | } > > |} > > > > In that change, there is a possibility that > > 'connectThread' is assigned before entering synchronized clause. > > And then, 'this' (an instance of Session) has been already > > used to send data, so 'disconnect' may be blocked in some case. > > > > How about the following patch? > > > > --- jsch-0.1.39/src/com/jcraft/jsch/Session.java Thu Jun > > 12 07:06:49 2008 > > +++ jsch-0.1.40/src/com/jcraft/jsch/Session.java Mon Jul > > 14 05:05:36 2008 > > @@ -456,12 +456,21 @@ > > } > > > > isAuthed=true; > > - connectThread=new Thread(this); > > - connectThread.setName("Connect thread "+host+" session"); > > - if(daemon_thread){ > > - connectThread.setDaemon(daemon_thread); > > + > > + synchronized(Session.class){ > > + if(isConnected){ > > + connectThread=new Thread(this); > > + connectThread.setName("Connect thread "+host+" session"); > > + if(daemon_thread){ > > + connectThread.setDaemon(daemon_thread); > > + } > > + connectThread.start(); > > + } > > + else{ > > + // The session has been already down and > > + // we don't have to start new thread. > > + } > > } > > - connectThread.start(); > > } > > catch(Exception e) { > > in_kex=false; > > @@ -1487,10 +1496,12 @@ > > PortWatcher.delPort(this); > > ChannelForwardedTCPIP.delPort(this); > > > > - synchronized(connectThread){ > > - Thread.yield(); > > - connectThread.interrupt(); > > - connectThread=null; > > + synchronized(Session.class){ > > + if(connectThread!=null){ > > + Thread.yield(); > > + connectThread.interrupt(); > > + connectThread=null; > > + } > > } > > thread=null; > > try{ > > ------------------------------------------------------------------------- This SF.Net email is sponsored by the Moblin Your Move Developer's challenge Build the coolest Linux based applications with Moblin SDK & win great prizes Grand prize is a trip for two to an Open Source event anywhere in the world http://moblin-contest.org/redirect.php?banner_id=100&url=/ _______________________________________________ JSch-users mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/jsch-users
