mdns crash due to not check empty proto, service (IDFGH-6675)

[chegewara]

Hi, im not sure if its due to my badly initialized mdns, but i believe it should be "fixed" anyway, sometimes we can receive requests with only instance, and having service and proto empty:

instance: test
instance: test
instance: test
instance: test
GET / HTTP/1.1
User-Agent: Dalvik/2.1.0 (Linux; U; Android 11; SM-A725F Build/RP1A.200720.012)
Host: 192.168.0.113:50000
Connection: Keep-Alive
Accept-Encoding: gzip

instance: softap-8AE4
service: _telnet
proto: _tcp

This is causing crash here:
https://github.com/espressif/esp-idf/blob/master/components/mdns/mdns.c#L287-L288

I dont know if it is good solution, as i am fairly new to mdns, but this works to me:

    return (_str_null_or_empty(service) || !strcasecmp(srv->service, service)) && 
            (_str_null_or_empty(instance) || _mdns_instance_name_match(srv->instance, instance)) &&
            (_str_null_or_empty(proto) || !strcasecmp(srv->proto, proto)) && 
            (_str_null_or_empty(hostname) || !strcasecmp(srv->hostname, hostname));

Long story short, i am trying to add mdns to product with TCP/UDP server.
Testing with Service browser app on android.
esp-idf 309c111

Thanks

[chegewara]

Another bug this time, is that mdns allows to set port to 0 (at least my synology NAS server is advertising with port 0 on _device-info._tcp), but this check will return with ESP_ERR_INVALID_ARG
https://github.com/espressif/esp-idf/blob/master/components/mdns/mdns.c#L5168

[chegewara]

mdns also crash here due to service == NULL
https://github.com/espressif/esp-idf/blob/master/components/mdns/mdns.c#L1608

Simply check solves it
if(!service) return; - continue will stay in endless loop

PS i doubt it is related, but i am using TCP and UDP socket servers together with mdns, although the proposed fixed are working

[chegewara]

There is few more crashes. The easy way to recreate it is:

• run 2 esp32 with the same mdns code
• when 1 esp32 is fully operating, then restart another
• i will try to test with mdns example and confirm

    const char* name, const char* ip, uint16_t port, int type // TCP or UDP;
    ESP_RETURN_ON_ERROR(mdns_init(), TAG, "failed to init mdns");
    ESP_RETURN_ON_ERROR(mdns_hostname_set(name), TAG, "failed to set hostname");
    ESP_LOGI(TAG, "mdns hostname set to: [%s]", name);
    //set default mDNS instance name
    ESP_ERROR_CHECK_WITHOUT_ABORT(mdns_instance_name_set(name));

    mdns_ip_addr_t addr4;
    esp_netif_str_to_ip4(ip, &addr4.addr.u_addr.ip4);
    addr4.addr.type = ESP_IPADDR_TYPE_V4;
    addr4.next = NULL;
    ESP_ERROR_CHECK_WITHOUT_ABORT(mdns_delegate_hostname_add(name, &addr4));
       
    if(type) ESP_ERROR_CHECK_WITHOUT_ABORT( mdns_service_add(NULL, "_telnet", "_udp", port, NULL, 0) );
    else ESP_ERROR_CHECK_WITHOUT_ABORT( mdns_service_add(NULL, "_telnet", "_tcp", port, NULL, 0) );

[david-cermak]

Hi @chegewara

Thanks very much for sharing the steps to reproduce those issues! I've just checked that they are fixed by the recently published bugfix related to default instances: f46dffca627 (that's not included in the version you reference above)

Could you please update your master and test again? The proposed fixes you shared make sense in general, but we have to make sure that if the service or proto becomes NULL we don't have an issue with fqdn parsing, or mark the name invalid.

As for allowing the por to be zero, I don't think we should support that, it's an invalid port number and although avahi allows for it, dns-sd doesn't.

[chegewara]

Thanks for checking it. I will update to master today and check it again.
Im not saying it is very important to support port 0, just saying that my synology NAS server, which is based on linux is advertising _device-info with port 0, and i thought it is normally supported, but of course i am noob in that matter and i will accept that answer.

[chegewara]

@david-cermak
It updated esp-idf to e899edd but still the same crash:

Guru Meditation Error: Core  0 panic'ed (LoadProhibited). Exception was unhandled.

Core  0 register dump:
PC      : 0x400011f1  PS      : 0x00060130  A0      : 0x800dd965  A1      : 0x3ffc7a10  
A2      : 0x0000005f  A3      : 0x00000000  A4      : 0x00000006  A5      : 0x3ffd1c26  
A6      : 0x0000006c  A7      : 0x00000000  A8      : 0x00000000  A9      : 0x00000000  
A10     : 0x0000007f  A11     : 0x3ff96354  A12     : 0x3ffd1b80  A13     : 0x00000000  
A14     : 0x00000001  A15     : 0x00000001  SAR     : 0x00000007  EXCCAUSE: 0x0000001c  
EXCVADDR: 0x00000000  LBEG    : 0x4000c2e0  LEND    : 0x4000c2f6  LCOUNT  : 0x00000000  


Backtrace:0x400011ee:0x3ffc7a100x400dd962:0x3ffc7a30 0x400dd9df:0x3ffc7a50 0x400e11fc:0x3ffc7a70 0x400e2603:0x3ffc7ab0 0x400e2c75:0x3ffc7b50 0x400e2cd6:0x3ffc7b90 0x4008c8b9:0x3ffc7bc0 
0x400dd962: _mdns_service_match_instance at /home/chegewara/esp/master/components/mdns/mdns.c:287

0x400dd9df: _mdns_get_service_item_instance at /home/chegewara/esp/master/components/mdns/mdns.c:297

0x400e11fc: _mdns_create_answer_from_parsed_packet at /home/chegewara/esp/master/components/mdns/mdns.c:1607

0x400e2603: mdns_parse_packet at /home/chegewara/esp/master/components/mdns/mdns.c:3653

0x400e2c75: _mdns_execute_action at /home/chegewara/esp/master/components/mdns/mdns.c:4643

0x400e2cd6: _mdns_service_task at /home/chegewara/esp/master/components/mdns/mdns.c:4766

0x4008c8b9: vPortTaskWrapper at /home/chegewara/esp/master/components/freertos/FreeRTOS-Kernel/portable/xtensa/port.c:134

Its exactly the same issue from OP.

[chegewara]

Just in case, this is android app i am using:
https://play.google.com/store/apps/details?id=com.druk.servicebrowser&hl=en_US&gl=US

esp32 does not crash until i want to read complete information advertised by mdns service.

[david-cermak]

Oh, thanks for the additional pointers! I'm able to reproduce the issue with ServiceBrowser! will fix.

[david-cermak]

@chegewara Found one more crash and one mem-leak when testing this scenario. Here's the preliminary patch, it's very WIP, but should at least help you to continue with testing.
mdns_WIP_fix_null_deref.txt

[chegewara]

Thanks, i will try to test it.

[chegewara]

Hi @david-cermak
sorry for late reply, but i am sick.
After quick test i can tell the patch seems to work, and no longer crash.

Thanks