Bug fix: Failure when opening two SSH sessions at the same time

The primary cause was concurrent use of buffer-like MontyContext::scratch member.
But other non-thread-safe code was fixed along.

Found while implementing Issue 2318 which involves adding multiple queue items at the same moment

Source commit: f08842701165b662e475d249d0612a4404731a39

source/core/PuttyIntf.cpp

@@ -21,7 +21,6 @@ char appname_[50];
 const char *const appname = appname_;
 extern const bool share_can_be_downstream = false;
 extern const bool share_can_be_upstream = false;
-THierarchicalStorage * PuttyStorage = NULL;
 //---------------------------------------------------------------------------
 extern "C"
 {
@@ -466,7 +465,8 @@ ScpSeat::ScpSeat(TSecureShell * ASecureShell)
   vt = &ScpSeatVtable;
 }
 //---------------------------------------------------------------------------
-static std::unique_ptr<TCriticalSection> PuttyRegistrySection(TraceInitPtr(new TCriticalSection()));
+std::unique_ptr<TCriticalSection> PuttyStorageSection(TraceInitPtr(new TCriticalSection()));
+THierarchicalStorage * PuttyStorage = NULL;
 enum TPuttyRegistryMode { prmPass, prmRedirect, prmCollect, prmFail };
 static TPuttyRegistryMode PuttyRegistryMode = prmRedirect;
 typedef std::map<UnicodeString, unsigned long> TPuttyRegistryTypes;
@@ -482,7 +482,7 @@ void putty_registry_pass(bool enable)
 {
   if (enable)
   {
-    PuttyRegistrySection->Enter();
+    PuttyStorageSection->Enter();
     DebugAssert(PuttyRegistryMode == prmRedirect);
     PuttyRegistryMode = prmPass;
   }
@@ -490,7 +490,7 @@ void putty_registry_pass(bool enable)
   {
     DebugAssert(PuttyRegistryMode == prmPass);
     PuttyRegistryMode = prmRedirect;
-    PuttyRegistrySection->Leave();
+    PuttyStorageSection->Leave();
   }
 }
 //---------------------------------------------------------------------------
