sr-dev September 2015

sr-dev@lists.kamailio.org

37 participants
387 discussions

git:master:cbc5034b: parser: safer function to parse header name stored in short buffer

by Daniel-Constantin Mierla

Module: kamailio Branch: master Commit: cbc5034b26189371c2ffb80424414aa8b1b8dea5 URL: https://github.com/kamailio/kamailio/commit/cbc5034b26189371c2ffb80424414aa… Author: Daniel-Constantin Mierla <miconda(a)gmail.com> Committer: Daniel-Constantin Mierla <miconda(a)gmail.com> Date: 2015-09-02T13:07:45+02:00 parser: safer function to parse header name stored in short buffer --- Modified: parser/parse_hname2.c Modified: parser/parse_hname2.h --- Diff: https://github.com/kamailio/kamailio/commit/cbc5034b26189371c2ffb80424414aa… Patch: https://github.com/kamailio/kamailio/commit/cbc5034b26189371c2ffb80424414aa… --- diff --git a/parser/parse_hname2.c b/parser/parse_hname2.c index c90d3e5..42fb865 100644 --- a/parser/parse_hname2.c +++ b/parser/parse_hname2.c @@ -242,3 +242,31 @@ char* parse_hname2(char* const begin, const char* const end, struct hdr_field* c } } +/** + * parse_hname2_short() - safer version to parse header name stored in short buffers + * - parse_hanem2() reads 4 bytes at once, expecting to walk through a buffer + * that contains more than the header name (e.g., sip msg buf, full header buf + * with name and body) + */ +char* parse_hname2_short(char* const begin, const char* const end, struct hdr_field* const hdr) +{ +#define HBUF_MAX_SIZE 256 + char hbuf[HBUF_MAX_SIZE]; + char *p; + + if(end-begin>=HBUF_MAX_SIZE-4) { + p = q_memchr(p, ':', end - begin); + if(p && p-4> begin) { + /* header name termination char found and enough space in buffer after it */ + return parse_hname2(begin, end, hdr); + } + /* not enough space */ + return NULL; + } + /* pad with whitespace - tipycal char after the ':' of the header name */ + memset(hbuf, ' ', HBUF_MAX_SIZE); + memcpy(hbuf, begin, end-begin); + p = parse_hname2(hbuf, hbuf + 4 + (end-begin), hdr); + if(!p) return NULL; + return begin + (p-hbuf); +} diff --git a/parser/parse_hname2.h b/parser/parse_hname2.h index 6f5b76b..d44abb7 100644 --- a/parser/parse_hname2.h +++ b/parser/parse_hname2.h @@ -36,5 +36,6 @@ * @file */ char* parse_hname2(char* const begin, const char* const end, struct hdr_field* const hdr); +char* parse_hname2_safe(char* const begin, const char* const end, struct hdr_field* const hdr); #endif /* PARSE_HNAME2_H */

8 years, 8 months

git:master:aad248fc: tmx: err level replaced with info when getting t reply code pv

by Daniel-Constantin Mierla

Module: kamailio Branch: master Commit: aad248fc39279f84879929772e9306fc01473860 URL: https://github.com/kamailio/kamailio/commit/aad248fc39279f84879929772e9306f… Author: Daniel-Constantin Mierla <miconda(a)gmail.com> Committer: Daniel-Constantin Mierla <miconda(a)gmail.com> Date: 2015-09-02T09:18:09+02:00 tmx: err level replaced with info when getting t reply code pv --- Modified: modules/tmx/t_var.c --- Diff: https://github.com/kamailio/kamailio/commit/aad248fc39279f84879929772e9306f… Patch: https://github.com/kamailio/kamailio/commit/aad248fc39279f84879929772e9306f… --- diff --git a/modules/tmx/t_var.c b/modules/tmx/t_var.c index 0338233..fa1478c 100644 --- a/modules/tmx/t_var.c +++ b/modules/tmx/t_var.c @@ -518,7 +518,8 @@ int pv_get_tm_reply_code(struct sip_msg *msg, pv_param_t *param, } break; default: - LM_ERR("unsupported route_type %d\n", get_route_type()); + LM_INFO("unsupported route_type %d - code set to 0\n", + get_route_type()); code = 0; } }

8 years, 8 months

git:4.2:99ea3939: tsilo: Fix transaction removal to update list head

by Federico Cabiddu

