v3.4, ndpi_free() causes SIGSEGV trying to free tcp.tls.message.buffer

[RudeDude]

Syntax error probably caused buffer pointer to equal 0x1

A call to ndpi_free() in ndpi_main.c tries to free flow->l4.tcp.tls.message.buffer but pointer equals 0x1 which is inaccessible
Example of my own additional debug print says [TLS Mem] DE-Allocating (0 bytes, 0 used) message buffer at 0x1.

Possible copy-paste from protocols/tls.c lines 141-142 onto line 145? It looks like a comma where a semicolon and newline would be desired. newbuf is checked but assignment goes awry.

P/R diff https://github.com/RudeDude/nDPI/pull/1/files

Relevant stack dump:
Program terminated with signal SIGSEGV, Segmentation fault.
#0 0x00007f1806f42af1 in free () from /lib64/libc.so.6
#1 0x00007f180777328d in ndpi_free (ptr=0x1) at ndpi_main.c:113
#2 0x00007f180777f9a5 in ndpi_init_packet_header (ndpi_str=0x7f17d01c4010, flow=0x7f178488b370, packetlen=76) at ndpi_main.c:3716
#3 0x00007f1807782b9f in ndpi_detection_process_packet (ndpi_str=0x7f17d01c4010, flow=0x7f178488b370,
packet=0x7f177eb83c6a <error: Cannot access memory at address 0x7f177eb83c6a>, packetlen=76, current_time_ms=1604495709604, src=0x7f17845e2dd0, dst=0x7f17845e2eb8)
at ndpi_main.c:4645

[RudeDude]

submitted pull request #1051 #1051

[RudeDude]

ok, this P/R may not actually fix my issue entirely. I put a guard around the offending line in ndpi_main
if( flow->l4.tcp.tls.message.buffer_len == 0 ) { ... }

And it just hit for me printing out the info I added:
[CFAI] l4_packet_len: 24 payload_packet_len: 0 actual_payload_len: 0 iph.saddr: 59F8A783 tcp.source: 27689 iph.daddr: 407C4674 tcp.dest: 9600
[CFAI] Skip freeing of (0 bytes, 0 used) message buffer at 0x1.

[utoni]

Do you have any PCAP file to share which triggers that SIGSEGV?
That would reduce the time needed to find&fix that issue significantly.

[IvanNardi]

Could you share a pcap triggering the SEGv, please?
As already said by @lnslbrty, the comma changes are completely irrelevant to the bug you reported

[IvanNardi]

Could you share a pcap triggering the SEGv, please?
As already said by @lnslbrty, the comma changes are completely irrelevant to the bug you reported

Two minutes late :-D

[RudeDude]

On the commas: Yes, I realize now that this is doing nothing other than white space formatting. Oh well.

I have attempted to reproduce with a PCAP in the environment where I use nDPI (as a library) without success. It seems to be related to threading or packet rate. To rule out threading (from my side) I wrapped all my calls to ndpi_detection_process_packet() and any destruction of my flow pointer (ndpi_flow_struct) in a mutex lock but that didn't remove the problem.

I just started digging into instances of packet->payload_packet_len == 0 possibly causing a problem instead.

For the time being I just put a "guard" into ndpi_main.c inside the ndpi_init_packet_header(). It baffles me that the if buffer passes but my buffer_len zero check fails.

if(flow->l4.tcp.tls.message.buffer) {
  if( flow->l4.tcp.tls.message.buffer_len == 0 ) {
    fprintf(stderr,...);
  } else {
    ndpi_free(flow->l4.tcp.tls.message.buffer);
    flow->l4.tcp.tls.message.buffer = NULL;
    flow->l4.tcp.tls.message.buffer_len = flow->l4.tcp.tls.message.buffer_used = 0;
}

[IvanNardi]

I have attempted to reproduce with a PCAP in the environment where I use nDPI (as a library) without success. It seems to be related to threading or packet rate. To rule out threading (from my side) I wrapped all my calls to ndpi_detection_process_packet() and any destruction of my flow pointer (ndpi_flow_struct) in a mutex lock but that didn't remove the problem.

If feasible, try running your application with Valgrind, or try compiling it (and nDPI) with ASAN: with some luck, you might be fortunate enough to find some explicit errors before the SEGV...
My 2 cents

[RudeDude]

So my branch continues to not fix the problem so its up to you to decide how long you want to keep this open. Since I'm currently stuck in a "test in production" situation it will be a while before I can poke at this 🐻 some more. Someday I hope to replicate it outside production or just dedicate some more off-hours testing time.

[RudeDude]

Over the weekend, with my "guard" in place, I still got a crash but the stack trace is different so I've got a new lead on this and its a memmove() in my code, not nDPI. Also, I see @lucaderi merged my branch so I'll close this issue and open something new if I end up inside the nDPI code again. Thanks!