From 990d99e650e1473826a876c280510e0d66f308c1 Mon Sep 17 00:00:00 2001 From: David Beer Date: Wed, 23 Apr 2014 11:47:39 -0600 Subject: [PATCH 1/4] TRQ-2547. Fix a valgrind issue with id_map's destructor. Destructors are called on global objects once the main thread exits, causing invalid reads and potentially crashes for threads that are still finishing work at that point. Now the destructor does nothing to prevent problems. --- src/include/id_map.hpp | 1 + src/server/id_map.cpp | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/src/include/id_map.hpp b/src/include/id_map.hpp index 381553a124..58d71c2740 100644 --- a/src/include/id_map.hpp +++ b/src/include/id_map.hpp @@ -91,6 +91,7 @@ class id_map public: id_map(); + ~id_map(); id_map(const id_map &other); int get_new_id(const char *name); int get_id(const char *name); diff --git a/src/server/id_map.cpp b/src/server/id_map.cpp index c25a49a62c..737889ad59 100644 --- a/src/server/id_map.cpp +++ b/src/server/id_map.cpp @@ -154,6 +154,14 @@ const char *id_map::get_name( } +id_map::~id_map() + { + // leave the destructor blank. C++ destroys globally constructed + // objects when the main thread exits, and this class is only destroyed + // at shutdown, so we don't care about the memory usage at that point. + } + + id_map::id_map() : counter(0) From a867277d41feb02ceae86366f374fb99dbdef97d Mon Sep 17 00:00:00 2001 From: David Beer Date: Wed, 23 Apr 2014 15:06:18 -0600 Subject: [PATCH 2/4] Fix some scaffolding files. Conflicts: src/server/test/exiting_jobs/scaffolding.c src/server/test/job_container/scaffolding.c src/server/test/job_func/scaffolding.c src/server/test/job_recov/scaffolding.c src/server/test/req_jobobit/scaffolding.c src/server/test/req_quejob/scaffolding.c src/server/test/track_alps_reservations/scaffolding.c --- src/server/test/exiting_jobs/scaffolding.c | 1 + src/server/test/job_container/scaffolding.c | 1 + src/server/test/job_func/scaffolding.c | 3 ++- src/server/test/job_recov/scaffolding.c | 1 + src/server/test/node_func/scaffolding.c | 5 +++++ src/server/test/node_manager/scaffolding.c | 5 +++++ src/server/test/req_jobobit/scaffolding.c | 2 +- src/server/test/req_quejob/scaffolding.c | 1 - 8 files changed, 16 insertions(+), 3 deletions(-) diff --git a/src/server/test/exiting_jobs/scaffolding.c b/src/server/test/exiting_jobs/scaffolding.c index b1145d8c9e..2e662a88b4 100644 --- a/src/server/test/exiting_jobs/scaffolding.c +++ b/src/server/test/exiting_jobs/scaffolding.c @@ -41,3 +41,4 @@ void on_job_exit(batch_request *preq, char *jobid) {} void force_purge_work(job *pjob) {} void log_event(int eventtype, int objclass, const char *objname, const char *text) {} + diff --git a/src/server/test/job_container/scaffolding.c b/src/server/test/job_container/scaffolding.c index 4475f9e827..63bbef6391 100644 --- a/src/server/test/job_container/scaffolding.c +++ b/src/server/test/job_container/scaffolding.c @@ -59,3 +59,4 @@ job *job_alloc(void) return(pj); } + diff --git a/src/server/test/job_func/scaffolding.c b/src/server/test/job_func/scaffolding.c index b6b20611ad..7508c4b484 100644 --- a/src/server/test/job_func/scaffolding.c +++ b/src/server/test/job_func/scaffolding.c @@ -3,6 +3,7 @@ #include /* fprintf */ #include #include +#include #include "pbs_ifl.h" /* MAXPATHLEN, PBS_MAXSERVERNAME */ #include "server.h" /* server, NO_BUFFER_SPACE */ @@ -13,7 +14,6 @@ #include "batch_request.h" /* batch_request */ #include "work_task.h" /* all_tasks */ #include "array.h" /* ArrayEventsEnum */ -#include /* This section is for manipulting function return values */ #include "test_job_func.h" /* *_SUITE */ @@ -591,3 +591,4 @@ job *find_job_by_array( return(pj); } /* END find_job_by_array() */ + diff --git a/src/server/test/job_recov/scaffolding.c b/src/server/test/job_recov/scaffolding.c index efc6a968f0..0f72822d44 100644 --- a/src/server/test/job_recov/scaffolding.c +++ b/src/server/test/job_recov/scaffolding.c @@ -330,3 +330,4 @@ job *find_job_by_array(all_jobs *aj, char *job_id, int get_subjob, bool locked) { return(NULL); } + diff --git a/src/server/test/node_func/scaffolding.c b/src/server/test/node_func/scaffolding.c index 32cf55f307..21bae83e08 100644 --- a/src/server/test/node_func/scaffolding.c +++ b/src/server/test/node_func/scaffolding.c @@ -335,6 +335,11 @@ int id_map::get_new_id(char const *name) return(id++); } +id_map::~id_map() + { + } + + id_map node_mapper; struct pbsnode *tfind_addr( diff --git a/src/server/test/node_manager/scaffolding.c b/src/server/test/node_manager/scaffolding.c index 73f77c4dbc..a5c53ee157 100644 --- a/src/server/test/node_manager/scaffolding.c +++ b/src/server/test/node_manager/scaffolding.c @@ -546,6 +546,11 @@ const char *id_map::get_name(int id) return(strdup(buf)); } +id_map::~id_map() + { + } + + id_map node_mapper; #ifdef CAN_TIME diff --git a/src/server/test/req_jobobit/scaffolding.c b/src/server/test/req_jobobit/scaffolding.c index 1dc1617a81..8c968796f5 100644 --- a/src/server/test/req_jobobit/scaffolding.c +++ b/src/server/test/req_jobobit/scaffolding.c @@ -21,7 +21,6 @@ #include "queue.h" /* pbs_queue */ - const char *msg_momnoexec2 = "Job cannot be executed\nSee job standard error file"; const char *msg_job_end_sig = "Terminated on signal %d"; const char *msg_obitnojob = "Job Obit notice received from %s has error %d"; @@ -325,3 +324,4 @@ void log_record(int eventtype, int objclass, const char *objname, const char *te void account_jobend(job *pjob, char *used) {} void update_array_values(job_array *pa, int old_state, enum ArrayEventsEnum event, char *job_id, long job_atr_hold, int job_exit_status) {} + diff --git a/src/server/test/req_quejob/scaffolding.c b/src/server/test/req_quejob/scaffolding.c index 9b083cce7a..a4ef95944a 100644 --- a/src/server/test/req_quejob/scaffolding.c +++ b/src/server/test/req_quejob/scaffolding.c @@ -411,4 +411,3 @@ job *find_job_by_array(all_jobs *aj, char *jobid, int get_subjob, bool locked) } - From d45bd7d033ce60f60e93f5c0b1fda80983bb9f64 Mon Sep 17 00:00:00 2001 From: bdaw Date: Wed, 23 Apr 2014 15:50:48 -0600 Subject: [PATCH 3/4] trq-2547 cleaned up some valgrind issues. --- src/include/id_map.hpp | 4 ++-- src/server/id_map.cpp | 18 ++++++++++-------- src/server/test/node_func/scaffolding.c | 1 + src/server/test/node_manager/scaffolding.c | 1 + 4 files changed, 14 insertions(+), 10 deletions(-) diff --git a/src/include/id_map.hpp b/src/include/id_map.hpp index 58d71c2740..54d206d8cc 100644 --- a/src/include/id_map.hpp +++ b/src/include/id_map.hpp @@ -84,8 +84,8 @@ class id_map { - std::map str_map; - std::vector names; + std::map *str_map; + std::vector *names; pthread_mutex_t mutex; int counter; diff --git a/src/server/id_map.cpp b/src/server/id_map.cpp index 737889ad59..8ab4575bf6 100644 --- a/src/server/id_map.cpp +++ b/src/server/id_map.cpp @@ -93,14 +93,14 @@ int id_map::get_new_id( try { - id = this->str_map.at(nname); + id = this->str_map->at(nname); } catch (...) { id = this->counter++; std::pair p1(nname, id); - this->str_map.insert(p1); - this->names.push_back(nname); + this->str_map->insert(p1); + this->names->push_back(nname); } pthread_mutex_unlock(&this->mutex); @@ -121,7 +121,7 @@ int id_map::get_id( try { std::string nname(name); - id = this->str_map.at(nname); + id = this->str_map->at(nname); } catch (...) { @@ -142,7 +142,7 @@ const char *id_map::get_name( pthread_mutex_lock(&this->mutex); try { - std::string const &nname = this->names.at(id); + std::string const &nname = this->names->at(id); name = nname.c_str(); } catch (...) @@ -164,16 +164,18 @@ id_map::~id_map() id_map::id_map() : counter(0) - { + str_map = new std::map(); + names = new std::vector(); pthread_mutex_init(&mutex, NULL); } -id_map::id_map(const id_map &other) : counter(other.counter), names(other.names), str_map(other.str_map) - +id_map::id_map(const id_map &other) : counter(other.counter) { + str_map = new std::map(*other.str_map); + names = new std::vector(*other.names); pthread_mutex_init(&mutex, NULL); } diff --git a/src/server/test/node_func/scaffolding.c b/src/server/test/node_func/scaffolding.c index 21bae83e08..03e65f0908 100644 --- a/src/server/test/node_func/scaffolding.c +++ b/src/server/test/node_func/scaffolding.c @@ -292,6 +292,7 @@ struct pbsnode *create_alps_subnode(struct pbsnode *parent, const char *node_id) } id_map::id_map() : counter(0) {} +id_map::~id_map(){} int id_map::get_id(char const *name) { diff --git a/src/server/test/node_manager/scaffolding.c b/src/server/test/node_manager/scaffolding.c index a5c53ee157..5fab1da1a6 100644 --- a/src/server/test/node_manager/scaffolding.c +++ b/src/server/test/node_manager/scaffolding.c @@ -526,6 +526,7 @@ int decode_resc( id_map::id_map() : counter(0) {} +id_map::~id_map(){} int id_map::get_id(const char *name) { From 238a6ac8093c586c653aa1aa43bfa0748343f3eb Mon Sep 17 00:00:00 2001 From: bdaw Date: Wed, 23 Apr 2014 15:59:33 -0600 Subject: [PATCH 4/4] Fixed unit tests. --- src/server/test/node_func/scaffolding.c | 1 - src/server/test/node_manager/scaffolding.c | 1 - 2 files changed, 2 deletions(-) diff --git a/src/server/test/node_func/scaffolding.c b/src/server/test/node_func/scaffolding.c index 03e65f0908..21bae83e08 100644 --- a/src/server/test/node_func/scaffolding.c +++ b/src/server/test/node_func/scaffolding.c @@ -292,7 +292,6 @@ struct pbsnode *create_alps_subnode(struct pbsnode *parent, const char *node_id) } id_map::id_map() : counter(0) {} -id_map::~id_map(){} int id_map::get_id(char const *name) { diff --git a/src/server/test/node_manager/scaffolding.c b/src/server/test/node_manager/scaffolding.c index 5fab1da1a6..a5c53ee157 100644 --- a/src/server/test/node_manager/scaffolding.c +++ b/src/server/test/node_manager/scaffolding.c @@ -526,7 +526,6 @@ int decode_resc( id_map::id_map() : counter(0) {} -id_map::~id_map(){} int id_map::get_id(const char *name) {