Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent panic in update_servers for Domain Name System Socket #892

Closed
wants to merge 3 commits into from

Conversation

dunef-io
Copy link

@dunef-io dunef-io commented Jan 9, 2024

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.

… length of servers argument exceeds DNS_MAX_SERVER_COUNT; instead take a slice of servers for the first DNS_MAX_SERVER_COUNT elements.
Copy link

codecov bot commented Jan 9, 2024

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thvdveld
Copy link
Contributor

thvdveld commented Jan 9, 2024

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
Copy link
Author

dunef-io commented Jan 9, 2024

Ahh yes thanks for the correction!

@thvdveld
Copy link
Contributor

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.

@thvdveld thvdveld closed this Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants