Skip to content

Commit db38d4c

Browse files
soheilhysoheilhy
authored
Merge pull request grpc#17939 from vertextao/gpr_mu_cv-leak-test
Add ASAN-only leak detection for gpr_mu/cv and fix the newly caught leaks
2 parents b794777 + 7260eb6 commit db38d4c

File tree

14 files changed

+125
-12
lines changed

14 files changed

+125
-12
lines changed

‎include/grpc/impl/codegen/port_platform.h

+17-6
Original file line numberDiff line numberDiff line change
@@ -534,6 +534,14 @@ typedef unsigned __int64 uint64_t;
534534
#endif
535535
#endif /* GPR_HAS_ATTRIBUTE */
536536

537+
#ifndef GPR_HAS_FEATURE
538+
#ifdef __has_feature
539+
#define GPR_HAS_FEATURE(a) __has_feature(a)
540+
#else
541+
#define GPR_HAS_FEATURE(a) 0
542+
#endif
543+
#endif /* GPR_HAS_FEATURE */
544+
537545
#ifndef GPR_ATTRIBUTE_NOINLINE
538546
#if GPR_HAS_ATTRIBUTE(noinline) || (defined(__GNUC__) && !defined(__clang__))
539547
#define GPR_ATTRIBUTE_NOINLINE __attribute__((noinline))
@@ -556,11 +564,9 @@ typedef unsigned __int64 uint64_t;
556564
#endif /* GPR_ATTRIBUTE_WEAK */
557565

558566
#ifndef GPR_ATTRIBUTE_NO_TSAN /* (1) */
559-
#if defined(__has_feature)
560-
#if __has_feature(thread_sanitizer)
567+
#if GPR_HAS_FEATURE(thread_sanitizer)
561568
#define GPR_ATTRIBUTE_NO_TSAN __attribute__((no_sanitize("thread")))
562-
#endif /* __has_feature(thread_sanitizer) */
563-
#endif /* defined(__has_feature) */
569+
#endif /* GPR_HAS_FEATURE */
564570
#ifndef GPR_ATTRIBUTE_NO_TSAN /* (2) */
565571
#define GPR_ATTRIBUTE_NO_TSAN
566572
#endif /* GPR_ATTRIBUTE_NO_TSAN (2) */
@@ -569,10 +575,15 @@ typedef unsigned __int64 uint64_t;
569575
/* GRPC_TSAN_ENABLED will be defined, when compiled with thread sanitizer. */
570576
#if defined(__SANITIZE_THREAD__)
571577
#define GRPC_TSAN_ENABLED
572-
#elif defined(__has_feature)
573-
#if __has_feature(thread_sanitizer)
578+
#elif GPR_HAS_FEATURE(thread_sanitizer)
574579
#define GRPC_TSAN_ENABLED
575580
#endif
581+
582+
/* GRPC_ASAN_ENABLED will be defined, when compiled with address sanitizer. */
583+
#if defined(__SANITIZE_ADDRESS__)
584+
#define GRPC_ASAN_ENABLED
585+
#elif GPR_HAS_FEATURE(address_sanitizer)
586+
#define GRPC_ASAN_ENABLED
576587
#endif
577588

578589
/* GRPC_ALLOW_EXCEPTIONS should be 0 or 1 if exceptions are allowed or not */

‎include/grpc/impl/codegen/sync_posix.h

+18
Original file line numberDiff line numberDiff line change
@@ -25,8 +25,26 @@
2525

2626
#include <pthread.h>
2727

28+
#ifdef GRPC_ASAN_ENABLED
29+
/* The member |leak_checker| is used to check whether there is a memory leak
30+
* caused by upper layer logic that's missing the |gpr_xx_destroy| call
31+
* to the object before freeing it.
32+
* This issue was reported at https://github.com/grpc/grpc/issues/17563
33+
* and discussed at https://github.com/grpc/grpc/pull/17586
34+
*/
35+
typedef struct {
36+
pthread_mutex_t mutex;
37+
int* leak_checker;
38+
} gpr_mu;
39+
40+
typedef struct {
41+
pthread_cond_t cond_var;
42+
int* leak_checker;
43+
} gpr_cv;
44+
#else
2845
typedef pthread_mutex_t gpr_mu;
2946
typedef pthread_cond_t gpr_cv;
47+
#endif
3048
typedef pthread_once_t gpr_once;
3149

3250
#define GPR_ONCE_INIT PTHREAD_ONCE_INIT