@@ -1511,7 +1511,7 @@ void WritePuttySettings(THierarchicalStorage * Storage, const UnicodeString & AS
 {
   if (PuttyRegistryTypes.empty())
   {
-    TGuard Guard(PuttyRegistrySection.get());
+    TGuard Guard(PuttyStorageSection.get());
     TValueRestorer<TPuttyRegistryMode> PuttyRegistryModeRestorer(PuttyRegistryMode, prmCollect);
     Conf * conf = conf_new();
     try
@@ -1556,14 +1556,14 @@ void WritePuttySettings(THierarchicalStorage * Storage, const UnicodeString & AS
 //---------------------------------------------------------------------------
 void PuttyDefaults(Conf * conf)
 {
-  TGuard Guard(PuttyRegistrySection.get());
+  TGuard Guard(PuttyStorageSection.get());
   TValueRestorer<TPuttyRegistryMode> PuttyRegistryModeRestorer(PuttyRegistryMode, prmFail);
   do_defaults(NULL, conf);
 }
 //---------------------------------------------------------------------------
 void SavePuttyDefaults(const UnicodeString & Name)
 {
-  TGuard Guard(PuttyRegistrySection.get());
+  TGuard Guard(PuttyStorageSection.get());
   TValueRestorer<TPuttyRegistryMode> PuttyRegistryModeRestorer(PuttyRegistryMode, prmPass);
   Conf * conf = conf_new();
   try

source/core/PuttyIntf.h

@@ -41,6 +41,7 @@ struct ScpSeat : public Seat
   ScpSeat(TSecureShell * SecureShell);
 };
 //---------------------------------------------------------------------------
+extern std::unique_ptr<TCriticalSection> PuttyStorageSection;
 extern THierarchicalStorage * PuttyStorage;
 //---------------------------------------------------------------------------
 #endif

source/core/SecureShell.cpp

@@ -23,7 +23,6 @@
 #define MAX_BUFSIZE 32768
 //---------------------------------------------------------------------------
 const wchar_t HostKeyDelimiter = L';';
-static std::unique_ptr<TCriticalSection> PuttyStorageSection(TraceInitPtr(new TCriticalSection()));
 //---------------------------------------------------------------------------
 struct TPuttyTranslation
 {

source/putty/crypto/mpint.c

@@ -13,6 +13,9 @@
 #include "mpint.h"
 #include "mpint_i.h"
 
+// for WINSCP_PUTTY_SECTION_*
+#include "putty.h"
+
 #pragma warn -ngu // WINSCP
 
 #define SIZE_T_BITS (CHAR_BIT * sizeof(size_t))
@@ -1507,7 +1510,7 @@ MontyContext *monty_new(mp_int *modulus)
         mc->powers_of_r_mod_m[j] = mp_modmul(
             mc->powers_of_r_mod_m[0], mc->powers_of_r_mod_m[j-1], mc->m);
 
-    mc->scratch = mp_make_sized(monty_scratch_size(mc));
+    mc->winscp_guarded_scratch = mp_make_sized(monty_scratch_size(mc));
 
     return mc;
     } // WINSCP
@@ -1520,7 +1523,7 @@ void monty_free(MontyContext *mc)
     for (j = 0; j < 3; j++)
         mp_free(mc->powers_of_r_mod_m[j]);
     mp_free(mc->minus_minv_mod_r);
-    mp_free(mc->scratch);
+    mp_free(mc->winscp_guarded_scratch);
     smemclr(mc, sizeof(*mc));
     sfree(mc);
 }
@@ -1585,16 +1588,18 @@ void monty_mul_into(MontyContext *mc, mp_int *r, mp_int *x, mp_int *y)
     assert(x->nw <= mc->rw);
     assert(y->nw <= mc->rw);
 
+    WINSCP_PUTTY_SECTION_ENTER;
     { // WINSCP
-    mp_int scratch = *mc->scratch;
+    mp_int scratch = *mc->winscp_guarded_scratch;
     mp_int tmp = mp_alloc_from_scratch(&scratch, 2*mc->rw);
     mp_mul_into(&tmp, x, y);
     { // WINSCP
     mp_int reduced = monty_reduce_internal(mc, &tmp, scratch);
     mp_copy_into(r, &reduced);
-    mp_clear(mc->scratch);
+    mp_clear(mc->winscp_guarded_scratch);
     } // WINSCP
     } // WINSCP
+    WINSCP_PUTTY_SECTION_LEAVE;
 }
 
 mp_int *monty_mul(MontyContext *mc, mp_int *x, mp_int *y)
@@ -1647,10 +1652,14 @@ void monty_import_into(MontyContext *mc, mp_int *r, mp_int *x)
  */
 void monty_export_into(MontyContext *mc, mp_int *r, mp_int *x)
 {
-    pinitassert(x->nw <= 2*mc->rw);
-    mp_int reduced = monty_reduce_internal(mc, x, *mc->scratch);
+    assert(x->nw <= 2*mc->rw);
+    WINSCP_PUTTY_SECTION_ENTER;
+    { // WINSCP
+    mp_int reduced = monty_reduce_internal(mc, x, *mc->winscp_guarded_scratch);
     mp_copy_into(r, &reduced);
-    mp_clear(mc->scratch);
+    mp_clear(mc->winscp_guarded_scratch);
+    WINSCP_PUTTY_SECTION_LEAVE;
+    } // WINSCP
 }
 
 mp_int *monty_export(MontyContext *mc, mp_int *x)
@@ -1783,7 +1792,9 @@ mp_int *monty_pow(MontyContext *mc, mp_int *base, mp_int *exponent)
     for (i = 0; i < MODPOW_WINDOW_SIZE; i++)
         mp_free(table[i]);
     mp_free(table_entry);
-    mp_clear(mc->scratch);
+    WINSCP_PUTTY_SECTION_ENTER;
+    mp_clear(mc->winscp_guarded_scratch);
+    WINSCP_PUTTY_SECTION_LEAVE;
     return out;
     } // WINSCP
     } // WINSCP

source/putty/crypto/mpint_i.h

