vingarzan left a comment (kamailio/kamailio#4191)
Hey Morten!
a seg-fault is indeed better. From my perspective it gives the
core-dump/back-trace to be able to fix the code, immediately when the abuse
potentially happens. IMHO the peer pointer should always be reset on lock
release, because an
@vingarzan commented on this pull request.
> @@ -1351,8 +1347,13 @@ void Rcv_Process(peer *p, AAAMessage *msg)
return;
}
- if(msg->sessionId)
+ if(msg->sessionId) {
+ // Ensure proper locking order
+ lock_release(p->lock);
yup, I
@vingarzan commented on this pull request.
> + // Ensure proper locking order
+ lock_release(p->lock);
session = cdp_get_session(msg->sessionId->data);
+ lock_get(p->lock);
I think this actually adds more to the current issue, maybe just i
@vingarzan commented on this pull request.
> + // Ensure proper locking order
+ lock_release(p->lock);
session = cdp_get_session(msg->sessionId->data);
+ lock_get(p->lock);
I see now that you tried to at least have the same order of lockin
vingarzan left a comment (kamailio/kamailio#4191)
The problem with "to (hopefully) have the same locking order as the other
operations" is that in theory it might solve the immediate issue... but in
practice the poison remains in the fridge :stuck_out_tongue_closed_eyes: ...
How long until some
vingarzan left a comment (kamailio/kamailio#4191)
Hey @mtryfoss!
I think I wrote some big parts of this code, but a very long time ago and it
might have a lot of changes by now that I could not be fully aware of. I also
further developed and heavily modified version of this code is in productio
@vingarzan commented on this pull request.
> @@ -367,9 +366,7 @@ int sm_process(
p->state = R_Open;
break;
case R_Rcv_Message:
- // delayed proces
:+1:
Just that there seem to be some CMake issues (if @kilianwww is stuck, maybe
@xkaraman could help with that?), otherwise, if this was tested, I think it can
be merged soon from my perspective.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/
@vingarzan commented on this pull request.
> + str found = {0, 0};
+
+ LM_DBG("isc_mark_get_from_lumps: Trying to get the mark from the add_rm
"
+ "lumps \n");
+
+ memset(mark, 0, sizeof(isc_mark));
+
+ parse_headers(msg, HDR_EOH_F, 0);
+
+ anchor_lu
Hey Victor!
sorry, I tried to understand what's going on there, but I don't get it why the
id of a sip_msg is used and how that relates to Diameter, so unable to help
much. This code was probably contributed by @carstenbock and it seems to be a
workaround to use Kamailio routing script for hand
@vingarzan commented on this pull request.
> +enum isc_mark_status
+{
+ ISCMARK_FOUND_ROUTE_HEADER =
+ 0, /*Request has been received from AS, old ISCMARK
found in Route header field*/
+ ISCMARK_FOUND_LUMPS =
+ 1, /*Request to AS has been
Module: kamailio
Branch: master
Commit: 1ceeba7364b745b6ca2840e830d006a37460d10e
URL:
https://github.com/kamailio/kamailio/commit/1ceeba7364b745b6ca2840e830d006a37460d10e
Author: Dragos Vingarzan
Committer: Dragos Vingarzan
Date: 2024-11-25T13:33:40+01:00
cdp: fixed issues discovered during st
Merged #4030 into master.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4030#event-15420024269
You are receiving this because you are subscribed to this thread.
Message ID: ___
Kamailio - Development Ma
Now it should be fine, so if all checks pass, it can be merged.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4030#issuecomment-2493544591
You are receiving this because you are subscribed to this thread.
Message ID: _
@vingarzan pushed 1 commit.
1b9f124ba2514b822554ad90a2d359af884b4bb5 cdp: fixed issues discovered during
static code analysis
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4030/files/09faa4ce88cd1ecbf544017f2f0e986c2843be48..1b9f124ba2514b822554ad90a2d359af884b4bb5
You are re
> Does this PR still need to be reviewed or is ready to merge?
I have a small concern about session.c:485 if it should be `time_t`... but then
that would go a bit deeper, yet I haven't seen issues with that. Hope to check
and fix tomorrow.
--
Reply to this email directly or view it on GitHub:
@vingarzan commented on this pull request.
> @@ -236,7 +236,7 @@ int diameter_peer_start(int blocking)
int seed;
peer *p;
- seed = random();
+ seed = kam_rand();
Right, that function is actually named `fork_process()`, exactly what I needed
:rofl:
--
Reply to t
@vingarzan commented on this pull request.
> @@ -236,7 +236,7 @@ int diameter_peer_start(int blocking)
int seed;
peer *p;
- seed = random();
+ seed = kam_rand();
Seems like that function is already called, so I can probably just remove the
seeding from here, since
@vingarzan commented on this pull request.
> @@ -830,7 +830,7 @@ int receive_loop(peer *original_peer)
#if OPENSSL_VERSION_NUMBER >= 0x1010L
if(enable_tls) {
to_ssl(&sp2-
@vingarzan pushed 1 commit.
09faa4ce88cd1ecbf544017f2f0e986c2843be48 cdp: fixed issues discovered during
static code analysis
--
View it on GitHub:
https://github.com/kamailio/kamailio/pull/4030/files/d856fa62bf9f83ea2afe38e9e74a11ec1a4c4389..09faa4ce88cd1ecbf544017f2f0e986c2843be48
You are re
@vingarzan commented on this pull request.
> @@ -236,7 +236,7 @@ int diameter_peer_start(int blocking)
int seed;
peer *p;
- seed = random();
+ seed = kam_rand();
Do we need though cryptographically secure seeding of the random though? I
looked now in core/pt.c and