Fix double free in dart_segment (#165)

* Remove dart_segment_remove and make deallocation of segments more robust * dart_segment_dealloc -> dart_segment_free && dart_segment_clear -> dart_segment_fini * Fixed logic error in dart_segment_free
dash-project · Nov 25, 2016 · 0a219ee · 0a219ee
1 parent 7e48d18
commit 0a219ee
Show file tree

Hide file tree

Showing 4 changed files with 27 additions and 74 deletions.
diff --git a/dart-impl/mpi/include/dash/dart/mpi/dart_segment.h b/dart-impl/mpi/include/dash/dart/mpi/dart_segment.h
@@ -44,11 +44,6 @@ dart_ret_t dart_segment_get_teamidx(dart_segid_t segid, uint16_t *team_idx);
  */
 dart_ret_t dart_segment_add_info(const dart_segment_info_t *item);
 
-/**
- * @brief Remove the segment with ID seg_id from the segment hash table.
- */
-dart_ret_t dart_segment_remove(int16_t seg_id);
-
 #if !defined(DART_MPI_DISABLE_SHARED_WINDOWS)
 /** @brief Query the shared memory window object associated with the specified seg_id.
  *
@@ -93,13 +88,13 @@ dart_ret_t dart_segment_get_size(
 /**
  * @brief Deallocates the segment identified by the segment ID.
  */
-dart_ret_t dart_segment_dealloc(dart_segid_t segid);
+dart_ret_t dart_segment_free(dart_segid_t segid);
 
 
 /**
  * @brief Clear the segment data hash table.
  */
-dart_ret_t dart_segment_clear();
+dart_ret_t dart_segment_fini();
 
 
 #endif /* DART_SEGMENT_H_ */
diff --git a/dart-impl/mpi/src/dart_globmem.c b/dart-impl/mpi/src/dart_globmem.c
@@ -390,12 +390,10 @@ dart_ret_t dart_team_memfree(
                  unitid, gptr.addr_or_offs.offset, gptr.unitid, teamid);
 	/* Remove the related correspondence relation record from the related
    * translation table. */
-  if (dart_segment_remove(seg_id) != DART_OK) {
+  if (dart_segment_free(seg_id) != DART_OK) {
     return DART_ERR_INVAL;
   }
 
-  dart_segment_dealloc(seg_id);
-
   return DART_OK;
 }
 
@@ -568,12 +566,10 @@ dart_team_memderegister(
     return DART_ERR_INVAL;
   }
   MPI_Win_detach(win, sub_mem);
-  if (dart_segment_remove(seg_id) != DART_OK) {
+  if (dart_segment_free(seg_id) != DART_OK) {
     return DART_ERR_INVAL;
   }
 
-  dart_segment_dealloc(seg_id);
-
   DART_LOG_DEBUG(
     "dart_team_memderegister: collective free, "
     "team unit %2d offset:%"PRIu64" gptr_unitid:%d" "across team %d",

diff --git a/dart-impl/mpi/src/dart_initialization.c b/dart-impl/mpi/src/dart_initialization.c
@@ -307,10 +307,6 @@ dart_ret_t dart_exit()
   MPI_Win_free(&team_data->window);
   MPI_Comm_free(&(team_data->sharedmem_comm));
 
-  /* <fuchsto>: Why calling dart_segment_clear twice? */
-/*
-  dart_segment_clear();
-*/
   dart_buddy_delete(dart_localpool);
 #if !defined(DART_MPI_DISABLE_SHARED_WINDOWS)
   free(team_data->sharedmem_tab);
@@ -319,12 +315,8 @@ dart_ret_t dart_exit()
 
 	dart_adapt_teamlist_destroy();
 
-  /* <fuchsto>: deactivated, currently segfaults when running
-   *            with 3 units:
-   */
-/*
-  dart_segment_clear();
-*/
+  dart_segment_fini();
+
   if (_init_by_dart) {
     DART_LOG_DEBUG("%2d: dart_exit: MPI_Finalize", unitid);
 		MPI_Finalize();

diff --git a/dart-impl/mpi/src/dart_segment.c b/dart-impl/mpi/src/dart_segment.c
@@ -191,37 +191,6 @@ dart_ret_t dart_segment_add_info(const dart_segment_info_t *item)
   return DART_OK;
 }
 
-/** <fuchsto>: what are semantics of
- *             - dart_segment_remove
- *             vs.
- *             - dart_segment_dealloc
- *             vs.
- *             - dart_segment_clear
- *             ?
- */
-dart_ret_t dart_segment_remove(int16_t seg_id)
-{
-  DART_LOG_DEBUG("dart_segment_remove() segid:%d", seg_id);
-
-  dart_segment_t *segment = get_segment(seg_id);
-  if (segment == NULL || segment->seg_info.seg_id != seg_id) {
-    DART_LOG_ERROR("Invalid segment ID %i", seg_id);
-    return DART_ERR_INVAL;
-  }
-  free(segment->seg_info.disp);
-  segment->seg_info.disp = NULL;
-#if !defined(DART_MPI_DISABLE_SHARED_WINDOWS)
-  if (segment->seg_info.baseptr) {
-    free(segment->seg_info.baseptr);
-    segment->seg_info.baseptr = NULL;
-  }
-#endif
-  memset(&segment->seg_info, 0, sizeof(segment->seg_info));
-
-  DART_LOG_DEBUG("dart_segment_remove > segid:%d", seg_id);
-  return DART_OK;
-}
-
 #if !defined(DART_MPI_DISABLE_SHARED_WINDOWS)
 dart_ret_t dart_segment_get_win(int16_t seg_id, MPI_Win * win)
 {
@@ -330,13 +299,27 @@ dart_ret_t dart_segment_get_size(
   return DART_OK;
 }
 
+static inline void free_segment_info(dart_segment_info_t *seg_info){
+  if (seg_info->disp != NULL) {
+    free(seg_info->disp);
+    seg_info->disp = NULL;
+  }
+#if !defined(DART_MPI_DISABLE_SHARED_WINDOWS)
+  if (seg_info->baseptr) {
+    free(seg_info->baseptr);
+    seg_info->baseptr = NULL;
+  }
+#endif
+  memset(seg_info, 0, sizeof(dart_segment_info_t));
+}
+
 /**
  * @brief Deallocates the segment identified by the segment ID.
  *
- * @return 0 on success.
- *        <0 if the segment was not found.
+ * @return DART_OK on success.
+ *         DART_ERR_INVAL if the segment was not found.
  */
-dart_ret_t dart_segment_dealloc(dart_segid_t segid)
+dart_ret_t dart_segment_free(dart_segid_t segid)
 {
   int slot = hash_segid(segid);
   dart_seghash_elem_t *pred = NULL;
@@ -354,15 +337,9 @@ dart_ret_t dart_segment_dealloc(dart_segid_t segid)
   while (elem != NULL) {
 
     if (elem->data.segid == segid) {
+      pred->next = elem->next;
+      free_segment_info(&elem->data.seg_info);
       elem->seg_id = DART_SEGMENT_INVALID;
-      free(elem->data.seg_info.disp);
-      elem->data.seg_info.disp = NULL;
-  #if !defined(DART_MPI_DISABLE_SHARED_WINDOWS)
-      if (elem->data.seg_info.baseptr) {
-        free(elem->data.seg_info.baseptr);
-        elem->data.seg_info.baseptr = NULL;
-      }
-  #endif
       if (freelist_head == NULL) {
         // make it the new freelist head
         freelist_head = elem;
@@ -372,7 +349,6 @@ dart_ret_t dart_segment_dealloc(dart_segid_t segid)
         elem->next = freelist_head->next;
         freelist_head->next = elem;
       }
-      pred->next = elem->next;
       return DART_OK;
     }
 
@@ -391,13 +367,7 @@ static void clear_segdata_list(dart_seghash_elem_t *listhead)
     dart_seghash_elem_t *tmp = elem;
     elem = tmp->next;
     tmp->next = NULL;
-    free(tmp->data.seg_info.disp);
-#if !defined(DART_MPI_DISABLE_SHARED_WINDOWS)
-    if (tmp->data.seg_info.baseptr) {
-      free(elem->data.seg_info.baseptr);
-      elem->data.seg_info.baseptr = NULL;
-    }
-#endif
+    free_segment_info(&tmp->data.seg_info);
     free(tmp);
   }
   listhead->next = NULL;
@@ -406,7 +376,7 @@ static void clear_segdata_list(dart_seghash_elem_t *listhead)
 /**
  * @brief Clear the segment data hash table.
  */
-dart_ret_t dart_segment_clear()
+dart_ret_t dart_segment_fini()
 {
   int i;
   // clear the hash table