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

Fix Crash in dns module #904

Merged
merged 8 commits into from Dec 11, 2019
Merged

Fix Crash in dns module #904

merged 8 commits into from Dec 11, 2019

Conversation

michaelortmann
Copy link
Member

@michaelortmann michaelortmann commented Nov 26, 2019

Found by: weweilep
Patch by: michaelortmann
Fixes: #901

One-line summary:
Prevent possible crash, seen under CentOS 8, don't overflow system resolver nsaddr_list; fix size of dns-servers; fix doc and default settings; enhance logging

Additional description (if needed):
Thanks vanosg for helping with this one
sizeof dns servers was 120 and weweilep noted in IRC: 3 ipv6 addresses with full ports could hit that, IE: [aaaa:aaaa:aaaa:aaaa:aaaa:aaaa:aaaa:aaaa]:65000 [aaaa:aaaa:aaaa:aaaa:aaaa:aaaa:aaaa:aaaa]:65000 [aaaa:aaaa:aaaa:aaaa:aaaa:aaaa:aaaa:aaaa]:65000

Test cases demonstrating functionality (if applicable):
BotA.conf:

loadmodule dns
set dns-servers "1.1.1.1 foo 9.9.9.9 8.8.8.8 4.2.2.1"

$ ./eggdrop -nt BotA.conf

Module loaded: dns             
WARNING: Invalid dns-server foo
WARNING: 5 dns-servers configured but MAXNS is 3
         Surplus dns-servers ignored

.status all
  Module: dns, v 1.2
    Async DNS resolver is active.
    DNS server list: 1.1.1.1:53 9.9.9.9:53 8.8.8.8:53
    Using 0 bytes of memory

@michaelortmann
Copy link
Member Author

michaelortmann commented Nov 26, 2019

Note, that i can repeat the crash with eggdrop 1.8.4 and up to commit 35912be see #760 which was a change to dns.mod. Since this commit i can't repeat the crash. But this patch here doesn't look obsolete.

@vanosg
Copy link
Member

vanosg commented Dec 11, 2019

@weweilep have you tried this yet? Any issues?

@vanosg vanosg merged commit 7934bbd into eggheads:develop Dec 11, 2019
@michaelortmann michaelortmann deleted the maxns branch December 11, 2019 08:36
@weweilep
Copy link

I have now tested this.

Started with 4 nameservers, got a warning about limiting to 3. Uptime has properly reported without crash. .tcl dnslookup does not crash.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Random tclhash.c crash/segment violation on CentOS 8
3 participants