Skip to content

Commit 11a8e30

Browse files
vveljkovveljko
committed
Sanitize locks
Added a new lock for the timer list, instead of re-using the "watcher" lock, which is for the array of sockets to watch. Before this, we could get into a "reverse order of locking", which is a potential deadlock. It would not manifest, because we do everything on the same thread, but, it's better to make this "future safe". Also, more granular locking gives to more reponsive system. Again, in our case, since all is done on the same thread, it doesn't help, but, again, it will help if we have a thread shism in the future.
1 parent c182918 commit 11a8e30

File tree

2 files changed

+34
-25
lines changed

2 files changed

+34
-25
lines changed

‎openssl/pubnub_ntf_callback_windows.c‎

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,11 @@
3434
struct SocketWatcherData {
3535
_Guarded_by_(mutw) struct pbpal_poll_data* poll;
3636
CRITICAL_SECTION mutw;
37+
CRITICAL_SECTION timerlock;
3738
HANDLE thread_handle;
3839
DWORD thread_id;
3940
#if PUBNUB_TIMERS_API
40-
_Guarded_by_(mutw) pubnub_t* timer_head;
41+
_Guarded_by_(timerlock) pubnub_t* timer_head;
4142
#endif
4243
struct pbpal_ntf_callback_queue queue;
4344
};
@@ -94,9 +95,9 @@ void socket_watcher_thread(void* arg)
9495
GetSystemTimeAsFileTime(&current_time);
9596
elapsed = elapsed_ms(prev_time, current_time);
9697
if (elapsed > 0) {
97-
EnterCriticalSection(&m_watcher.mutw);
98+
EnterCriticalSection(&m_watcher.timerlock);
9899
pbntf_handle_timer_list(elapsed, &m_watcher.timer_head);
99-
LeaveCriticalSection(&m_watcher.mutw);
100+
LeaveCriticalSection(&m_watcher.timerlock);
100101

101102
prev_time = current_time;
102103
}
@@ -108,6 +109,7 @@ void socket_watcher_thread(void* arg)
108109
int pbntf_init(void)
109110
{
110111
InitializeCriticalSection(&m_watcher.mutw);
112+
InitializeCriticalSection(&m_watcher.timerlock);
111113

112114
m_watcher.poll = pbpal_ntf_callback_poller_init();
113115
if (NULL == m_watcher.poll) {
@@ -121,6 +123,7 @@ int pbntf_init(void)
121123
PUBNUB_LOG_ERROR("Failed to start the polling thread, error code: %d\n",
122124
errno);
123125
DeleteCriticalSection(&m_watcher.mutw);
126+
DeleteCriticalSection(&m_watcher.timerlock);
124127
pbpal_ntf_callback_queue_deinit(&m_watcher.queue);
125128
pbpal_ntf_callback_poller_deinit(&m_watcher.poll);
126129
return -1;
@@ -145,31 +148,35 @@ int pbntf_requeue_for_processing(pubnub_t* pb)
145148

146149
int pbntf_got_socket(pubnub_t* pb, pb_socket_t sockt)
147150
{
148-
EnterCriticalSection(&m_watcher.mutw);
149-
150151
PUBNUB_UNUSED(sockt);
151152

153+
EnterCriticalSection(&m_watcher.mutw);
152154
pbpal_ntf_callback_save_socket(m_watcher.poll, pb);
155+
LeaveCriticalSection(&m_watcher.mutw);
156+
153157
if (PUBNUB_TIMERS_API) {
158+
EnterCriticalSection(&m_watcher.timerlock);
154159
m_watcher.timer_head = pubnub_timer_list_add(m_watcher.timer_head, pb);
160+
LeaveCriticalSection(&m_watcher.timerlock);
155161
}
156-
LeaveCriticalSection(&m_watcher.mutw);
157162

158163
return +1;
159164
}
160165

161166

162167
void pbntf_lost_socket(pubnub_t* pb, pb_socket_t sockt)
163168
{
164-
EnterCriticalSection(&m_watcher.mutw);
165-
166169
PUBNUB_UNUSED(sockt);
167170

171+
EnterCriticalSection(&m_watcher.mutw);
168172
pbpal_ntf_callback_remove_socket(m_watcher.poll, pb);
169-
pbpal_remove_timer_safe(pb, &m_watcher.timer_head);
173+
LeaveCriticalSection(&m_watcher.mutw);
174+
170175
pbpal_ntf_callback_remove_from_queue(&m_watcher.queue, pb);
171176

172-
LeaveCriticalSection(&m_watcher.mutw);
177+
EnterCriticalSection(&m_watcher.timerlock);
178+
pbpal_remove_timer_safe(pb, &m_watcher.timer_head);
179+
LeaveCriticalSection(&m_watcher.timerlock);
173180
}
174181

175182

@@ -178,8 +185,6 @@ void pbntf_update_socket(pubnub_t* pb, pb_socket_t socket)
178185
PUBNUB_UNUSED(socket);
179186

180187
EnterCriticalSection(&m_watcher.mutw);
181-
182188
pbpal_ntf_callback_update_socket(m_watcher.poll, pb);
183-
184189
LeaveCriticalSection(&m_watcher.mutw);
185190
}

‎windows/pubnub_ntf_callback_windows.c‎

Lines changed: 17 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -34,10 +34,11 @@
3434
struct SocketWatcherData {
3535
_Guarded_by_(mutw) struct pbpal_poll_data* poll;
3636
CRITICAL_SECTION mutw;
37+
CRITICAL_SECTION timerlock;
3738
HANDLE thread_handle;
3839
DWORD thread_id;
3940
#if PUBNUB_TIMERS_API
40-
_Guarded_by_(mutw) pubnub_t* timer_head;
41+
_Guarded_by_(timerlock) pubnub_t* timer_head;
4142
#endif
4243
struct pbpal_ntf_callback_queue queue;
4344
};
@@ -94,9 +95,9 @@ void socket_watcher_thread(void* arg)
9495
GetSystemTimeAsFileTime(&current_time);
9596
elapsed = elapsed_ms(prev_time, current_time);
9697
if (elapsed > 0) {
97-
EnterCriticalSection(&m_watcher.mutw);
98+
EnterCriticalSection(&m_watcher.timerlock);
9899
pbntf_handle_timer_list(elapsed, &m_watcher.timer_head);
99-
LeaveCriticalSection(&m_watcher.mutw);
100+
LeaveCriticalSection(&m_watcher.timerlock);
100101

101102
prev_time = current_time;
102103
}
@@ -108,6 +109,7 @@ void socket_watcher_thread(void* arg)
108109
int pbntf_init(void)
109110
{
110111
InitializeCriticalSection(&m_watcher.mutw);
112+
InitializeCriticalSection(&m_watcher.timerlock);
111113

112114
m_watcher.poll = pbpal_ntf_callback_poller_init();
113115
if (NULL == m_watcher.poll) {
@@ -121,6 +123,7 @@ int pbntf_init(void)
121123
PUBNUB_LOG_ERROR("Failed to start the polling thread, error code: %d\n",
122124
errno);
123125
DeleteCriticalSection(&m_watcher.mutw);
126+
DeleteCriticalSection(&m_watcher.timerlock);
124127
pbpal_ntf_callback_queue_deinit(&m_watcher.queue);
125128
pbpal_ntf_callback_poller_deinit(&m_watcher.poll);
126129
return -1;
@@ -145,32 +148,35 @@ int pbntf_requeue_for_processing(pubnub_t* pb)
145148

146149
int pbntf_got_socket(pubnub_t* pb, pb_socket_t sockt)
147150
{
148-
EnterCriticalSection(&m_watcher.mutw);
149-
150151
PUBNUB_UNUSED(sockt);
151152

153+
EnterCriticalSection(&m_watcher.mutw);
152154
pbpal_ntf_callback_save_socket(m_watcher.poll, pb);
155+
LeaveCriticalSection(&m_watcher.mutw);
156+
153157
if (PUBNUB_TIMERS_API) {
158+
EnterCriticalSection(&m_watcher.timerlock);
154159
m_watcher.timer_head = pubnub_timer_list_add(m_watcher.timer_head, pb);
160+
LeaveCriticalSection(&m_watcher.timerlock);
155161
}
156162

157-
LeaveCriticalSection(&m_watcher.mutw);
158-
159163
return +1;
160164
}
161165

162166

163167
void pbntf_lost_socket(pubnub_t* pb, pb_socket_t sockt)
164168
{
165-
EnterCriticalSection(&m_watcher.mutw);
166-
167169
PUBNUB_UNUSED(sockt);
168170

171+
EnterCriticalSection(&m_watcher.mutw);
169172
pbpal_ntf_callback_remove_socket(m_watcher.poll, pb);
170-
pbpal_remove_timer_safe(pb, &m_watcher.timer_head);
173+
LeaveCriticalSection(&m_watcher.mutw);
174+
171175
pbpal_ntf_callback_remove_from_queue(&m_watcher.queue, pb);
172176

173-
LeaveCriticalSection(&m_watcher.mutw);
177+
EnterCriticalSection(&m_watcher.timerlock);
178+
pbpal_remove_timer_safe(pb, &m_watcher.timer_head);
179+
LeaveCriticalSection(&m_watcher.timerlock);
174180
}
175181

176182

@@ -179,8 +185,6 @@ void pbntf_update_socket(pubnub_t* pb, pb_socket_t socket)
179185
PUBNUB_UNUSED(socket);
180186

181187
EnterCriticalSection(&m_watcher.mutw);
182-
183188
pbpal_ntf_callback_update_socket(m_watcher.poll, pb);
184-
185189
LeaveCriticalSection(&m_watcher.mutw);
186190
}

0 commit comments

Comments
 (0)