Fix segfault due to invalid memory access on SharedFiles class

Issue #1318

Author: zimmerle

src/utils/shared_files.cc

@@ -37,27 +37,22 @@ namespace utils {
 
 msc_file_handler_t *SharedFiles::find_handler(
     const std::string &fileName) {
-    msc_file_handler_t *current = m_first;
-    while (current != NULL) {
-        if (current->file_name == fileName) {
-            return current;
+    for (const auto &i: m_handlers) {
+        if (i.first == fileName) {
+            return i.second;
         }
-        current = reinterpret_cast<msc_file_handler_t *>(current->next);
     }
-
     return NULL;
 }
 
 
 msc_file_handler_t *SharedFiles::add_new_handler(
     const std::string &fileName, std::string *error) {
-    msc_file_handler_t *current = m_first;
     int shm_id;
     key_t mem_key_structure;
-    key_t mem_key_file_name;
     msc_file_handler_t *new_debug_log;
-    char *shm_ptr2;
     FILE *fp;
+    bool toBeCreated = true;
 
     fp = fopen(fileName.c_str(), "a");
     if (fp == 0) {
@@ -72,81 +67,44 @@ msc_file_handler_t *SharedFiles::add_new_handler(
         goto err_mem_key;
     }
 
-    mem_key_file_name = ftok(fileName.c_str(), 2);
-    if (mem_key_file_name < 0) {
-        error->assign("Failed to select key for the shared memory (2): ");
-        error->append(strerror(errno));
-        goto err_mem_key;
-    }
-
-    shm_id = shmget(mem_key_structure, sizeof (msc_file_handler_t),
-        IPC_CREAT | 0666);
+    shm_id = shmget(mem_key_structure, sizeof (msc_file_handler_t) + fileName.size() + 1,
+        IPC_CREAT | IPC_EXCL | 0666);
     if (shm_id < 0) {
-        error->assign("Failed to allocate shared memory (1): ");
-        error->append(strerror(errno));
-        goto err_shmget1;
+        shm_id = shmget(mem_key_structure, sizeof (msc_file_handler_t)
+            + fileName.size() + 1, IPC_CREAT | 0666);
+        toBeCreated = false;
+        if (shm_id < 0) {
+            error->assign("Failed to allocate shared memory (1): ");
+            error->append(strerror(errno));
+            goto err_shmget1;
+        }
     }
 
     new_debug_log = reinterpret_cast<msc_file_handler_t *>(
-        shmat(shm_id, NULL, 0));
+            shmat(shm_id, NULL, 0));
     if ((reinterpret_cast<char *>(new_debug_log)[0]) == -1) {
         error->assign("Failed to attach shared memory (1): ");
         error->append(strerror(errno));
         goto err_shmat1;
     }
-    memset(new_debug_log, '\0', sizeof(msc_file_handler_t));
-
-    pthread_mutex_init(&new_debug_log->lock, NULL);
-    new_debug_log->fp = fp;
-    new_debug_log->file_handler = fileno(new_debug_log->fp);
-    new_debug_log->next = NULL;
-    new_debug_log->previous = NULL;
-    new_debug_log->shm_id_structure = shm_id;
-    shm_id = shmget(mem_key_file_name, (fileName.size() + 1 * sizeof(char)),
-        IPC_CREAT | 0666);
-    if (shm_id < 0) {
-        error->assign("Failed to allocate shared memory (2): ");
-        error->append(strerror(errno));
-        goto err_shmget2;
-    }
-    new_debug_log->shm_id_file_name = shm_id;
-    shm_ptr2 = reinterpret_cast<char *>(shmat(shm_id, NULL, 0));
-    if (shm_ptr2[0] == -1) {
-        error->assign("Failed to attach shared memory (2): ");
-        error->append(strerror(errno));
-        goto err_shmat2;
-    }
-    memcpy(shm_ptr2, fileName.c_str(), fileName.size());
-    shm_ptr2[fileName.size()] = '\0';
-
-    new_debug_log->file_name = shm_ptr2;
-
-    if (m_first == NULL) {
-        m_first = new_debug_log;
-    } else {
-        current = m_first;
-        while (current != NULL) {
-            if (current->next == NULL) {
-                current->next = new_debug_log;
-                new_debug_log->previous = current;
-                new_debug_log->next = NULL;
-                current = NULL;
-            } else {
-                current = reinterpret_cast<msc_file_handler_t *>(
-                    current->next);
-            }
-        }
+
+    if (toBeCreated) {
+        memset(new_debug_log, '\0', sizeof(msc_file_handler_t));
+        pthread_mutex_init(&new_debug_log->lock, NULL);
+        new_debug_log->fp = fp;
+        new_debug_log->file_handler = fileno(new_debug_log->fp);
+        new_debug_log->shm_id_structure = shm_id;
+        memcpy(new_debug_log->file_name, fileName.c_str(), fileName.size());
+        new_debug_log->file_name[fileName.size()] = '\0';
     }
+    m_handlers.push_back(std::make_pair(fileName, new_debug_log));
 
     return new_debug_log;
-err_shmget2:
-err_shmat2:
-    shmdt(shm_ptr2);
-    fclose(new_debug_log->fp);
 err_shmget1:
 err_shmat1:
     shmdt(new_debug_log);
 err_mem_key:
+    fclose(fp);
 err_fh:
     return NULL;
 }
@@ -173,6 +131,7 @@ bool SharedFiles::open(const std::string& fileName, std::string *error) {
 
 void SharedFiles::close(const std::string& fileName) {
     msc_file_handler_t *a;
+    int j = 0;
 
     if (fileName.empty()) {
         return;
@@ -186,41 +145,23 @@ void SharedFiles::close(const std::string& fileName) {
     a->using_it--;
 
     if (a->using_it == 0) {
-        bool first = false;
         int shm_id1 = a->shm_id_structure;
-        int shm_id2 = a->shm_id_file_name;
         msc_file_handler_t *p , *n;
         pthread_mutex_lock(&a->lock);
         fclose(a->fp);
-
-        p = reinterpret_cast<msc_file_handler_t *>(a->previous);
-        n = reinterpret_cast<msc_file_handler_t *>(a->next);
-        if (p != NULL) {
-            p->next = reinterpret_cast<msc_file_handler_t *>(n);
-        }
-        if (n != NULL) {
-            n->previous = reinterpret_cast<msc_file_handler_t *>(p);
-        }
-        a->previous = NULL;
-        a->next = NULL;
         pthread_mutex_unlock(&a->lock);
         pthread_mutex_destroy(&a->lock);
-
-        if (a->file_name == m_first->file_name) {
-            first = true;
-        }
-
-        shmdt(a->file_name);
         shmdt(a);
-
         shmctl(shm_id1, IPC_RMID, NULL);
-        shmctl(shm_id2, IPC_RMID, NULL);
+    }
 
-        if (first) {
-            m_first = NULL;
+    for (const auto &i: m_handlers) {
+        if (i.first == fileName) {
+            j++;
         }
-        a = NULL;
     }
+
+    m_handlers.erase(m_handlers.begin() + j, m_handlers.begin() + j + 1);
 }
 
 

src/utils/shared_files.h

@@ -33,15 +33,12 @@ namespace utils {
 
 
 typedef struct msc_file_handler {
-    char *file_name;
     FILE *fp;
     int file_handler;
-    int shm_id_file_name;
     int shm_id_structure;
     int using_it;
     pthread_mutex_t lock;
-    void *next;
-    void *previous;
+    char file_name[];
 } msc_file_handler_t;
 
 
@@ -62,7 +59,7 @@ class SharedFiles {
         std::string *error);
 
  private:
-    SharedFiles() : m_first(NULL) { }
+    SharedFiles() { }
     ~SharedFiles() { }
 
     // C++ 03
@@ -73,7 +70,7 @@ class SharedFiles {
     SharedFiles(SharedFiles const&);
     void operator=(SharedFiles const&);
 
-    msc_file_handler_t *m_first;
+    std::vector<std::pair<std::string, msc_file_handler *>> m_handlers;
 };