Module: kamailio Branch: 4.2 Commit: 99ea393959af90a9ea612f431a7c6e3648434f75 URL: https://github.com/kamailio/kamailio/commit/99ea393959af90a9ea612f431a7c6e3… Author: Chris Double <chris.double(a)double.co.nz> Committer: Federico Cabiddu <federico.cabiddu(a)gmail.com> Date: 2015-09-01T16:30:56+02:00 tsilo: Fix transaction removal to update list head - When transaction being removed is the head of the list, correctly update the head to point to the next transaction. (cherry picked from commit 6ce6803d57dabe287d7d6fa859e93c1df402d821) --- Modified: modules/tsilo/ts_hash.c --- Diff: https://github.com/kamailio/kamailio/commit/99ea393959af90a9ea612f431a7c6e3… Patch: https://github.com/kamailio/kamailio/commit/99ea393959af90a9ea612f431a7c6e3… --- diff --git a/modules/tsilo/ts_hash.c b/modules/tsilo/ts_hash.c index 31e1dcf..44f6dce 100644 --- a/modules/tsilo/ts_hash.c +++ b/modules/tsilo/ts_hash.c @@ -396,8 +396,8 @@ void remove_ts_transaction(ts_transaction_t* ts_t) if (ts_t->prev) ts_t->prev->next = ts_t->next; - if ((ts_t->prev == NULL) && (ts_t->next == NULL)) - ts_t->urecord->transactions = NULL; + if (ts_t->urecord->transactions == ts_t) + ts_t->urecord->transactions = ts_t->next; free_ts_transaction((void*)ts_t);

8 years, 8 months

git:4.3:4d1b658b: tsilo: Fix transaction removal to update list head

by Federico Cabiddu

Module: kamailio Branch: 4.3 Commit: 4d1b658b2b380349b7d74fae7083661a18f04bf1 URL: https://github.com/kamailio/kamailio/commit/4d1b658b2b380349b7d74fae7083661… Author: Chris Double <chris.double(a)double.co.nz> Committer: Federico Cabiddu <federico.cabiddu(a)gmail.com> Date: 2015-09-01T16:30:30+02:00 tsilo: Fix transaction removal to update list head - When transaction being removed is the head of the list, correctly update the head to point to the next transaction. (cherry picked from commit 6ce6803d57dabe287d7d6fa859e93c1df402d821) --- Modified: modules/tsilo/ts_hash.c --- Diff: https://github.com/kamailio/kamailio/commit/4d1b658b2b380349b7d74fae7083661… Patch: https://github.com/kamailio/kamailio/commit/4d1b658b2b380349b7d74fae7083661… --- diff --git a/modules/tsilo/ts_hash.c b/modules/tsilo/ts_hash.c index cf529ee..7986991 100644 --- a/modules/tsilo/ts_hash.c +++ b/modules/tsilo/ts_hash.c @@ -396,8 +396,8 @@ void remove_ts_transaction(ts_transaction_t* ts_t) if (ts_t->prev) ts_t->prev->next = ts_t->next; - if ((ts_t->prev == NULL) && (ts_t->next == NULL)) - ts_t->urecord->transactions = NULL; + if (ts_t->urecord->transactions == ts_t) + ts_t->urecord->transactions = ts_t->next; free_ts_transaction((void*)ts_t);

8 years, 8 months

git:master:199202f4: Merge pull request #311 from doublec/tsilo_transaction_fix

by Federico Cabiddu

Module: kamailio Branch: master Commit: 199202f453b4a5f01783a4c2ba5c5ed2e10b0739 URL: https://github.com/kamailio/kamailio/commit/199202f453b4a5f01783a4c2ba5c5ed… Author: Federico Cabiddu <federico.cabiddu(a)gmail.com> Committer: Federico Cabiddu <federico.cabiddu(a)gmail.com> Date: 2015-09-01T16:28:29+02:00 Merge pull request #311 from doublec/tsilo_transaction_fix Change remove_ts_transaction so it updates head pointer of list --- Modified: modules/tsilo/ts_hash.c --- Diff: https://github.com/kamailio/kamailio/commit/199202f453b4a5f01783a4c2ba5c5ed… Patch: https://github.com/kamailio/kamailio/commit/199202f453b4a5f01783a4c2ba5c5ed… --- diff --git a/modules/tsilo/ts_hash.c b/modules/tsilo/ts_hash.c index cf529ee..7986991 100644 --- a/modules/tsilo/ts_hash.c +++ b/modules/tsilo/ts_hash.c @@ -396,8 +396,8 @@ void remove_ts_transaction(ts_transaction_t* ts_t) if (ts_t->prev) ts_t->prev->next = ts_t->next; - if ((ts_t->prev == NULL) && (ts_t->next == NULL)) - ts_t->urecord->transactions = NULL; + if (ts_t->urecord->transactions == ts_t) + ts_t->urecord->transactions = ts_t->next; free_ts_transaction((void*)ts_t);

