CONCPP-138 Application could crash if rs used after connection closed

That could happen only with binary results, i.e. with results of
PreparedStatement if useServerPrepStmts option set.

Also the commit contain some performance improvements. In particular for
converting string values to double.

src/com/RowProtocol.cpp

@@ -17,7 +17,7 @@
    51 Franklin St., Fifth Floor, Boston, MA 02110, USA
 *************************************************************************************/
 
-
+#include <cstring>
 #include "RowProtocol.h"
 
 #include "ColumnDefinition.h"
@@ -188,13 +188,17 @@ namespace mariadb
 
   long double RowProtocol::stringToDouble(const char* str, uint32_t len)
   {
-    std::string doubleAsString(str, len);
-    std::istringstream convStream(doubleAsString);
-    std::locale C("C");
-    long double result;
-    convStream.imbue(C);
-    convStream >> result;
-
+    char *end= nullptr;
+    long double result= std::strtod(str, &end);
+    /* Noramlly it should be always NULL-terminated. And if we read past the end - that could already end up bad
+     * but leaving in place this paranoid branch with old code
+     */
+    if (end - str > len) {
+      std::istringstream convStream(str, len);
+      std::locale C("C");
+      convStream.imbue(C);
+      convStream >> result;
+    }
     return result;
   }
 

src/protocol/capi/ConnectProtocol.cpp

@@ -249,15 +249,16 @@ namespace capi
   /** Closes socket and stream readers/writers Attempts graceful shutdown. */
   void ConnectProtocol::close()
   {
-    std::unique_lock<std::mutex> localScopeLock(*lock);
     this->connected= false;
     try {
       // skip acquires lock
-      localScopeLock.unlock();
       skip();
-    }catch (std::runtime_error& ){
     }
-    localScopeLock.lock();
+    catch (std::runtime_error& ) {
+    }
+    std::unique_lock<std::mutex> localScopeLock(*lock);
+    // We still can statements waiting to be closed
+    forceReleaseWaitingPrepareStatement();
     closeSocket();
     cleanMemory();
   }

src/protocol/capi/QueryProtocol.cpp

@@ -1,5 +1,5 @@
 /************************************************************************************
-   Copyright (C) 2020,2024 MariaDB Corporation plc
+   Copyright (C) 2020,2025 MariaDB Corporation plc
 
    This library is free software; you can redistribute it and/or
    modify it under the terms of the GNU Library General Public
@@ -349,7 +349,6 @@ namespace capi
     if ((serverCapabilities & MariaDbServerCapabilities::_MARIADB_CLIENT_STMT_BULK_OPERATIONS) == 0)
       return false;
 
-    SQLString sql(origSql);
     // ensure that type doesn't change
     std::vector<Shared::ParameterHolder> initParameters= parametersList.front();
     std::size_t parameterCount= initParameters.size();
@@ -380,7 +379,7 @@ namespace capi
     }
 
     // any select query is not applicable to bulk
-    if (Utils::findstrni(StringImp::get(sql), "select", 6) != std::string::npos) {
+    if (Utils::findstrni(StringImp::get(origSql), "select", 6) != std::string::npos) {
       return false;
     }
 
@@ -395,7 +394,7 @@ namespace capi
       // send PREPARE if needed
       // **************************************************************************************
       if (!tmpServerPrepareResult){
-        tmpServerPrepareResult= prepareInternal(sql, true);
+        tmpServerPrepareResult= prepareInternal(origSql, true);
       }
 
       // **************************************************************************************
@@ -421,7 +420,7 @@ namespace capi
       }
       catch (SQLException& sqle) {
         if (!serverPrepareResult && tmpServerPrepareResult) {
-          releasePrepareStatement(tmpServerPrepareResult);
+          //releasePrepareStatement(tmpServerPrepareResult);
           // releasePrepareStatement basically cares only about releasing stmt on server(and C API handle)
           delete tmpServerPrepareResult;
           tmpServerPrepareResult= nullptr;
@@ -434,7 +433,7 @@ namespace capi
           return false;
         }
         if (exception.getMessage().empty()) {
-          exception= logQuery->exceptionWithQuery(sql, sqle, explicitClosed);
+          exception= logQuery->exceptionWithQuery(origSql, sqle, explicitClosed);
           if (!options->continueBatchOnError){
             throw exception;
           }
@@ -447,16 +446,15 @@ namespace capi
       results->setRewritten(true);
       
       if (!serverPrepareResult && tmpServerPrepareResult) {
-        releasePrepareStatement(tmpServerPrepareResult);
+        //releasePrepareStatement(tmpServerPrepareResult);
         // releasePrepareStatement basically cares only about releasing stmt on server(and C API handle)
         delete tmpServerPrepareResult;
       }
       return true;
-
     }
     catch (std::runtime_error& e) {
       if (!serverPrepareResult && tmpServerPrepareResult) {
-        releasePrepareStatement(tmpServerPrepareResult);
+        //releasePrepareStatement(tmpServerPrepareResult);
         // releasePrepareStatement basically cares only about releasing stmt on server(and C API handle)
         delete tmpServerPrepareResult;
       }
@@ -1075,7 +1073,6 @@ namespace capi
       Shared::Results& results,
       std::vector<Shared::ParameterHolder>& parameters)
   {
-
     cmdPrologue();
 
     try {
@@ -1103,10 +1100,13 @@ namespace capi
       }
       /*CURSOR_TYPE_NO_CURSOR);*/
       getResult(results.get(), serverPrepareResult);
