{"action":"created","issue":{"url":"https://api.github.com/repos/kamailio/kamailio/issues/1583","repository_url":"https://api.github.com/repos/kamailio/kamailio","labels_url":"https://api.github.com/repos/kamailio/kamailio/issues/1583/labels{/name}","comments_url":"https://api.github.com/repos/kamailio/kamailio/issues/1583/comments","events_url":"https://api.github.com/repos/kamailio/kamailio/issues/1583/events","html_url":"https://github.com/kamailio/kamailio/issues/1583","id":338383079,"node_id":"MDU6SXNzdWUzMzgzODMwNzk=","number":1583,"title":"cfg_rpc updates changing variable values to large random numbers ","user":{"login":"vinesinha","id":7777201,"node_id":"MDQ6VXNlcjc3NzcyMDE=","avatar_url":"https://avatars0.githubusercontent.com/u/7777201?v=4","gravatar_id":"","url":"https://api.github.com/users/vinesinha","html_url":"https://github.com/vinesinha","followers_url":"https://api.github.com/users/vinesinha/followers","following_url":"https://api.github.com/users/vinesinha/following{/other_user}","gists_url":"https://api.github.com/users/vinesinha/gists{/gist_id}","starred_url":"https://api.github.com/users/vinesinha/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vinesinha/subscriptions","organizations_url":"https://api.github.com/users/vinesinha/orgs","repos_url":"https://api.github.com/users/vinesinha/repos","events_url":"https://api.github.com/users/vinesinha/events{/privacy}","received_events_url":"https://api.github.com/users/vinesinha/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":23,"created_at":"2018-07-04T22:13:58Z","updated_at":"2018-10-16T20:44:23Z","closed_at":"2018-08-20T09:46:22Z","author_association":"NONE","body":"I was trying to do a cfg_rpc update on a variable and some of the times, the variable would take on randomly large values or negative values. \r\n\r\nThe -1 here is probably causing issues.\r\n\r\nhttps://github.com/kamailio/kamailio/blob/1d53ea3dba4e59b05b2e92ecc973c44159b9bb36/src/core/cfg/cfg_struct.c#L625\r\n\r\nHere's an explanation of what's going on (debugged using gdb). In cfg_struc.c if you put a breakpoint in the following line in cfg_clone_global\r\n\r\nhttps://github.com/kamailio/kamailio/blob/1d53ea3dba4e59b05b2e92ecc973c44159b9bb36/src/core/cfg/cfg_struct.c#L631\r\n\r\nchecking variable values:\r\n\r\n```\r\n--- Old (correct) value ---\r\n \r\n \r\n(gdb) print sizeof(*(int *)(((unsigned char *)((*cfg_global)->vars + 984)) + 172))\r\n$33 = 4\r\n(gdb) print *((unsigned char *)&(*(int *)(((unsigned char *)((*cfg_global)->vars + 984)) + 172)) + 0)\r\n$34 = 99 'c'\r\n(gdb) print *((unsigned char *)&(*(int *)(((unsigned char *)((*cfg_global)->vars + 984)) + 172)) + 1)\r\n$35 = 0 '\\000'\r\n(gdb) print *((unsigned char *)&(*(int *)(((unsigned char *)((*cfg_global)->vars + 984)) + 172)) + 2)\r\n$36 = 0 '\\000'\r\n(gdb) print *((unsigned char *)&(*(int *)(((unsigned char *)((*cfg_global)->vars + 984)) + 172)) + 3)\r\n$37 = 0 '\\000'\r\n \r\n \r\n--- new (corrupt) value ---\r\n \r\n \r\n(gdb) print sizeof(*(int *)(((unsigned char *)((block)->vars + 984)) + 172))\r\n$28 = 4\r\n(gdb) print *((unsigned char *)&(*(int *)(((unsigned char *)((block)->vars + 984)) + 172)) + 0)\r\n$29 = 99 'c'\r\n(gdb) print *((unsigned char *)&(*(int *)(((unsigned char *)((block)->vars + 984)) + 172)) + 1)\r\n$30 = 0 '\\000'\r\n(gdb) print *((unsigned char *)&(*(int *)(((unsigned char *)((block)->vars + 984)) + 172)) + 2)\r\n$31 = 0 '\\000'\r\n(gdb) print *((unsigned char *)&(*(int *)(((unsigned char *)((block)->vars + 984)) + 172)) + 3)\r\n$32 = 130 '\\202'\r\n```\r\n\r\nAs visible, the last byte is getting corrupted. \r\nI checked the memory allocation variables:\r\n\r\n```\r\n--> cfg_block_size\r\n \r\n \r\n(gdb) print cfg_block_size\r\n$2 = 1156\r\n \r\n \r\n--> sizeof(cfg_block_t)\r\n \r\n \r\nprint sizeof(cfg_block_t)\r\n$15 = 8\r\n ```\r\n \r\nThis means that total memory assigned = 8 + 1156 -(1) = 1163\r\n \r\n``` \r\n--> address of new block\r\n \r\n \r\n(gdb) print (void *) block\r\n$12 = (void *) 0x7f63086b6758\r\n \r\n \r\n--> address of the corrupted variable in the new block\r\n \r\n \r\n(gdb) print (void *)(((unsigned char *)((block)->vars + 984)) + 172)\r\n$13 = (void *) 0x7f63086b6be0\r\n \r\n \r\n--> offset of the variable from the block start\r\n \r\n \r\n(gdb) print 0x7f63086b6be0 - 0x7f63086b6758\r\n$14 = 1160\r\n```\r\n\r\nsince the variable is an integer, memory that should be assigned = 1160 + 4 = 1164\r\nHowever, we're assigning 1163.\r\n\r\nTherefore the last byte is getting corrupted.\r\n\r\nDoes it make sense to remove the -1 from all the memory allocation in cfg_struct ?"},"comment":{"url":"https://api.github.com/repos/kamailio/kamailio/issues/comments/430393708","html_url":"https://github.com/kamailio/kamailio/issues/1583#issuecomment-430393708","issue_url":"https://api.github.com/repos/kamailio/kamailio/issues/1583","id":430393708,"node_id":"MDEyOklzc3VlQ29tbWVudDQzMDM5MzcwOA==","user":{"login":"vinesinha","id":7777201,"node_id":"MDQ6VXNlcjc3NzcyMDE=","avatar_url":"https://avatars0.githubusercontent.com/u/7777201?v=4","gravatar_id":"","url":"https://api.github.com/users/vinesinha","html_url":"https://github.com/vinesinha","followers_url":"https://api.github.com/users/vinesinha/followers","following_url":"https://api.github.com/users/vinesinha/following{/other_user}","gists_url":"https://api.github.com/users/vinesinha/gists{/gist_id}","starred_url":"https://api.github.com/users/vinesinha/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/vinesinha/subscriptions","organizations_url":"https://api.github.com/users/vinesinha/orgs","repos_url":"https://api.github.com/users/vinesinha/repos","events_url":"https://api.github.com/users/vinesinha/events{/privacy}","received_events_url":"https://api.github.com/users/vinesinha/received_events","type":"User","site_admin":false},"created_at":"2018-10-16T20:44:23Z","updated_at":"2018-10-16T20:44:23Z","author_association":"NONE","body":"Say you have a kamailio config file with variables ordered as A(int),B(int),C(string),D(int) – (all belonging to the same group).\r\nKamailio first creates a linked list of these variables in a given group and calculates the group size as follows [actual code](https://github.com/kamailio/kamailio/blob/master/src/core/cfg/cfg_script.c#L34)):\r\n\r\n```\r\nPseudo-code\r\n---------------------------------------------------------------------------------------------\r\ngroup->size = 0\r\ngroup->vars = null // the pointer to the head of the linked list of variables\r\ncreate_group_linked_list:\r\n    for variable in group:\r\n        if variable is integer:\r\n            round up group_size to the nearest 4 byte multiple\r\n            group_size += sizeof(int)\r\n        if variable is string:\r\n            round up group_size to the nearest 8 byte multiple\r\n            group_size += sizeof(str) //str is a kamailio type (size 16 in our C++ compiler)\r\n        if group->vars is null:\r\n            group->vars = variable\r\n        else\r\n            variables->next = group->vars\r\n            group->vars = variable\r\n---------------------------------------------------------------------------------------------\r\nLinked list creation\r\n \r\n \r\nNULL\r\ngroup->size 0\r\n \r\n \r\nA(int)\r\ngroup->size 4\r\n \r\n \r\nB(int) -> A\r\ngroup->size 8\r\n \r\n \r\nC(string) -> B -> A\r\ngroup->size 24\r\n \r\n \r\nD(int) -> C -> B -> A\r\ngroup->size 28\r\n```\r\nHowever, this linked list is not used as is. it is copied to a single shared memory block for all child processes.\r\nThis is done in the  [cfg_shmize()](https://github.com/kamailio/kamailio/blob/master/src/core/cfg/cfg_struct.c#L180) which further calls [cfg_script_fixup()](https://github.com/kamailio/kamailio/blob/master/src/core/cfg/cfg_script.c#L222)\r\nBasically, the function goes through the linked list of variables created above and calculates the offset of each variable in the memory block assuming that the offset of the first variable is 0.\r\nFollowing is the pseudo-code of what cfg_script_fixup() does for the linked list and offset calculation:\r\n\r\n```\r\nPseudo-code\r\n--------------------------------------------------------------------------------------------\r\noffset = 0\r\nnode = group->vars:\r\nwhile node != null:\r\n    if node's value is int:\r\n        round up offset to nearest multiple of 4\r\n        node->offset = offset\r\n        offset += sizeof(int)\r\n    if node's value is str:\r\n        round up offset to nearest multiple of 8\r\n        node->offset = offset\r\n        offset += sizeof(str)\r\n    node = node -> next\r\n--------------------------------------------------------------------------------------------\r\n \r\n \r\nlinked list (from above):\r\nD(int) -> C(string) -> B(int) -> A(int)\r\n \r\n \r\noffset=0\r\n \r\nD\r\nD->offset = 0\r\noffset = 4\r\n \r\nC\r\nC->offset = 8\r\noffset = 24\r\n \r\nB\r\nB->offset = 24\r\noffset = 28\r\nA\r\nA->offset = 28\r\noffset = 32 (which would be the actual group size)\r\n```\r\n\r\nSo, as visible, ***the order in which the linked list is being processed is causing issues*** (from A to D in one case and from D to A in the next case). \r\nwhile the group size calculation code block leads to a group size of 28, the group size according to the second code block would be 32 and the offset of the last variable would we 28. This would lead to the last variable (all 4 bytes) getting corrupted.\r\n\r\nMy temporary solution is to recalculate the group size every time a variable is added in the first code block by re-traversing the linked list but is definitely not the ideal solution. "}}
{"actor":{"display_login":"vinesinha"}}