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

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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

Daniel-Constantin Mierla-11

related to be46742

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, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"DESCRIPTION","message":"htable race condition on expired keys (#1152)"}],"action":{"name":"View Issue","url":"https://github.com/kamailio/kamailio/issues/1152"}}}</script>
_______________________________________________
Kamailio (SER) - Development Mailing List
[hidden email]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [kamailio/kamailio] htable race condition on expired keys (#1152)

Daniel-Constantin Mierla-11

core dump attached
core-dump.zip


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"PERSON","message":"@lazedo in #1152: core dump attached\r\n[core-dump.zip](https://github.com/kamailio/kamailio/files/1084801/core-dump.zip)\r\n\r\n"}],"action":{"name":"View Issue","url":"https://github.com/kamailio/kamailio/issues/1152#issuecomment-309420202"}}}</script>
_______________________________________________
Kamailio (SER) - Development Mailing List
[hidden email]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [kamailio/kamailio] htable race condition on expired keys (#1152)

Daniel-Constantin Mierla-11
In reply to this post by Daniel-Constantin Mierla-11

Do not do patches that comment a lot of code because it is hard to detect if all code was commented or some were removed/added/changed. Just remove the code that you think is broken or eventually add it in between #if 0 ... #endif.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"PERSON","message":"@miconda in #1152: Do not do patches that comment a lot of code because it is hard to detect if all code was commented or some were removed/added/changed. Just remove the code that you think is broken or eventually add it in between `#if 0 ... #endif`."}],"action":{"name":"View Issue","url":"https://github.com/kamailio/kamailio/issues/1152#issuecomment-309421286"}}}</script>
_______________________________________________
Kamailio (SER) - Development Mailing List
[hidden email]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [kamailio/kamailio] htable race condition on expired keys (#1152)

Daniel-Constantin Mierla-11
In reply to this post by Daniel-Constantin Mierla-11

sorry about that, patch updated

diff --git a/src/modules/htable/ht_api.c b/src/modules/htable/ht_api.c
index 1bbd079..b5872d0 100644
--- a/src/modules/htable/ht_api.c
+++ b/src/modules/htable/ht_api.c
@@ -674,6 +674,7 @@
 				&& strncmp(name->s, it->name.s, name->len)==0)
 		{
 			/* found */
+#if 0
 			if(now>0 && it->expire!=0 && it->expire<now) {
 				/* entry has expired */
 				ht_handle_expired_record(ht, it);
@@ -697,6 +698,7 @@
 					return NULL;
 				}
 			}
+#endif
 			/* update value */
 			if(it->flags&AVP_VAL_STR)
 			{
@@ -803,6 +805,7 @@
 				&& strncmp(name->s, it->name.s, name->len)==0)
 		{
 			/* found */
+#if 0
 			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);
@@ -818,6 +821,7 @@
 				ht_cell_free(it);
 				return NULL;
 			}