‎src/core/ext/filters/max_age/max_age_filter.cc

+4-1
Original file line numberDiff line numberDiff line change
@@ -499,7 +499,10 @@ static grpc_error* init_channel_elem(grpc_channel_element* elem,
499499
}
500500

501501
/* Destructor for channel_data. */
502-
static void destroy_channel_elem(grpc_channel_element* elem) {}
502+
static void destroy_channel_elem(grpc_channel_element* elem) {
503+
channel_data* chand = static_cast<channel_data*>(elem->channel_data);
504+
gpr_mu_destroy(&chand->max_age_timer_mu);
505+
}
503506

504507
const grpc_channel_filter grpc_max_age_filter = {
505508
grpc_call_next_op,

‎src/core/ext/transport/inproc/inproc_transport.cc

+3
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,8 @@ struct shared_mu {
6464
gpr_ref_init(&refs, 2);
6565
}
6666

67+
~shared_mu() { gpr_mu_destroy(&mu); }
68+
6769
gpr_mu mu;
6870
gpr_refcount refs;
6971
};
@@ -83,6 +85,7 @@ struct inproc_transport {
8385
~inproc_transport() {
8486
grpc_connectivity_state_destroy(&connectivity);
8587
if (gpr_unref(&mu->refs)) {
88+
mu->~shared_mu();
8689
gpr_free(mu);
8790
}
8891
}

‎src/core/lib/gpr/sync_posix.cc

+65-4
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@
1818

1919
#include <grpc/support/port_platform.h>
2020

21+
#include <grpc/support/alloc.h>
22+
2123
#ifdef GPR_POSIX_SYNC
2224

2325
#include <errno.h>
@@ -72,27 +74,53 @@ gpr_atm gpr_counter_atm_add = 0;
7274
#endif
7375

7476
void gpr_mu_init(gpr_mu* mu) {
77+
#ifdef GRPC_ASAN_ENABLED
78+
GPR_ASSERT(pthread_mutex_init(&mu->mutex, nullptr) == 0);
79+
mu->leak_checker = static_cast<int*>(gpr_malloc(sizeof(*mu->leak_checker)));
80+
GPR_ASSERT(mu->leak_checker != nullptr);
81+
#else
7582
GPR_ASSERT(pthread_mutex_init(mu, nullptr) == 0);
83+
#endif
7684
}
7785

78-
void gpr_mu_destroy(gpr_mu* mu) { GPR_ASSERT(pthread_mutex_destroy(mu) == 0); }
86+
void gpr_mu_destroy(gpr_mu* mu) {
87+
#ifdef GRPC_ASAN_ENABLED
88+
GPR_ASSERT(pthread_mutex_destroy(&mu->mutex) == 0);
89+
gpr_free(mu->leak_checker);
90+
#else
91+
GPR_ASSERT(pthread_mutex_destroy(mu) == 0);
92+
#endif
93+
}
7994

8095
void gpr_mu_lock(gpr_mu* mu) {
8196
#ifdef GPR_LOW_LEVEL_COUNTERS
8297
GPR_ATM_INC_COUNTER(gpr_mu_locks);
8398
#endif
8499
GPR_TIMER_SCOPE("gpr_mu_lock", 0);
100+
#ifdef GRPC_ASAN_ENABLED
101+
GPR_ASSERT(pthread_mutex_lock(&mu->mutex) == 0);
102+
#else
85103
GPR_ASSERT(pthread_mutex_lock(mu) == 0);
104+
#endif
86105
}
87106

88107
void gpr_mu_unlock(gpr_mu* mu) {
89108
GPR_TIMER_SCOPE("gpr_mu_unlock", 0);
109+
#ifdef GRPC_ASAN_ENABLED
110+
GPR_ASSERT(pthread_mutex_unlock(&mu->mutex) == 0);
111+
#else
90112
GPR_ASSERT(pthread_mutex_unlock(mu) == 0);
113+
#endif
91114
}
92115

93116
int gpr_mu_trylock(gpr_mu* mu) {
94117
GPR_TIMER_SCOPE("gpr_mu_trylock", 0);
95-
int err = pthread_mutex_trylock(mu);
118+
int err = 0;
119+
#ifdef GRPC_ASAN_ENABLED
120+
err = pthread_mutex_trylock(&mu->mutex);
121+
#else
122+
err = pthread_mutex_trylock(mu);
123+
#endif
96124
GPR_ASSERT(err == 0 || err == EBUSY);
97125
return err == 0;
98126
}
@@ -105,10 +133,24 @@ void gpr_cv_init(gpr_cv* cv) {
105133
#if GPR_LINUX
106134
GPR_ASSERT(pthread_condattr_setclock(&attr, CLOCK_MONOTONIC) == 0);
107135
#endif // GPR_LINUX
136+
137+
#ifdef GRPC_ASAN_ENABLED
138+
GPR_ASSERT(pthread_cond_init(&cv->cond_var, &attr) == 0);
139+
cv->leak_checker = static_cast<int*>(gpr_malloc(sizeof(*cv->leak_checker)));
140+
GPR_ASSERT(cv->leak_checker != nullptr);
141+
#else
108142
GPR_ASSERT(pthread_cond_init(cv, &attr) == 0);
143+
#endif
109144
}
110145

111-
void gpr_cv_destroy(gpr_cv* cv) { GPR_ASSERT(pthread_cond_destroy(cv) == 0); }
146+
void gpr_cv_destroy(gpr_cv* cv) {
147+
#ifdef GRPC_ASAN_ENABLED
148+
GPR_ASSERT(pthread_cond_destroy(&cv->cond_var) == 0);
149+
gpr_free(cv->leak_checker);
150+
#else
151+
GPR_ASSERT(pthread_cond_destroy(cv) == 0);
152+
#endif
153+
}
112154

113155
// For debug of the timer manager crash only.
114156
// TODO (mxyan): remove after bug is fixed.
@@ -169,7 +211,11 @@ int gpr_cv_wait(gpr_cv* cv, gpr_mu* mu, gpr_timespec abs_deadline) {
169211
#endif
170212
if (gpr_time_cmp(abs_deadline, gpr_inf_future(abs_deadline.clock_type)) ==
171213
0) {
214+
#ifdef GRPC_ASAN_ENABLED
215+
err = pthread_cond_wait(&cv->cond_var, &mu->mutex);
216+
#else
172217
err = pthread_cond_wait(cv, mu);
218+
#endif
173219
} else {
174220
struct timespec abs_deadline_ts;
175221
#if GPR_LINUX
@@ -181,7 +227,12 @@ int gpr_cv_wait(gpr_cv* cv, gpr_mu* mu, gpr_timespec abs_deadline) {
181227
#endif // GPR_LINUX
182228
abs_deadline_ts.tv_sec = static_cast<time_t>(abs_deadline.tv_sec);
183229
abs_deadline_ts.tv_nsec = abs_deadline.tv_nsec;
230+
#ifdef GRPC_ASAN_ENABLED
231+
err = pthread_cond_timedwait(&cv->cond_var, &mu->mutex, &abs_deadline_ts);
232+
#else
184233
err = pthread_cond_timedwait(cv, mu, &abs_deadline_ts);
234+
#endif
235+
185236
#ifdef GRPC_DEBUG_TIMER_MANAGER
186237
// For debug of the timer manager crash only.
187238
// TODO (mxyan): remove after bug is fixed.
@@ -226,10 +277,20 @@ int gpr_cv_wait(gpr_cv* cv, gpr_mu* mu, gpr_timespec abs_deadline) {
226277
return err == ETIMEDOUT;
227278
}
228279

229-
void gpr_cv_signal(gpr_cv* cv) { GPR_ASSERT(pthread_cond_signal(cv) == 0); }
280+
void gpr_cv_signal(gpr_cv* cv) {
281+
#ifdef GRPC_ASAN_ENABLED
282+
GPR_ASSERT(pthread_cond_signal(&cv->cond_var) == 0);
283+
#else
284+
GPR_ASSERT(pthread_cond_signal(cv) == 0);
285+
#endif
286+
}
230287

231288
void gpr_cv_broadcast(gpr_cv* cv) {
289+
#ifdef GRPC_ASAN_ENABLED
290+
GPR_ASSERT(pthread_cond_broadcast(&cv->cond_var) == 0);
291+
#else
232292
GPR_ASSERT(pthread_cond_broadcast(cv) == 0);
293+
#endif
233294
}
234295

235296
/*----------------------------------------*/

‎src/core/lib/iomgr/ev_epollex_linux.cc

+1
Original file line numberDiff line numberDiff line change
@@ -612,6 +612,7 @@ static void pollable_unref(pollable* p, int line, const char* reason) {
612612
close(p->epfd);
613613
grpc_wakeup_fd_destroy(&p->wakeup);
614614
gpr_mu_destroy(&p->owner_orphan_mu);
615+
gpr_mu_destroy(&p->mu);
615616
gpr_free(p);
616617
}
617618
}

‎src/core/lib/iomgr/ev_poll_posix.cc

+4
Original file line numberDiff line numberDiff line change
@@ -1535,6 +1535,9 @@ static void cache_harvest_locked() {
15351535
gpr_inf_future(GPR_CLOCK_MONOTONIC));
15361536
}
15371537
args->poller_thd.Join();
1538+
gpr_cv_destroy(&args->trigger);
1539+
gpr_cv_destroy(&args->harvest);
1540+
gpr_cv_destroy(&args->join);
15381541
gpr_free(args);
15391542
}
15401543
}
@@ -1713,6 +1716,7 @@ static int cvfd_poll(struct pollfd* fds, nfds_t nfds, int timeout) {
17131716
}
17141717