-
-    }catch (SQLException& qex){
+      // We have to do this due to CONCPP-138
+      results->loadFully(false, this);
+    }
+    catch (SQLException& qex) {
       throw logQuery->exceptionWithQuery(parameters, qex, serverPrepareResult);
-    }catch (std::runtime_error& e){
+    }
+    catch (std::runtime_error& e) {
       handleIoException(e).Throw();
     }
   }
@@ -1167,7 +1167,7 @@ namespace capi
    */
   void QueryProtocol::forceReleaseWaitingPrepareStatement()
   {
-    if (statementIdToRelease != nullptr && forceReleasePrepareStatement(statementIdToRelease)){
+    if (statementIdToRelease != nullptr && capi::mysql_stmt_close(statementIdToRelease) == 0) {
       statementIdToRelease= nullptr;
     }
   }

test/unit/classes/preparedstatement.cpp

@@ -2170,8 +2170,6 @@ void preparedstatement::concpp116_getByte()
 /* CONCPP - 133 */
 void preparedstatement::multirs_caching()
 {
-  //SKIP("This is not working and won't be fixed in this version");
-
   createSchemaObject("PROCEDURE", "ccpptest_multirs_caching", "()\
                         BEGIN\
                           SELECT 1 as id, 'text' as val UNION SELECT 7 as id, 'seven' as val;\
@@ -2218,6 +2216,7 @@ void preparedstatement::multirs_caching()
   ASSERT_EQUALS(-1, pstmt1->getUpdateCount());
 }
 
+/* if sql::bytes wrapped C array, setBytes would crash the application */
 void preparedstatement::bytesArrParam()
 {
   pstmt.reset(con->prepareStatement("SELECT ?"));
@@ -2245,5 +2244,24 @@ void preparedstatement::bytesArrParam()
   ASSERT_EQUALS(1, res->getInt(1));
 }
 
+/* CONCPP-138 application crashes if binary resultset used after closing the connection */
+void preparedstatement::concpp138_useRsAfterConClose()
+{
+  pstmt.reset(sspsCon->prepareStatement("SELECT 1275, 'Just some text', 1.25"));
+  ssps.reset(sspsCon->prepareStatement("SELECT 55555"));
+
+  ResultSet rs(pstmt->executeQuery());
+  res.reset(ssps->executeQuery());
+  sspsCon->close();
+  ASSERT(res->next());
+  ASSERT_EQUALS(55555, res->getInt(1));
+  ASSERT(rs->next());
+  ASSERT_EQUALS(1275, rs->getInt(1));
+  ASSERT_EQUALS("Just some text", rs->getString(2));
+  ASSERT_EQUALS_EPSILON(1.25, rs->getDouble(3), 0.0001);
+  ASSERT(!res->next());
+  ASSERT(!rs->next());
+  sspsCon.reset();
+}
 } /* namespace preparedstatement */
 } /* namespace testsuite */

test/unit/classes/preparedstatementtest.h

@@ -80,6 +80,7 @@ public:
     TEST_CASE(concpp116_getByte);
     TEST_CASE(multirs_caching);
     TEST_CASE(bytesArrParam);
+    TEST_CASE(concpp138_useRsAfterConClose);
   }
 
   /**
@@ -171,12 +172,13 @@ public:
   void concpp116_getByte();
 
   void multirs_caching();
-
   /**
    * sql::bytes may have negative length(internally) and that caused problems
    */
   void bytesArrParam();
 
+  void concpp138_useRsAfterConClose();
+
   /* unit_fixture methods overriding */
   void setUp();
 };