@@ -317,7 +317,9 @@ struct MontyContext {
      * Persistent scratch space from which monty_* functions can
      * allocate storage for intermediate values.
      */
-    mp_int *scratch;
+    // WINSCP: this is volatile member that needs to be guarded for thread-safety
+    // We want to know if it starts being used elsewhre/otherwise.
+    mp_int *winscp_guarded_scratch;
 };
 
 /* Functions shared between mpint.c and mpunsafe.c */

source/putty/windows/network.c

@@ -904,7 +904,9 @@ static Socket *sk_net_accept(accept_ctx_t ctx, Plug *plug)
         return &s->sock;
     }
 
+    WINSCP_PUTTY_SECTION_ENTER;
     add234(sktree, s);
+    WINSCP_PUTTY_SECTION_LEAVE;
 
     return &s->sock;
 }
@@ -957,7 +959,9 @@ static DWORD try_connect(NetSocket *sock,
      * sorting criterion. We'll add it back before exiting this
      * function, whether we changed anything or not.
      */
+    WINSCP_PUTTY_SECTION_ENTER;
     del234(sktree, sock);
+    WINSCP_PUTTY_SECTION_LEAVE;
 
     s = p_socket(family, SOCK_STREAM, 0);
     sock->s = s;
@@ -1170,7 +1174,9 @@ static DWORD try_connect(NetSocket *sock,
     /*
      * No matter what happened, put the socket back in the tree.
      */
+    WINSCP_PUTTY_SECTION_ENTER;
     add234(sktree, sock);
+    WINSCP_PUTTY_SECTION_LEAVE;
 
     if (err) {
         SockAddr thisaddr = sk_extractaddr_tmp(
@@ -1420,7 +1426,9 @@ static Socket *sk_newlistener_internal(
         return &s->sock;
     }
 
+    WINSCP_PUTTY_SECTION_ENTER;
     add234(sktree, s);
+    WINSCP_PUTTY_SECTION_LEAVE;
 
 #ifndef NO_IPV6
     /*
@@ -1482,7 +1490,9 @@ static void sk_net_close(Socket *sock)
 
     bufchain_clear(&s->output_data);
 
+    WINSCP_PUTTY_SECTION_ENTER;
     del234(sktree, s);
+    WINSCP_PUTTY_SECTION_LEAVE;
 #ifdef MPEXT
     do_select(s->plug, s->s, false);
 #else
@@ -1519,7 +1529,11 @@ static void socket_error_callback(void *vs)
      * Just in case other socket work has caused this socket to vanish
      * or become somehow non-erroneous before this callback arrived...
      */
-    if (!find234(sktree, s, NULL) || !s->pending_error)
+    int nr;
+    WINSCP_PUTTY_SECTION_ENTER;
+    nr = !find234(sktree, s, NULL) || !s->pending_error;
+    WINSCP_PUTTY_SECTION_LEAVE;
+    if (nr)
         return;
 
     /*
@@ -1683,7 +1697,9 @@ void select_result(WPARAM wParam, LPARAM lParam)
     if (wParam == 0)
         return;                /* boggle */
 
+    WINSCP_PUTTY_SECTION_ENTER;
     s = find234(sktree, (void *) wParam, cmpforsearch);
+    WINSCP_PUTTY_SECTION_LEAVE;
     if (!s)
         return;                /* boggle */
 
@@ -1949,6 +1965,9 @@ static void sk_net_set_frozen(Socket *sock, bool is_frozen)
     s->frozen_readable = false;
 }
 
+#ifndef WINSCP
+// WINSCP: if ever needed, do not forget about guarding access to sktree
+
 void socket_reselect_all(void)
 {
     NetSocket *s;
@@ -1956,11 +1975,7 @@ void socket_reselect_all(void)
 
     for (i = 0; (s = index234(sktree, i)) != NULL; i++) {
         if (!s->frozen)
-#ifdef MPEXT
-            do_select(s->plug, s->s, true);
-#else
             do_select(s->s, true);
-#endif
     }
 }
 
@@ -1990,6 +2005,7 @@ bool socket_writable(SOCKET skt)
     else
         return false;
 }
+#endif
 
 int net_service_lookup(const char *service)
 {