[sr-dev] [kamailio/kamailio] htable race condition on expired keys (#1152)

[lazedo]

related to https://github.com/kamailio/kamailio/commit/be46742ebe162a2c48ee5ce27e09497b23b66ea5

the below patch solves it, but i'm not sure of the implications
why not let the expire routine do its job instead of checking expiration when adding a value ?
also moved the `ht_handle_expired_record` after the record is removed

```
diff --git a/src/modules/htable/ht_api.c b/src/modules/htable/ht_api.c
index 1bbd079..92a9421 100644
--- a/src/modules/htable/ht_api.c
+++ b/src/modules/htable/ht_api.c
@@ -674,29 +674,29 @@
 				&& strncmp(name->s, it->name.s, name->len)==0)
 		{
 			/* found */
-			if(now>0 && it->expire!=0 && it->expire<now) {
-				/* entry has expired */
-				ht_handle_expired_record(ht, it);
-
-				if(ht->flags==PV_VAL_INT) {
-					/* initval is integer, use it to create a fresh entry */
-					it->flags &= ~AVP_VAL_STR;
-					it->value.n = ht->initval.n;
-					/* increment will be done below */
-				} else {
-					/* delete expired entry */
-					if(it->prev==NULL)
-						ht->entries[idx].first = it->next;
-					else
-						it->prev->next = it->next;
-					if(it->next)
-						it->next->prev = it->prev;
-					ht->entries[idx].esize--;
-					if(mode) ht_slot_unlock(ht, idx);
-					ht_cell_free(it);
-					return NULL;
-				}
-			}
+//			if(now>0 && it->expire!=0 && it->expire<now) {
+//				/* entry has expired */
+//				ht_handle_expired_record(ht, it);
+//
+//				if(ht->flags==PV_VAL_INT) {
+//					/* initval is integer, use it to create a fresh entry */
+//					it->flags &= ~AVP_VAL_STR;
+//					it->value.n = ht->initval.n;
+//					/* increment will be done below */
+//				} else {
+//					/* delete expired entry */
+//					if(it->prev==NULL)
+//						ht->entries[idx].first = it->next;
+//					else
+//						it->prev->next = it->next;
+//					if(it->next)
+//						it->next->prev = it->prev;
+//					ht->entries[idx].esize--;
+//					if(mode) ht_slot_unlock(ht, idx);
+//					ht_cell_free(it);
+//					return NULL;
+//				}
+//			}
 			/* update value */
 			if(it->flags&AVP_VAL_STR)
 			{
@@ -803,21 +803,21 @@
 				&& strncmp(name->s, it->name.s, name->len)==0)
 		{
 			/* found */
-			if(ht->htexpire>0 && it->expire!=0 && it->expire<time(NULL)) {
-				/* entry has expired, delete it and return NULL */
-				ht_handle_expired_record(ht, it);
-
-				if(it->prev==NULL)
-					ht->entries[idx].first = it->next;
-				else
-					it->prev->next = it->next;
-				if(it->next)
-					it->next->prev = it->prev;
-				ht->entries[idx].esize--;
-				ht_slot_unlock(ht, idx);
-				ht_cell_free(it);
-				return NULL;
-			}
+//			if(ht->htexpire>0 && it->expire!=0 && it->expire<time(NULL)) {
+//				/* entry has expired, delete it and return NULL */
+//				ht_handle_expired_record(ht, it);
+//
+//				if(it->prev==NULL)
+//					ht->entries[idx].first = it->next;
+//				else
+//					it->prev->next = it->next;
+//				if(it->next)
+//					it->next->prev = it->prev;
+//				ht->entries[idx].esize--;
+//				ht_slot_unlock(ht, idx);
+//				ht_cell_free(it);
+//				return NULL;
+//			}
 			if(old!=NULL)
 			{
 				if(old->msize>=it->msize)
@@ -1056,7 +1056,6 @@
 					if(it->expire!=0 && it->expire<now)
 					{
 						/* expired */
-						ht_handle_expired_record(ht, it);
 						if(it->prev==NULL)
 							ht->entries[i].first = it->next;
 						else
@@ -1064,6 +1063,7 @@
 						if(it->next)
 							it->next->prev = it->prev;
 						ht->entries[i].esize--;
+						ht_handle_expired_record(ht, it);
 						ht_cell_free(it);
 					}
 					it = it0;
```


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/issues/1152
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.kamailio.org/pipermail/sr-dev/attachments/20170619/750c0810/attachment.html>