17151718
gpr_free(fd_cvs);
1719+
gpr_cv_destroy(pollcv->cv);
17161720
gpr_free(pollcv);
17171721
if (result) {
17181722
decref_poll_result(result);

‎test/core/end2end/inproc_callback_test.cc

+4-1
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,10 @@ class ShutdownCallback : public grpc_experimental_completion_queue_functor {
6565
gpr_mu_init(&mu_);
6666
gpr_cv_init(&cv_);
6767
}
68-
~ShutdownCallback() {}
68+
~ShutdownCallback() {
69+
gpr_mu_destroy(&mu_);
70+
gpr_cv_destroy(&cv_);
71+
}
6972
static void StaticRun(grpc_experimental_completion_queue_functor* cb,
7073
int ok) {
7174
auto* callback = static_cast<ShutdownCallback*>(cb);

‎test/core/gpr/cpu_test.cc

+2
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,8 @@ static void cpu_test(void) {
140140
}
141141
fprintf(stderr, "] (%d/%d)\n", cores_seen, ct.ncores);
142142
fflush(stderr);
143+
gpr_mu_destroy(&ct.mu);
144+
gpr_cv_destroy(&ct.done_cv);
143145
gpr_free(ct.used);
144146
}
145147

‎test/core/gpr/mpscq_test.cc

+1
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,7 @@ static void test_mt_multipop(void) {
178178
for (auto& th : thds) {
179179
th.Join();
180180
}
181+
gpr_mu_destroy(&pa.mu);
181182
gpr_mpscq_destroy(&q);
182183
}
183184

‎test/core/gprpp/thd_test.cc

+2
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,8 @@ static void test1(void) {
7171
th.Join();
7272
}
7373
GPR_ASSERT(t.n == 0);
74+
gpr_mu_destroy(&t.mu);
75+
gpr_cv_destroy(&t.done_cv);
7476
}
7577

