Re: [JSch-users] JSch-0.1.37: NPE in Session.disconnect() when calledbefore authenticated

[Oberhuber, M. <Mar...@wi...>]

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:ym...@jc...] 
> Sent: Monday, July 14, 2008 9:13 AM
> To: Oberhuber, Martin
> Cc: jsc...@li...
> Subject: Re: [JSch-users] JSch-0.1.37: NPE in 
> Session.disconnect() when calledbefore authenticated
> 
> Hi,
> 
>    +-From: "Oberhuber, Martin" <Mar...@wi...> --
>    |_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{
>