Mining Dissector: Dual Registration Attempt

[dsokoloski]

Describe the bug

The Mining dissector has two different search functions, one for UDP, one for TCP. The dissector attempts to register itself twice with two different selection bitmasks:

• NDPI_SELECTION_BITMASK_PROTOCOL_V4_V6_TCP_WITH_PAYLOAD_WITHOUT_RETRANSMISSION
• NDPI_SELECTION_BITMASK_PROTOCOL_V4_V6_UDP_WITH_PAYLOAD

Expected behavior

The intended behaviour would be to process ndpi_search_mining_udp for UDP flows, and ndpi_search_mining_tcp for TCP flows.

Obtained behavior

Only the ndpi_search_mining_udp function is being called because ndpi_set_bitmask_protocol_detection will overwrite the first TCP registration with the UDP registration here.

nDPI Environment

• nDPI version or commit hash: 26df140

[IvanNardi]

I agree that Mining code should handle the TCP/UDP stuff like all the other protocols, with only one registration and only one callback.
Having said that, I don't see any real bug with the current code: ndpi_search_mining_tcp is definitely called (*idx is incremented between the two registrations).
You can try yourself running the tests with this trivial patch and see that there are some differences in the test results

diff --git a/src/lib/protocols/mining.c b/src/lib/protocols/mining.c
index f9e26068..d4584498 100644
--- a/src/lib/protocols/mining.c
+++ b/src/lib/protocols/mining.c
@@ -81,6 +81,7 @@ static u_int8_t isEthPort(u_int16_t dport) {
 
 void ndpi_search_mining_tcp(struct ndpi_detection_module_struct *ndpi_struct,
                            struct ndpi_flow_struct *flow) {
+  return;
   struct ndpi_packet_struct *packet = &ndpi_struct->packet;
 
   NDPI_LOG_DBG(ndpi_struct, "search MINING TCP\n");

Am I missing something obvious here?

[dsokoloski]

My mistake, I see now that ndpi_str->proto_defaults[ndpi_protocol_id].func is overwritten by the second registration, but ndpi_str->callback_buffer[idx].func is set uniquely as it's indexed by idx (*id += 1 by the dissector).

Just hard to read / understand I guess.

[IvanNardi]

#1496 should make the code clearer