Browse code

keepalive: prevent race condition when deleting a destination

- Added a lock to ka_dest type, so we get it when we run callbacks that may be associated to an OPTIONS response
- Same lock is used to not remove destinations that are running callbacks
- Now find destinations consider owner and uri

Nacho Garcia Segovia authored on 14/09/2020 11:18:37
Showing 3 changed files
... ...
@@ -77,6 +77,7 @@ typedef struct _ka_dest
77 77
 	unsigned short int port;   /*!< Port of the URI */
78 78
 	unsigned short int proto;  /*!< Protocol of the URI */
79 79
 	struct timer_ln *timer;    /*!< Timer firing the OPTIONS test */
80
+	gen_lock_t lock;		   /*!< Lock of this record to prevent being removed while running */
80 81
 	struct _ka_dest *next;
81 82
 } ka_dest_t;
82 83
 
... ...
@@ -112,6 +112,9 @@ int ka_add_dest(str *uri, str *owner, int flags, int ping_interval,
112 112
 	dest->response_clb = response_clb;
113 113
 	dest->user_attr = user_attr;
114 114
 	dest->ping_interval = MS_TO_TICKS((ping_interval == 0 ? ka_ping_interval : ping_interval) * 1000) ;
115
+	if (lock_init(&dest->lock)==0){
116
+		LM_ERR("failed initializing Lock \n");
117
+	}
115 118
 
116 119
     dest->timer = timer_alloc();
117 120
 	if (dest->timer == NULL) {
... ...
@@ -177,7 +180,7 @@ ka_state ka_destination_state(str *destination)
177 180
 * @result 1 successful  , -1 fail
178 181
 */
179 182
 int ka_del_destination(str *uri, str *owner){
180
-
183
+	LM_DBG("removing destination: %.*s\n", uri->len, uri->s);
181 184
 	ka_dest_t *target=0,*head=0;
182 185
 	ka_lock_destination_list();
183 186
 
... ...
@@ -190,15 +193,14 @@ int ka_del_destination(str *uri, str *owner){
190 193
 		LM_ERR("Couldn't find destination \r\n");
191 194
 		goto err;
192 195
 	}
193
-
196
+	lock_get(&target->lock);
194 197
 	if(!head){
195 198
 		LM_DBG("There isn't any head so maybe it is first \r\n");
196 199
 		ka_destinations_list->first = target->next;
197
-		free_destination(target);
198
-		ka_unlock_destination_list();
199
-		return 1;
200
+	} else {
201
+		head->next = target->next;
200 202
 	}
201
-	head->next = target->next;
203
+	lock_release(&target->lock);
202 204
 	free_destination(target);
203 205
 	ka_unlock_destination_list();
204 206
 	return 1;
... ...
@@ -211,7 +213,7 @@ err:
211 213
 * @abstract find given destination uri address in destination_list stack
212 214
 *           don't forget to add lock via ka_lock_destination_list to prevent crashes
213 215
 * @param *uri given uri
214
-* @param *owner given owner name, not using now
216
+* @param *owner given owner name
215 217
 * @param **target searched address in stack
216 218
 * @param **head which points target
217 219
 *	*
... ...
@@ -226,10 +228,7 @@ int ka_find_destination(str *uri, str *owner, ka_dest_t **target, ka_dest_t **he
226 228
 		if(!dest)
227 229
 			break;
228 230
 
229
-		if(uri->len!=dest->uri.len)
230
-			continue;
231
-
232
-		if(memcmp(dest->uri.s , uri->s , uri->len>dest->uri.len?dest->uri.len : uri->len)==0){
231
+		if (STR_EQ(*uri, dest->uri) && STR_EQ(*owner, dest->owner)){
233 232
 			*head = temp;
234 233
 			*target = dest;
235 234
 			LM_DBG("destination is found [target : %p] [head : %p] \r\n",target,temp);
... ...
@@ -243,6 +242,7 @@ int ka_find_destination(str *uri, str *owner, ka_dest_t **target, ka_dest_t **he
243 242
 
244 243
 /*!
245 244
 * @function ka_find_destination_by_uuid
245
+*			don't forget to add lock via ka_lock_destination_list to prevent crashes
246 246
 *
247 247
 * @param *uuid uuid of ka_dest record
248 248
 * @param **target searched address in stack
... ...
@@ -281,7 +281,7 @@ int ka_find_destination_by_uuid(str uuid, ka_dest_t **target, ka_dest_t **head){
281 281
 * @result 1 successful  , -1 fail
282 282
 */
283 283
 int free_destination(ka_dest_t *dest){
284
-
284
+	
285 285
 	if(dest){
286 286
         if(timer_del(dest->timer) < 0){
287 287
           LM_ERR("failed to remove timer for destination <%.*s>\n", dest->uri.len, dest->uri.s);
... ...
@@ -289,6 +289,7 @@ int free_destination(ka_dest_t *dest){
289 289
         }
290 290
 
291 291
         timer_free(dest->timer);
292
+		lock_destroy(dest->lock);
292 293
 		if(dest->uri.s)
293 294
 			shm_free(dest->uri.s);
294 295
 
... ...
@@ -103,13 +103,22 @@ static void ka_options_callback(
103 103
 
104 104
 	str *uuid = (str *)(*ps->param);
105 105
 
106
+	LM_DBG("ka_options_callback with uuid: %.*s\n", uuid->len, uuid->s);
107
+
106 108
 	// Retrieve ka_dest by uuid from destination list
109
+	ka_lock_destination_list();
107 110
 	ka_dest_t *ka_dest=0,*hollow=0;
108 111
 	if (!ka_find_destination_by_uuid(*uuid, &ka_dest, &hollow)) {
109 112
 		LM_ERR("Couldn't find destination \r\n");
113
+		shm_free(uuid->s);
114
+		shm_free(uuid);
115
+		ka_unlock_destination_list();
110 116
 		return;
111 117
 	}
118
+	lock_get(&ka_dest->lock); // Lock record so we prevent to be removed in the meantime
119
+	shm_free(uuid->s);
112 120
 	shm_free(uuid);
121
+	ka_unlock_destination_list();
113 122
 
114 123
 	uri.s = t->to.s + 5;
115 124
 	uri.len = t->to.len - 8;
... ...
@@ -141,6 +150,7 @@ static void ka_options_callback(
141 150
 	if(ka_dest->response_clb != NULL) {
142 151
 		ka_dest->response_clb(&ka_dest->uri, ps, ka_dest->user_attr);
143 152
 	}
153
+	lock_release(&ka_dest->lock);
144 154
 }
145 155
 
146 156
 /*