7678
static void thd_body2(void* v) {}

‎test/core/util/mock_endpoint.cc

+1
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,7 @@ static void me_destroy(grpc_endpoint* ep) {
8989
mock_endpoint* m = reinterpret_cast<mock_endpoint*>(ep);
9090
grpc_slice_buffer_destroy(&m->read_buffer);
9191
grpc_resource_user_unref(m->resource_user);
92+
gpr_mu_destroy(&m->mu);
9293
gpr_free(m);
9394
}
9495

‎test/cpp/microbenchmarks/bm_closure.cc

+2
Original file line numberDiff line numberDiff line change
@@ -183,6 +183,7 @@ static void BM_AcquireMutex(benchmark::State& state) {
183183
DoNothing(nullptr, GRPC_ERROR_NONE);
184184
gpr_mu_unlock(&mu);
185185
}
186+
gpr_mu_destroy(&mu);
186187

187188
track_counters.Finish(state);
188189
}
@@ -202,6 +203,7 @@ static void BM_TryAcquireMutex(benchmark::State& state) {
202203
abort();
203204
}
204205
}
206+
gpr_mu_destroy(&mu);
205207

206208
track_counters.Finish(state);
207209
}

‎test/cpp/util/grpc_tool.cc

+1
Original file line numberDiff line numberDiff line change
@@ -590,6 +590,7 @@ bool GrpcTool::CallMethod(int argc, const char** argv,
590590

591591
call.WritesDoneAndWait();
592592
read_thread.join();
593+
gpr_mu_destroy(&parser_mu);
593594

594595
std::multimap<grpc::string_ref, grpc::string_ref> server_trailing_metadata;
595596
Status status = call.Finish(&server_trailing_metadata);

0 commit comments

Comments
 (0)