+#endif
 			if(old!=NULL)
 			{
 				if(old->msize>=it->msize)
@@ -1056,7 +1060,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 +1067,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, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"PERSON","message":"@lazedo in #1152: sorry about that, patch updated\r\n\r\n```\r\ndiff --git a/src/modules/htable/ht_api.c b/src/modules/htable/ht_api.c\r\nindex 1bbd079..b5872d0 100644\r\n--- a/src/modules/htable/ht_api.c\r\n+++ b/src/modules/htable/ht_api.c\r\n@@ -674,6 +674,7 @@\r\n \t\t\t\t\u0026\u0026 strncmp(name-\u003es, it-\u003ename.s, name-\u003elen)==0)\r\n \t\t{\r\n \t\t\t/* found */\r\n+#if 0\r\n \t\t\tif(now\u003e0 \u0026\u0026 it-\u003eexpire!=0 \u0026\u0026 it-\u003eexpire\u003cnow) {\r\n \t\t\t\t/* entry has expired */\r\n \t\t\t\tht_handle_expired_record(ht, it);\r\n@@ -697,6 +698,7 @@\r\n \t\t\t\t\treturn NULL;\r\n \t\t\t\t}\r\n \t\t\t}\r\n+#endif\r\n \t\t\t/* update value */\r\n \t\t\tif(it-\u003eflags\u0026AVP_VAL_STR)\r\n \t\t\t{\r\n@@ -803,6 +805,7 @@\r\n \t\t\t\t\u0026\u0026 strncmp(name-\u003es, it-\u003ename.s, name-\u003elen)==0)\r\n \t\t{\r\n \t\t\t/* found */\r\n+#if 0\r\n \t\t\tif(ht-\u003ehtexpire\u003e0 \u0026\u0026 it-\u003eexpire!=0 \u0026\u0026 it-\u003eexpire\u003ctime(NULL)) {\r\n \t\t\t\t/* entry has expired, delete it and return NULL */\r\n \t\t\t\tht_handle_expired_record(ht, it);\r\n@@ -818,6 +821,7 @@\r\n \t\t\t\tht_cell_free(it);\r\n \t\t\t\treturn NULL;\r\n \t\t\t}\r\n+#endif\r\n \t\t\tif(old!=NULL)\r\n \t\t\t{\r\n \t\t\t\tif(old-\u003emsize\u003e=it-\u003emsize)\r\n@@ -1056,7 +1060,6 @@\r\n \t\t\t\t\tif(it-\u003eexpire!=0 \u0026\u0026 it-\u003eexpire\u003cnow)\r\n \t\t\t\t\t{\r\n \t\t\t\t\t\t/* expired */\r\n-\t\t\t\t\t\tht_handle_expired_record(ht, it);\r\n \t\t\t\t\t\tif(it-\u003eprev==NULL)\r\n \t\t\t\t\t\t\tht-\u003eentries[i].first = it-\u003enext;\r\n \t\t\t\t\t\telse\r\n@@ -1064,6 +1067,7 @@\r\n \t\t\t\t\t\tif(it-\u003enext)\r\n \t\t\t\t\t\t\tit-\u003enext-\u003eprev = it-\u003eprev;\r\n \t\t\t\t\t\tht-\u003eentries[i].esize--;\r\n+\t\t\t\t\t\tht_handle_expired_record(ht, it);\r\n \t\t\t\t\t\tht_cell_free(it);\r\n \t\t\t\t\t}\r\n \t\t\t\t\tit = it0;\r\n```\r\n"}],"action":{"name":"View Issue","url":"https://github.com/kamailio/kamailio/issues/1152#issuecomment-309422043"}}}</script>
_______________________________________________
Kamailio (SER) - Development Mailing List
[hidden email]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [kamailio/kamailio] htable race condition on expired keys (#1152)

Daniel-Constantin Mierla-11
In reply to this post by Daniel-Constantin Mierla-11

Do you have an event_route for expired htable items in your configuration file?

I do not see at a very quick check how the removed code is causing an issue, unless there are executions with mode==0 of ht_cell_value_add(), so the lock is not acquired. As a matter of fact, that mode stuff should be removed, because the htable locks are re-entrant now.

The check for expired items needs to be done at that moment, otherwise one can increment/decrement an expired item, because the auto-expire routine is done on a timer based, on an interval basis.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"PERSON","message":"@miconda in #1152: Do you have an event_route for expired htable items in your configuration file?\r\n\r\nI do not see at a very quick check how the removed code is causing an issue, unless there are executions with mode==0 of ht_cell_value_add(), so the lock is not acquired. As a matter of fact, that mode stuff should be removed, because the htable locks are re-entrant now.\r\n\r\nThe check for expired items needs to be done at that moment, otherwise one can increment/decrement an expired item, because the auto-expire routine is done on a timer based, on an interval basis."}],"action":{"name":"View Issue","url":"https://github.com/kamailio/kamailio/issues/1152#issuecomment-309455174"}}}</script>
_______________________________________________
Kamailio (SER) - Development Mailing List
[hidden email]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [kamailio/kamailio] htable race condition on expired keys (#1152)

Daniel-Constantin Mierla-11
In reply to this post by Daniel-Constantin Mierla-11

yes, https://github.com/2600hz/kazoo-configs-kamailio/blob/master/kamailio/nodes-role.cfg


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"PERSON","message":"@lazedo in #1152: yes, https://github.com/2600hz/kazoo-configs-kamailio/blob/master/kamailio/nodes-role.cfg\r\n"}],"action":{"name":"View Issue","url":"https://github.com/kamailio/kamailio/issues/1152#issuecomment-309456035"}}}</script>
_______________________________________________
Kamailio (SER) - Development Mailing List
[hidden email]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [kamailio/kamailio] htable race condition on expired keys (#1152)

Daniel-Constantin Mierla-11
In reply to this post by Daniel-Constantin Mierla-11

OK, I will dig in more once I get some time. Again, I could not find a reason on a quick check, but no much time right now, have to go for a while. Maybe, if you have some time, you can try to remove the mode and do always locking there.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.

<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/kamailio/kamailio","title":"kamailio/kamailio","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/kamailio/kamailio"}},"updates":{"snippets":[{"icon":"PERSON","message":"@miconda in #1152: OK, I will dig in more once I get some time. Again, I could not find a reason on a quick check, but no much time right now, have to go for a while. Maybe, if you have some time, you can try to remove the mode and do always locking there."}],"action":{"name":"View Issue","url":"https://github.com/kamailio/kamailio/issues/1152#issuecomment-309459312"}}}</script>
_______________________________________________
Kamailio (SER) - Development Mailing List
[hidden email]
https://lists.kamailio.org/cgi-bin/mailman/listinfo/sr-dev
Loading...