Merge pull request #3190 from Carbenium/db-locking

Fix SQL queries with missing lock

cmake/compiler/gcc/settings.cmake

@@ -2,6 +2,7 @@ target_compile_options(zm-warning-interface
   INTERFACE
     -Wall
     -Wextra
+    -Wformat-security
     -Wno-cast-function-type
     -Wno-type-limits
     -Wno-unused-parameter)

src/zm_config.cpp

@@ -340,16 +340,11 @@ Config::~Config() {
 }
 
 void Config::Load() {
-  if ( mysql_query(&dbconn, "SELECT `Name`, `Value`, `Type` FROM `Config` ORDER BY `Id`") ) {
+  MYSQL_RES *result = zmDbFetch("SELECT `Name`, `Value`, `Type` FROM `Config` ORDER BY `Id`");
-    Error("Can't run query: %s", mysql_error(&dbconn));
+  if (!result) {
-    exit(mysql_errno(&dbconn));
+    exit(-1);
   }
 
-  MYSQL_RES *result = mysql_store_result(&dbconn);
-  if ( !result ) {
-    Error("Can't use query result: %s", mysql_error(&dbconn));
-    exit(mysql_errno(&dbconn));
-  }
   n_items = mysql_num_rows(result);
 
   if ( n_items <= ZM_MAX_CFG_ID ) {
@@ -362,7 +357,6 @@ void Config::Load() {
     items[i] = new ConfigItem(dbrow[0], dbrow[1], dbrow[2]);
   }
   mysql_free_result(result);
-  result = nullptr;
 }
 
 void Config::Assign() {

src/zm_eventstream.cpp

@@ -41,22 +41,14 @@ const std::string EventStream::StreamMode_Strings[4] = {
 };
 
 bool EventStream::loadInitialEventData(int monitor_id, time_t event_time) {
-  static char sql[ZM_SQL_SML_BUFSIZ];
+  std::string sql = stringtf("SELECT `Id` FROM `Events` WHERE "
+                             "`MonitorId` = %d AND unix_timestamp(`EndDateTime`) > %ld "
+                             "ORDER BY `Id` ASC LIMIT 1", monitor_id, event_time);
 
-  snprintf(sql, sizeof(sql), "SELECT `Id` FROM `Events` WHERE "
+  MYSQL_RES *result = zmDbFetch(sql.c_str());
-      "`MonitorId` = %d AND unix_timestamp(`EndDateTime`) > %ld "
+  if (!result)
-      "ORDER BY `Id` ASC LIMIT 1", monitor_id, event_time);
+    exit(-1);
 
-  if ( mysql_query(&dbconn, sql) ) {
-    Error("Can't run query: %s", mysql_error(&dbconn));
-    exit(mysql_errno(&dbconn));
-  }
-
-  MYSQL_RES *result = mysql_store_result(&dbconn);
-  if ( !result ) {
-    Error("Can't use query result: %s", mysql_error(&dbconn));
-    exit(mysql_errno(&dbconn));
-  }
   MYSQL_ROW dbrow = mysql_fetch_row(result);
 
   if ( mysql_errno(&dbconn) ) {
@@ -115,23 +107,15 @@ bool EventStream::loadInitialEventData(
 }
 
 bool EventStream::loadEventData(uint64_t event_id) {
-  static char sql[ZM_SQL_MED_BUFSIZ];
+  std::string sql = stringtf(
-
-  snprintf(sql, sizeof(sql),
       "SELECT `MonitorId`, `StorageId`, `Frames`, unix_timestamp( `StartDateTime` ) AS StartTimestamp, "
       "unix_timestamp( `EndDateTime` ) AS EndTimestamp, "
       "(SELECT max(`Delta`)-min(`Delta`) FROM `Frames` WHERE `EventId`=`Events`.`Id`) AS FramesDuration, "
       "`DefaultVideo`, `Scheme`, `SaveJPEGs`, `Orientation`+0 FROM `Events` WHERE `Id` = %" PRIu64, event_id);
 
-  if ( mysql_query(&dbconn, sql) ) {
+  MYSQL_RES *result = zmDbFetch(sql.c_str());
-    Error("Can't run query: %s", mysql_error(&dbconn));
+  if (!result) {
-    exit(mysql_errno(&dbconn));
+    exit(-1);
-  }
-
-  MYSQL_RES *result = mysql_store_result(&dbconn);
-  if ( !result ) {
-    Error("Can't use query result: %s", mysql_error(&dbconn));
-    exit(mysql_errno(&dbconn));
   }
 
   if ( !mysql_num_rows(result) ) {
@@ -228,17 +212,12 @@ bool EventStream::loadEventData(uint64_t event_id) {
 
   updateFrameRate((event_data->frame_count and event_data->duration) ? (double)event_data->frame_count/event_data->duration : 1);
 
-  snprintf(sql, sizeof(sql), "SELECT `FrameId`, unix_timestamp(`TimeStamp`), `Delta` "
+  sql = stringtf("SELECT `FrameId`, unix_timestamp(`TimeStamp`), `Delta` "
-      "FROM `Frames` WHERE `EventId` = %" PRIu64 " ORDER BY `FrameId` ASC", event_id);
+                 "FROM `Frames` WHERE `EventId` = %" PRIu64 " ORDER BY `FrameId` ASC", event_id);
-  if ( mysql_query(&dbconn, sql) ) {
-    Error("Can't run query: %s", mysql_error(&dbconn));
-    exit(mysql_errno(&dbconn));
-  }
 
-  result = mysql_store_result(&dbconn);
+  result = zmDbFetch(sql.c_str());
-  if ( !result ) {
+  if (!result) {
-    Error("Can't use query result: %s", mysql_error(&dbconn));
+    exit(-1);
-    exit(mysql_errno(&dbconn));
   }
 
   event_data->n_frames = mysql_num_rows(result);
@@ -604,10 +583,10 @@ void EventStream::processCommand(const CmdMsg *msg) {
 }  // void EventStream::processCommand(const CmdMsg *msg)
 
 bool EventStream::checkEventLoaded() {
-  static char sql[ZM_SQL_SML_BUFSIZ];
+  std::string sql;
 
   if ( curr_frame_id <= 0 ) {
-    snprintf(sql, sizeof(sql),
+    sql = stringtf(
         "SELECT `Id` FROM `Events` WHERE `MonitorId` = %d AND `Id` < %" PRIu64 " ORDER BY `Id` DESC LIMIT 1",
         event_data->monitor_id, event_data->event_id);
   } else if ( (unsigned int)curr_frame_id > event_data->last_frame_id ) {
@@ -618,7 +597,7 @@ bool EventStream::checkEventLoaded() {
         curr_frame_id = event_data->last_frame_id;
       return false;
     }
-    snprintf(sql, sizeof(sql),
+    sql = stringtf(
         "SELECT `Id` FROM `Events` WHERE `MonitorId` = %d AND `Id` > %" PRIu64 " ORDER BY `Id` ASC LIMIT 1",
         event_data->monitor_id, event_data->event_id);
   } else {
@@ -630,19 +609,15 @@ bool EventStream::checkEventLoaded() {
 
   // Event change required.
   if ( forceEventChange || ( (mode != MODE_SINGLE) && (mode != MODE_NONE) ) ) {
-    Debug(1, "Checking for next event %s", sql);
+    Debug(1, "Checking for next event %s", sql.c_str());
-    if ( mysql_query(&dbconn, sql) ) {
+
-      Error("Can't run query: %s", mysql_error(&dbconn));
+    MYSQL_RES *result = zmDbFetch(sql.c_str());
-      exit(mysql_errno(&dbconn));
+    if (!result) {
+      exit(-1);
     }
 
-    MYSQL_RES *result = mysql_store_result(&dbconn);
-    if ( !result ) {
-      Error("Can't use query result: %s", mysql_error(&dbconn));
-      exit(mysql_errno(&dbconn));
-    }
     if ( mysql_num_rows(result) != 1 ) {
-      Debug(1, "No rows returned for %s", sql);
+      Debug(1, "No rows returned for %s", sql.c_str());
     }
     MYSQL_ROW dbrow = mysql_fetch_row(result);
 
@@ -664,7 +639,7 @@ bool EventStream::checkEventLoaded() {
       Debug(2, "New frame id = %d", curr_frame_id);
       return true;
     } else {
-      Debug(2, "No next event loaded using %s. Pausing", sql);
+      Debug(2, "No next event loaded using %s. Pausing", sql.c_str());
       if ( curr_frame_id <= 0 )
         curr_frame_id = 1;
       else

src/zm_monitor.cpp

@@ -2391,25 +2391,19 @@ void Monitor::ReloadLinkedMonitors(const char *p_linked_monitors) {
       for ( int i = 0; i < n_link_ids; i++ ) {
         Debug(1, "Checking linked monitor %d", link_ids[i]);
 
-        std::lock_guard<std::mutex> lck(db_mutex);
+        std::string sql = stringtf(
-        static char sql[ZM_SQL_SML_BUFSIZ];
-        snprintf(sql, sizeof(sql),
             "SELECT `Id`, `Name` FROM `Monitors`"
             "  WHERE `Id` = %d"
             "   AND `Function` != 'None'"
             "   AND `Function` != 'Monitor'"
             "   AND `Enabled`=1",
             link_ids[i]);
-        if (mysql_query(&dbconn, sql)) {
+
-          Error("Can't run query: %s", mysql_error(&dbconn));
+        MYSQL_RES *result = zmDbFetch(sql.c_str());
+        if (!result) {
           continue;
         }
 
-        MYSQL_RES *result = mysql_store_result(&dbconn);
-        if ( !result ) {
-          Error("Can't use query result: %s", mysql_error(&dbconn));
-          continue;
-        }
         int n_monitors = mysql_num_rows(result);
         if ( n_monitors == 1 ) {
           MYSQL_ROW dbrow = mysql_fetch_row(result);

src/zm_remote_camera_http.cpp

@@ -105,7 +105,7 @@ void RemoteCameraHttp::Initialise() {
     request += stringtf( "User-Agent: %s/%s\r\n", config.http_ua, ZM_VERSION );
     request += stringtf( "Host: %s\r\n", host.c_str());
     if ( strcmp( config.http_version, "1.0" ) == 0 )
-      request += stringtf( "Connection: Keep-Alive\r\n" );
+      request += "Connection: Keep-Alive\r\n";
     if ( !auth.empty() )
       request += stringtf( "Authorization: Basic %s\r\n", auth64.c_str() );
     request += "\r\n";
@@ -362,7 +362,7 @@ int RemoteCameraHttp::GetResponse() {
                   request += stringtf( "User-Agent: %s/%s\r\n", config.http_ua, ZM_VERSION );
                   request += stringtf( "Host: %s\r\n", host.c_str());
                   if ( strcmp( config.http_version, "1.0" ) == 0 )
-                    request += stringtf( "Connection: Keep-Alive\r\n" );
+                    request += "Connection: Keep-Alive\r\n";
                   request += mAuthenticator->getAuthHeader( "GET", path.c_str() );
                   request += "\r\n";
 
@@ -738,7 +738,7 @@ int RemoteCameraHttp::GetResponse() {
                   request += stringtf("User-Agent: %s/%s\r\n", config.http_ua, ZM_VERSION);
                   request += stringtf("Host: %s\r\n", host.c_str());
                   if ( strcmp(config.http_version, "1.0") == 0 )
-                    request += stringtf("Connection: Keep-Alive\r\n");
+                    request += "Connection: Keep-Alive\r\n";
                   request += mAuthenticator->getAuthHeader("GET", path.c_str());
                   request += "\r\n";
 

src/zm_user.cpp

@@ -94,33 +94,25 @@ bool User::canAccess(int monitor_id) {
 // Function to load a user from username and password
 // Please note that in auth relay mode = none, password is NULL
 User *zmLoadUser(const char *username, const char *password) {
-  char sql[ZM_SQL_MED_BUFSIZ] = "";
   int username_length = strlen(username);
-  char *safer_username = new char[(username_length * 2) + 1];
 
   // According to docs, size of safer_whatever must be 2*length+1
   // due to unicode conversions + null terminator.
-  mysql_real_escape_string(&dbconn, safer_username, username, username_length);
+  std::string escaped_username((username_length * 2) + 1, '\0');
 
-  snprintf(sql, sizeof(sql),
-      "SELECT `Id`, `Username`, `Password`, `Enabled`,"
-      " `Stream`+0, `Events`+0, `Control`+0, `Monitors`+0, `System`+0,"
-      " `MonitorIds`"
-      " FROM `Users` WHERE `Username` = '%s' AND `Enabled` = 1",
-      safer_username);
-  delete[] safer_username;
-  safer_username = nullptr;
 
-  if ( mysql_query(&dbconn, sql) ) {
+  size_t escaped_len = mysql_real_escape_string(&dbconn, &escaped_username[0], username, username_length);
-    Error("Can't run query: %s", mysql_error(&dbconn));
+  escaped_username.resize(escaped_len);
-    exit(mysql_errno(&dbconn));
-  }
 
-  MYSQL_RES *result = mysql_store_result(&dbconn);
+  std::string sql = stringtf("SELECT `Id`, `Username`, `Password`, `Enabled`,"
-  if ( !result ) {
+                             " `Stream`+0, `Events`+0, `Control`+0, `Monitors`+0, `System`+0,"
-    Error("Can't use query result: %s", mysql_error(&dbconn));
+                             " `MonitorIds`"
-    exit(mysql_errno(&dbconn));
+                             " FROM `Users` WHERE `Username` = '%s' AND `Enabled` = 1",
-  }
+                             escaped_username.c_str());
+
+  MYSQL_RES *result = zmDbFetch(sql.c_str());
+  if (!result)
+    return nullptr;
 
   if ( mysql_num_rows(result) == 1 ) {
     MYSQL_ROW dbrow = mysql_fetch_row(result);
@@ -165,22 +157,13 @@ User *zmLoadTokenUser(std::string jwt_token_str, bool use_remote_addr) {
     return nullptr;
   }
 
-  char sql[ZM_SQL_MED_BUFSIZ] = "";
+  std::string sql = stringtf("SELECT `Id`, `Username`, `Password`, `Enabled`, `Stream`+0, `Events`+0,"
-  snprintf(sql, sizeof(sql),
+                             " `Control`+0, `Monitors`+0, `System`+0, `MonitorIds`, `TokenMinExpiry`"
-    "SELECT `Id`, `Username`, `Password`, `Enabled`, `Stream`+0, `Events`+0,"
+                             " FROM `Users` WHERE `Username` = '%s' AND `Enabled` = 1", username.c_str());
-    " `Control`+0, `Monitors`+0, `System`+0, `MonitorIds`, `TokenMinExpiry`"
-    " FROM `Users` WHERE `Username` = '%s' AND `Enabled` = 1", username.c_str());
 
-  if ( mysql_query(&dbconn, sql) ) {
+  MYSQL_RES *result = zmDbFetch(sql.c_str());
-    Error("Can't run query: %s", mysql_error(&dbconn));
+  if (!result)
     return nullptr;
-  }
-
-  MYSQL_RES *result = mysql_store_result(&dbconn);
-  if ( !result ) {
-    Error("Can't use query result: %s", mysql_error(&dbconn));
-    return nullptr;
-  }
 
   int n_users = mysql_num_rows(result);
   if ( n_users != 1 ) {
@@ -227,22 +210,14 @@ User *zmLoadAuthUser(const char *auth, bool use_remote_addr) {
   }
 
   Debug(1, "Attempting to authenticate user from auth string '%s', remote addr(%s)", auth, remote_addr);
-  char sql[ZM_SQL_SML_BUFSIZ] = "";
+  std::string sql = "SELECT `Id`, `Username`, `Password`, `Enabled`,"
-  snprintf(sql, sizeof(sql),
+                    " `Stream`+0, `Events`+0, `Control`+0, `Monitors`+0, `System`+0,"
-      "SELECT `Id`, `Username`, `Password`, `Enabled`,"
+                    " `MonitorIds` FROM `Users` WHERE `Enabled` = 1";
-      " `Stream`+0, `Events`+0, `Control`+0, `Monitors`+0, `System`+0,"
-      " `MonitorIds` FROM `Users` WHERE `Enabled` = 1");
 
-  if ( mysql_query(&dbconn, sql) ) {
+  MYSQL_RES *result = zmDbFetch(sql.c_str());
-    Error("Can't run query: %s", mysql_error(&dbconn));
+  if (!result)
-    exit(mysql_errno(&dbconn));
-  }
-
-  MYSQL_RES *result = mysql_store_result(&dbconn);
-  if ( !result ) {
-    Error("Can't use query result: %s", mysql_error(&dbconn));
     return nullptr;
-  }
+
   int n_users = mysql_num_rows(result);
   if ( n_users < 1 ) {
     mysql_free_result(result);

src/zm_utils.cpp

@@ -65,34 +65,6 @@ std::string replaceAll(std::string str, std::string from, std::string to) {
   return str;
 }
 
-const std::string stringtf( const char *format, ... ) {
-  va_list ap;
-  char tempBuffer[8192];
-  std::string tempString;
-
-  va_start(ap, format);
-  vsnprintf(tempBuffer, sizeof(tempBuffer), format , ap);
-  va_end(ap);
-
-  tempString = tempBuffer;
-
-  return tempString;
-}
-
-const std::string stringtf(const std::string format, ...) {
-  va_list ap;
-  char tempBuffer[8192];
-  std::string tempString;
-
-  va_start(ap, format);
-  vsnprintf(tempBuffer, sizeof(tempBuffer), format.c_str(), ap);
-  va_end(ap);
-
-  tempString = tempBuffer;
-
-  return tempString;
-}
-
 bool startsWith(const std::string &haystack, const std::string &needle) {
   return ( haystack.substr(0, needle.length()) == needle );
 }

src/zm_utils.h

@@ -23,8 +23,9 @@
 #include <chrono>
 #include <ctime>
 #include <memory>
-#include <sys/time.h>
+#include <stdexcept>
 #include <string>
+#include <sys/time.h>
 #include <vector>
 
 typedef std::vector<std::string> StringVector;
@@ -33,8 +34,16 @@ std::string trimSpaces(const std::string &str);
 std::string trimSet(std::string str, std::string trimset);
 std::string replaceAll(std::string str, std::string from, std::string to);
 
-const std::string stringtf( const char *format, ... );
+template<typename... Args>
-const std::string stringtf( const std::string &format, ... );
+std::string stringtf(const std::string &format, Args... args) {
+  int size = snprintf(nullptr, 0, format.c_str(), args...) + 1; // Extra space for '\0'
+  if (size <= 0) {
+    throw std::runtime_error("Error during formatting.");
+  }
+  std::unique_ptr<char[]> buf(new char[size]);
+  snprintf(buf.get(), size, format.c_str(), args...);
+  return std::string(buf.get(), buf.get() + size - 1); // We don't want the '\0' inside
+}
 
 bool startsWith( const std::string &haystack, const std::string &needle );
 StringVector split( const std::string &string, const std::string &chars, int limit=0 );

src/zm_zone.cpp

@@ -819,28 +819,18 @@ bool Zone::ParseZoneString(const char *zone_string, int &zone_id, int &colour, P
 }  // end bool Zone::ParseZoneString(const char *zone_string, int &zone_id, int &colour, Polygon &polygon)
 
 int Zone::Load(Monitor *monitor, Zone **&zones) {
-  static char sql[ZM_SQL_MED_BUFSIZ];
+  std::string sql = stringtf("SELECT Id,Name,Type+0,Units,Coords,AlarmRGB,CheckMethod+0,"
-  MYSQL_RES *result;
+                             "MinPixelThreshold,MaxPixelThreshold,MinAlarmPixels,MaxAlarmPixels,"
+                             "FilterX,FilterY,MinFilterPixels,MaxFilterPixels,"
+                             "MinBlobPixels,MaxBlobPixels,MinBlobs,MaxBlobs,"
+                             "OverloadFrames,ExtendAlarmFrames"
+                             " FROM Zones WHERE MonitorId = %d ORDER BY Type, Id", monitor->Id());
 
-  {  // scope for lock
+  MYSQL_RES *result = zmDbFetch(sql.c_str());
-    std::lock_guard<std::mutex> lck(db_mutex);
-    snprintf(sql, sizeof(sql), "SELECT Id,Name,Type+0,Units,Coords,AlarmRGB,CheckMethod+0,"
-        "MinPixelThreshold,MaxPixelThreshold,MinAlarmPixels,MaxAlarmPixels,"
-        "FilterX,FilterY,MinFilterPixels,MaxFilterPixels,"
-        "MinBlobPixels,MaxBlobPixels,MinBlobs,MaxBlobs,"
-        "OverloadFrames,ExtendAlarmFrames"
-        " FROM Zones WHERE MonitorId = %d ORDER BY Type, Id", monitor->Id());
-    if ( mysql_query(&dbconn, sql) ) {
-      Error("Can't run query: %s", mysql_error(&dbconn));
-      return 0;
-    }
-
-    result = mysql_store_result(&dbconn);
-  }
   if (!result) {
-    Error("Can't use query result: %s", mysql_error(&dbconn));
     return 0;
   }
+
   int n_zones = mysql_num_rows(result);
   Debug(1, "Got %d zones for monitor %s", n_zones, monitor->Name());
   delete[] zones;

src/zmu.cpp

@@ -721,20 +721,14 @@ int main(int argc, char *argv[]) {
 
     if ( function & ZMU_LIST ) {
       std::string sql = "SELECT `Id`, `Function`+0 FROM `Monitors`";
-      if ( !verbose ) {
+      if (!verbose) {
         sql += "WHERE `Function` != 'None'";
       }
       sql += " ORDER BY Id ASC";
 
-      if ( mysql_query(&dbconn, sql.c_str()) ) {
+      MYSQL_RES *result = zmDbFetch(sql.c_str());
-        Error("Can't run query: %s", mysql_error(&dbconn));
+      if (!result) {
-        exit_zmu(mysql_errno(&dbconn));
+        exit_zmu(-1);
-      }
-
-      MYSQL_RES *result = mysql_store_result(&dbconn);
-      if ( !result ) {
-        Error("Can't use query result: %s", mysql_error(&dbconn));
-        exit_zmu(mysql_errno(&dbconn));
       }
       Debug(1, "Got %d monitors", mysql_num_rows(result));