possible memory corruption in sipcapture module about kamailio HOT 11 OPEN

I've worked with @sipgate-uhlig on this topic. He's out of office this week so I took a look at it together with another colleague. We can confirm that the latest master branch does no longer suffer from this issue . Thank you @miconda for resolving this so quickly!

Can we assume that it will be part of Kamailio 5.8.2 and if so, is there an ETA?

from kamailio.

another way to do it (just as a workaround)

``
modparam("sipdump", "event_callback", "sipdump_event")

//you can check everything here - method, si, RI and send to a destination.
event_route[sipdump_event] {
sip_trace("sip:10.1.1.2:5085", "$ci-abc");
}

``

from kamailio.

btw, same you can do for sipcapture:request - execute manual sip_trace and after send return 0 or exit;

from kamailio.

@adubovikov: as I see it right now, by replicating at SIP level (e.g., via sip dump) to another system, the original source ip/destination IP are lost in the receiving system. They can eventually be added as custom headers.

from kamailio.

@adubovikov: as I see it right now, by replicating at SIP level (e.g., via sip dump) to another system, the original source ip/destination IP are lost in the receiving system. They can eventually be added as custom headers.

or we should set the hep variables. Anyway @sipgate-uhlig is talking about the corrupted $Ri and $si and unfortunately this is not related to the siptrace module. Do you have any idea ?

from kamailio.

Thank you for your input so far. All the alternatives seem to work-around the issue but not fulfill our needs.

We have debugged a little using gdb. We noticed that the value of msg->rcv->bind_address->address_str is correct before calling do_action. Afterwards, it is wrong.

Before:

s = "172.20.21.3"
len = 11

After:

s = "217.116.120.247"
len = 11 (sic!)

We took a closer look (step by step) and the problem seems to be using ip_addr2a. It returns a pointer to a local static char[]. This is assigned to the original struct of address_str inside of parsing_hepv3_message.

Later, when ip_addr2a is called again for xlog, then the same address is used again and hence the value of address_str.s is overwriten.

A quick look at the git history for the functions did not show any relevant changes to my untrained eyes.

As the jump from 5.1/5.4 (debian jessie) to 5.5/5.8 (debian bookworm) is kind of huge, maybe this is related to changes in the compiler?

from kamailio.

Thank you for your input so far. All the alternatives seem to work-around the issue but not fulfill our needs.

We have debugged a little using gdb. We noticed that the value of msg->rcv->bind_address->address_str is correct before calling do_action. Afterwards, it is wrong.

Before:

s = "172.20.21.3"
len = 11

After:

s = "217.116.120.247"
len = 11 (sic!)

We took a closer look (step by step) and the problem seems to be using ip_addr2a. It returns a pointer to a local static char[]. This is assigned to the original struct of address_str inside of parsing_hepv3_message.

Later, when ip_addr2a is called again for xlog, then the same address is used again and hence the value of address_str.s is overwriten.

A quick look at the git history for the functions did not show any relevant changes to my untrained eyes.

As the jump from 5.1/5.4 (debian jessie) to 5.5/5.8 (debian bookworm) is kind of huge, maybe this is related to changes in the compiler?

ok, it can be related to the struct pack, need to check it the union works correct. What is the compiler ?

https://github.com/kamailio/kamailio/blob/master/src/core/ip_addr.h#L74-L80

from kamailio.

For kamailio 5.8 we have used

# gcc --version
gcc (Debian 12.2.0-14) 12.2.0

from kamailio.

I am not a C developer so I have to guess here.

For me it looks like the static address of buff is returned by ip_addr2a.

char *ip_addr2a(struct ip_addr *ip)
{
	static char buff[IP_ADDR_MAX_STR_SIZE];
        // ...
	return buff;
}

-- https://github.com/kamailio/kamailio/blob/1535031a6c992c23270050793b23d290a631b684/src/core/ip_addr.c#L267C1-L276C2

If that's the case, I think every user of that function should copy the value from that address instead of remembering the address. Otherwise it is always the same address for every call of ip_addr2a and subsequent calls overwrite the value of the previous call.

Unfortunately, the sipcapture module uses the returned value directly:

si->address_str.s = ip_addr2a(&si->address);

-- https://github.com/kamailio/kamailio/blob/1535031a6c992c23270050793b23d290a631b684/src/modules/sipcapture/hep.c#L487C2-L487C46

I tested with the following code and I think it confirms my theory because foo changes to "bar".

#include <stdio.h>
#include <string.h>

char *a(char* in);

int main() {
  char *foo = a("foo");
  printf("foo %08x -> %08x = %s\n", &foo, foo, foo);

  char *bar = a("bar");
  printf("foo %08x -> %08x = %s\n", &foo, foo, foo);
  printf("bar %08x -> %08x = %s\n", &bar, bar, bar);
}

char *a(char* in) {
  static char out[10];
  strcpy(out, in);
  return out;
}

Output:

foo 0c3399d8 -> b37ba018 = foo
foo 0c3399d8 -> b37ba018 = bar
bar 0c3399e0 -> b37ba018 = bar

However, I can't explain why this should be any different in previous kamailio versions.

from kamailio.

Can you try with master branch or the patch from the commit references above?

from kamailio.