smoltcp-rs / smoltcp

a smol tcp/ip stack
BSD Zero Clause License
3.63k stars 402 forks source link

Prevent panic in update_servers for Domain Name System Socket #892

Closed dunef-io closed 2 months ago

dunef-io commented 5 months ago

I wanted to connect my microcontroller with the wifi, but this library caused panics, because it found more than just one DNS Server. So I needed to change one line in the src/socket/dns.rs file with the update_servers method.

Instead of causing panic if the length of the servers argument exceeds DNS_MAX_SERVER_COUNT, it should take a slice of servers for the first DNS_MAX_SERVER_COUNT elements.

codecov[bot] commented 5 months ago

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (d185a37) 79.82% compared to head (1f99b60) 79.91%.

Files Patch % Lines
src/socket/dns.rs 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #892 +/- ## ========================================== + Coverage 79.82% 79.91% +0.08% ========================================== Files 80 80 Lines 28053 28233 +180 ========================================== + Hits 22394 22561 +167 - Misses 5659 5672 +13 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

thvdveld commented 5 months ago

Indeed, the documentation states that if the list of servers bigger than DNS_MAX_SERVER_COUNT, it will panic.

There are two options:

  1. We prevent the panic and add logic for this.
  2. We don't prevent the panic since it is written in the documentation that the list of servers cannot exceed DNS_MAX_SERVER_COUNT.

I'll leave the decision to @Dirbaio.


Your code might still panic. For example, it will panic when the length of the slice is smaller than DNS_MAX_SERVER_COUNT.

The update should be something like:

self.servers = Vec::from_slice(&servers[..servers.len().min(DNS_MAX_SERVER_COUNT)]).unwrap();
dunef-io commented 5 months ago

Ahh yes thanks for the correction!

thvdveld commented 2 months ago

I think it's better that the there is an actual panic when the provided list of servers is bigger than DNS_MAX_SERVER_COUNT. With your change, the user of the API would never know if all servers where added to the list of DNS servers. The function is also clearly documented that a panic might occur.