From 6b68d1cfc879f38e1a8b2475c4aca2be47202f1b Mon Sep 17 00:00:00 2001 From: Nathan Hjelm Date: Wed, 8 Nov 2017 14:25:53 -0700 Subject: [PATCH] ompi/errhandler: make set/get actually thread safe The current versions of these functions have a fatal flaw. If a errhandler set and free call is made by another thread while the thread calling get is between the cmpset and retain then we will retain an invalid object. Fixing this by just using locking. This is not a critical path so this should be ok. Signed-off-by: Nathan Hjelm --- ompi/mpi/c/comm_get_errhandler.c | 16 +++++----------- ompi/mpi/c/comm_set_errhandler.c | 11 ++++++----- ompi/mpi/c/file_get_errhandler.c | 16 +++++----------- ompi/mpi/c/file_set_errhandler.c | 11 ++++++----- ompi/mpi/c/win_get_errhandler.c | 10 ++-------- ompi/mpi/c/win_set_errhandler.c | 11 ++++++----- 6 files changed, 30 insertions(+), 45 deletions(-) diff --git a/ompi/mpi/c/comm_get_errhandler.c b/ompi/mpi/c/comm_get_errhandler.c index afb18c44a1..d9aa00e690 100644 --- a/ompi/mpi/c/comm_get_errhandler.c +++ b/ompi/mpi/c/comm_get_errhandler.c @@ -13,7 +13,7 @@ * Copyright (c) 2007-2009 Cisco Systems, Inc. All rights reserved. * Copyright (c) 2015 Research Organization for Information Science * and Technology (RIST). All rights reserved. - * Copyright (c) 2016 Los Alamos National Security, LLC. All rights + * Copyright (c) 2016-2017 Los Alamos National Security, LLC. All rights * reserved. * $COPYRIGHT$ * @@ -43,8 +43,6 @@ static const char FUNC_NAME[] = "MPI_Comm_get_errhandler"; int MPI_Comm_get_errhandler(MPI_Comm comm, MPI_Errhandler *errhandler) { - MPI_Errhandler tmp; - /* Error checking */ MEMCHECKER( memchecker_comm(comm); @@ -65,16 +63,12 @@ int MPI_Comm_get_errhandler(MPI_Comm comm, MPI_Errhandler *errhandler) } } - /* On 64 bits environments we have to make sure the reading of the - error_handler became atomic. */ - do { - tmp = comm->error_handler; - } while (!OPAL_ATOMIC_BOOL_CMPSET_PTR(&(comm->error_handler), tmp, tmp)); - + opal_mutex_lock (&comm->c_lock); /* Retain the errhandler, corresponding to object refcount decrease in errhandler_free.c. */ - *errhandler = tmp; - OBJ_RETAIN(tmp); + OBJ_RETAIN(comm->error_handler); + *errhandler = comm->error_handler; + opal_mutex_unlock (&comm->c_lock); /* All done */ diff --git a/ompi/mpi/c/comm_set_errhandler.c b/ompi/mpi/c/comm_set_errhandler.c index 8091495ea5..4c3a8a31ea 100644 --- a/ompi/mpi/c/comm_set_errhandler.c +++ b/ompi/mpi/c/comm_set_errhandler.c @@ -12,7 +12,7 @@ * All rights reserved. * Copyright (c) 2015 Research Organization for Information Science * and Technology (RIST). All rights reserved. - * Copyright (c) 2016 Los Alamos National Security, LLC. All rights + * Copyright (c) 2016-2017 Los Alamos National Security, LLC. All rights * reserved. * $COPYRIGHT$ * @@ -69,11 +69,12 @@ int MPI_Comm_set_errhandler(MPI_Comm comm, MPI_Errhandler errhandler) /* Prepare the new error handler */ OBJ_RETAIN(errhandler); - /* Ditch the old errhandler, and decrement its refcount. On 64 - bits environments we have to make sure the reading of the - error_handler became atomic. */ - tmp = OPAL_ATOMIC_SWAP_PTR(&comm->error_handler, errhandler); + opal_mutex_lock (&comm->c_lock); + /* Ditch the old errhandler, and decrement its refcount. */ + tmp = comm->error_handler; + comm->error_handler = errhandler; OBJ_RELEASE(tmp); + opal_mutex_unlock (&comm->c_lock); /* All done */ return MPI_SUCCESS; diff --git a/ompi/mpi/c/file_get_errhandler.c b/ompi/mpi/c/file_get_errhandler.c index ef8b300075..07236b3490 100644 --- a/ompi/mpi/c/file_get_errhandler.c +++ b/ompi/mpi/c/file_get_errhandler.c @@ -13,7 +13,7 @@ * Copyright (c) 2008 Cisco Systems, Inc. All rights reserved. * Copyright (c) 2015 Research Organization for Information Science * and Technology (RIST). All rights reserved. - * Copyright (c) 2016 Los Alamos National Security, LLC. All rights + * Copyright (c) 2016-2017 Los Alamos National Security, LLC. All rights * reserved. * $COPYRIGHT$ * @@ -42,8 +42,6 @@ static const char FUNC_NAME[] = "MPI_File_get_errhandler"; int MPI_File_get_errhandler( MPI_File file, MPI_Errhandler *errhandler) { - MPI_Errhandler tmp; - OPAL_CR_NOOP_PROGRESS(); /* Error checking */ @@ -64,16 +62,12 @@ int MPI_File_get_errhandler( MPI_File file, MPI_Errhandler *errhandler) } } - /* On 64 bits environments we have to make sure the reading of the - error_handler became atomic. */ - do { - tmp = file->error_handler; - } while (!OPAL_ATOMIC_BOOL_CMPSET_PTR(&(file->error_handler), tmp, tmp)); - + opal_mutex_lock (&file->f_mutex); /* Retain the errhandler, corresponding to object refcount decrease in errhandler_free.c. */ - *errhandler = tmp; - OBJ_RETAIN(tmp); + *errhandler = file->error_handler; + OBJ_RETAIN(file->error_handler); + opal_mutex_unlock (&file->f_mutex); /* All done */ diff --git a/ompi/mpi/c/file_set_errhandler.c b/ompi/mpi/c/file_set_errhandler.c index ebd4c9b132..788893aa00 100644 --- a/ompi/mpi/c/file_set_errhandler.c +++ b/ompi/mpi/c/file_set_errhandler.c @@ -13,7 +13,7 @@ * Copyright (c) 2008 Cisco Systems, Inc. All rights reserved. * Copyright (c) 2015 Research Organization for Information Science * and Technology (RIST). All rights reserved. - * Copyright (c) 2016 Los Alamos National Security, LLC. All rights + * Copyright (c) 2016-2017 Los Alamos National Security, LLC. All rights * reserved. * $COPYRIGHT$ * @@ -68,11 +68,12 @@ int MPI_File_set_errhandler( MPI_File file, MPI_Errhandler errhandler) /* Prepare the new error handler */ OBJ_RETAIN(errhandler); - /* Ditch the old errhandler, and decrement its refcount. On 64 - bits environments we have to make sure the reading of the - error_handler became atomic. */ - tmp = OPAL_ATOMIC_SWAP_PTR (&file->error_handler, errhandler); + opal_mutex_lock (&file->f_mutex); + /* Ditch the old errhandler, and decrement its refcount. */ + tmp = file->error_handler; + file->error_handler = errhandler; OBJ_RELEASE(tmp); + opal_mutex_unlock (&file->f_mutex); /* All done */ return MPI_SUCCESS; diff --git a/ompi/mpi/c/win_get_errhandler.c b/ompi/mpi/c/win_get_errhandler.c index bf088ad7b4..23a1c6f50f 100644 --- a/ompi/mpi/c/win_get_errhandler.c +++ b/ompi/mpi/c/win_get_errhandler.c @@ -42,8 +42,6 @@ static const char FUNC_NAME[] = "MPI_Win_get_errhandler"; int MPI_Win_get_errhandler(MPI_Win win, MPI_Errhandler *errhandler) { - MPI_Errhandler tmp; - OPAL_CR_NOOP_PROGRESS(); if (MPI_PARAM_CHECK) { @@ -57,16 +55,12 @@ int MPI_Win_get_errhandler(MPI_Win win, MPI_Errhandler *errhandler) } } - /* On 64 bits environments we have to make sure the reading of the - error_handler became atomic. */ - do { - tmp = win->error_handler; - } while (!OPAL_ATOMIC_BOOL_CMPSET_PTR(&(win->error_handler), tmp, tmp)); - + opal_mutex_lock (&win->w_lock); /* Retain the errhandler, corresponding to object refcount decrease in errhandler_free.c. */ OBJ_RETAIN(win->error_handler); *errhandler = win->error_handler; + opal_mutex_unlock (&win->w_lock); /* All done */ return MPI_SUCCESS; diff --git a/ompi/mpi/c/win_set_errhandler.c b/ompi/mpi/c/win_set_errhandler.c index d5e87cff77..b630cad3d5 100644 --- a/ompi/mpi/c/win_set_errhandler.c +++ b/ompi/mpi/c/win_set_errhandler.c @@ -13,7 +13,7 @@ * Copyright (c) 2008-2009 Cisco Systems, Inc. All rights reserved. * Copyright (c) 2015 Research Organization for Information Science * and Technology (RIST). All rights reserved. - * Copyright (c) 2016 Los Alamos National Security, LLC. All rights + * Copyright (c) 2016-2017 Los Alamos National Security, LLC. All rights * reserved. * $COPYRIGHT$ * @@ -63,11 +63,12 @@ int MPI_Win_set_errhandler(MPI_Win win, MPI_Errhandler errhandler) /* Prepare the new error handler */ OBJ_RETAIN(errhandler); - /* Ditch the old errhandler, and decrement its refcount. On 64 - bits environments we have to make sure the reading of the - error_handler became atomic. */ - tmp = OPAL_ATOMIC_SWAP_PTR(&win->error_handler, errhandler); + opal_mutex_lock (&win->w_lock); + /* Ditch the old errhandler, and decrement its refcount. */ + tmp = win->error_handler; + win->error_handler = errhandler; OBJ_RELEASE(tmp); + opal_mutex_unlock (&win->w_lock); /* All done */ return MPI_SUCCESS;