8 years, 8 months

git:master:6ce6803d: tsilo: Fix transaction removal to update list head

by Chris Double

Module: kamailio Branch: master Commit: 6ce6803d57dabe287d7d6fa859e93c1df402d821 URL: https://github.com/kamailio/kamailio/commit/6ce6803d57dabe287d7d6fa859e93c1… Author: Chris Double <chris.double(a)double.co.nz> Committer: Chris Double <chris.double(a)double.co.nz> Date: 2015-09-02T00:45:58+12:00 tsilo: Fix transaction removal to update list head - When transaction being removed is the head of the list, correctly update the head to point to the next transaction. --- Modified: modules/tsilo/ts_hash.c --- Diff: https://github.com/kamailio/kamailio/commit/6ce6803d57dabe287d7d6fa859e93c1… Patch: https://github.com/kamailio/kamailio/commit/6ce6803d57dabe287d7d6fa859e93c1… --- diff --git a/modules/tsilo/ts_hash.c b/modules/tsilo/ts_hash.c index cf529ee..7986991 100644 --- a/modules/tsilo/ts_hash.c +++ b/modules/tsilo/ts_hash.c @@ -396,8 +396,8 @@ void remove_ts_transaction(ts_transaction_t* ts_t) if (ts_t->prev) ts_t->prev->next = ts_t->next; - if ((ts_t->prev == NULL) && (ts_t->next == NULL)) - ts_t->urecord->transactions = NULL; + if (ts_t->urecord->transactions == ts_t) + ts_t->urecord->transactions = ts_t->next; free_ts_transaction((void*)ts_t);

8 years, 8 months

[kamailio] Change remove_ts_transaction so it updates head pointer of list (#311)

by Chris Double

If the transaction being removed is the first item in the list of transactions then we need to update the pointer to the head of the list so it does not have a stale reference. This is an attempt to fix the tsilo crashes we've been seeing. The crashes occur in ts_onreply while iterating over the transactions to remove the transaction for the TMCB_DESTROY callback. One of the transaction pointers is not a valid shared memory address and the process crashes. This crash results in the main kamailio process getting a SIGCHILD signal and it tries to shut down. This reaches code to free the transactions and it crashes in free_ts_urecord while trying to free the same transaction. Inspecting code I can't tell how remove_ts_transaction resets urecord->transactions to be the head of the list of the first transaction to be removed is the first item in the list. This would leave a dangling pointer there and seems likely to be the cause of the crash. You can view, comment on, or merge this pull request online at: https://github.com/kamailio/kamailio/pull/311 -- Commit Summary -- * Change remove_ts_transaction so it updates head pointer of list -- File Changes -- M modules/tsilo/ts_hash.c (4) -- Patch Links -- https://github.com/kamailio/kamailio/pull/311.patch https://github.com/kamailio/kamailio/pull/311.diff --- Reply to this email directly or view it on GitHub: https://github.com/kamailio/kamailio/pull/311

8 years, 8 months

Buffer overflow in parse_hname2

by Chris Double

In parser/parse_hname2.c we've hit what looks to be a 1 byte buffer overflow error. This was picked up in an ASAN enabled build. This is on 4.3 branch. Investigation shows that parse_hname2 is called with 'begin' set to the string "Reason:". This string was originally allocated in in rval_get_str as length 6, contents "Reason\0'. The actual pkg_malloc is size of 7 to account for the null terminator. In the caller to parse_hname2 (modules/textops/textops.c line 2229) the null terminator is replaced with a ':' character. parse_hname2 hits the FIRST_QUARTERNIONS macro which expands to a bunch of case statements. The one for the Reason string looks like (macro expanded): case _reas_: p += 4; val = READ(p); switch(LOWER_DWORD(val)) { case _on1_: hdr->type = HDR_REASON_T; hdr->name.len = 6; return (p + 3); The overflow occurs in the READ call. READ is: #define READ(val) \ (*(val + 0) + (*(val + 1) << 8) + (*(val + 2) << 16) + (*(val + 3) << 24)) With 'p' pointing to "Reason:", then p+4 is "on:". That's only three characters of allocated memory left(the : was originally the null character as explained above and the total pkg_malloc allocated length was 7). READ accesses 4 bytes so we go one past the end of the allocated area. ASAN bails at this point but in a non-ASAN build it would reach the 'case _on1_' clause. __on1__ is defined as: #define _on1_ 0x203a6e6f /* "on: " */ This matches for a space at the end of the string but that space won't exist since there were only 7 characters originally allocated. Only by good luck would it match. The error is noticeable in a DBG_SYS_MALLOC build but not a PKG_MALLOC build - I assume the latter has a large arena allocated making the buffer overflow still valid memory. Assuming my analysis is correct I'd like to fix this by putting some length checking in places and using a READ call that accounts for it. Would this be an acceptable approach? It's pretty complex code and I don't want to mess up so I welcome advice on how to address the issue if there's a better way. -- http://bluishcoder.co.nz

8 years, 8 months

db_free_columns and freeing column name

by Chris Double

This question relates to the commit in http://lists.sip-router.org/pipermail/sr-dev/2015-August/030514.html The db1_res_1 structure stores a string for the column names in the query. The commit in the email referenced above fixed an issue in the postgres backend where it was storing an internal postgres pointer in the result structure but by the time the results were used in sqlops a PQclear had been performed leaving that pointer dangling. Eventually a use after free occurs. The fix in the commit is to copy the column name into a pkg_malloc'd area. The question I have is where should this be free'd. I thought db_free_columns in lib/srdb1/db_res.c would be the place. In that function it frees the column str object (RES_NAMES(_r)[col]) but not the string char* (RES_NAMES(_r)[col]->s). Changing that function to free the string would seem to be the right fix - is it free'd anywhere else that I'm missing? Looking at the other database backends to work out whether they store anything there that needs to be free'd shows similar issues to the postgres one that was fixed in the commit referenced earlier: db_berkeley: Uses a pointer to internal database results db_mongodb: Uses a pointer to internal database results db_mysql: Uses a pointer to internal database results db_text: Uses a pointer to internal database results db_unixodbc: Uses a pointer to a stack variable The other backends (oracle, sqlite) seem to do the same as the fix made to the postgres backend. The unixodbc backend seem to have a dangling pointer into the stack which is problematic. Should these DB backends be changed to copy the column name instead of storing an internal pointer? If so, is db_free_columns the correct place to free that memory? Is there anywhere else that stores column name data in results which might need to be modified? -- http://bluishcoder.co.nz

8 years, 8 months

kex & pkg_info

by Carsten Bock

Hi, quick question: We've compiled Kamailio with the following settings: version: kamailio 4.3.1 (x86_64/linux) 7cd85b flags: STATS: Off, USE_TCP, USE_TLS, USE_SCTP, TLS_HOOKS, USE_RAW_SOCKS, DISABLE_NAGLE, USE_MCAST, DNS_IP_HACK, SHM_MEM, SHM_MMAP, F_MALLOC, DBG_F_MALLOC, USE_FUTEX, FAST_LOCK-ADAPTIVE_WAIT, USE_DNS_CACHE, USE_DNS_FAILOVER, USE_NAPTR, USE_DST_BLACKLIST, HAVE_RESOLV_RES ADAPTIVE_WAIT_LOOPS=1024, MAX_RECV_BUFFER_SIZE 262144, MAX_LISTEN 16, MAX_URI_SIZE 1024, BUF_SIZE 65535, DEFAULT PKG_SIZE 8MB poll method support: poll, epoll_lt, epoll_et, sigio_rt, select. id: 7cd85b (we are using F_MALLOC to do some memory debugging) However, with this compiler settings, we cannot use the "kex" module anymore: kamailio -c loading modules under config path: /usr/lib64/kamailio/modules/:/usr/lib/kamailio/modules/:/usr/lib/x86_64-linux-gnu/kamailio/modules/ 0(25506) ERROR: <core> [sr_module.c:574]: load_module(): could not open module </usr/lib/x86_64-linux-gnu/kamailio/modules/kex.so>: /usr/lib/x86_64-linux-gnu/kamailio/modules/kex.so: undefined symbol: pkg_info 0(25506) : <core> [cfg.y:3432]: yyerror_at(): parse error in config file /etc/kamailio/kamailio.cfg, line 63, column 12-16: failed to load module Looking at the files at "mem/*", the pkg_info seems to be defined for me. Does anyone (with more knowledge on the Memory stuff) have a clue, why this is not working? Thanks, Carsten -- Carsten Bock CEO (Geschäftsführer) ng-voice GmbH Schomburgstr. 80 D-22767 Hamburg / Germany http://www.ng-voice.com mailto:carsten@ng-voice.com Office +49 40 5247593-0 Fax +49 40 5247593-99 Sitz der Gesellschaft: Hamburg Registergericht: Amtsgericht Hamburg, HRB 120189 Geschäftsführer: Carsten Bock Ust-ID: DE279344284 Hier finden Sie unsere handelsrechtlichen Pflichtangaben: http://www.ng-voice.com/imprint/

8 years, 8 months

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

2008

sr-dev September 2015