mfontanini / libtins

High-level, multiplatform C++ network packet sniffing and crafting library.
http://libtins.github.io/
BSD 2-Clause "Simplified" License
1.89k stars 374 forks source link

Could not resolve address #440

Open Ri0ee opened 3 years ago

Ri0ee commented 3 years ago

I am trying to obtain an IPv4 address by calling Tins::Utils::resolve_domain(), however, the code is throwing Could not resolve address exception without further information, while nslookup is resolving domain names just fine. It could be that my environment is problematic, but I have no idea how to obtain further information on such an exception. Any information as to where to look for a possible fix would be appreciated.

My code is:

#include <iostream>

#define TINS_STATIC
#define WIN32
#include <tins/tins.h>

using namespace Tins;
using namespace std;

int main() {
    try {
        cout << "Resolving domain... ";
        auto target_addr = Utils::resolve_domain("google.com");
        cout << "target address: " << target_addr << "\n";
    }
    catch (Tins::exception_base e) {
        cout << e.what();
    }
}

The output: Resolving domain... Could not resolve address

laudrup commented 3 years ago

First of all you should catch exceptions by const reference. That is, rewrite catch (Tins::exception_base e) to catch (const Tins::exception_base& e).

Unfortunately, this doesn't explain why you don't get any more information from the exception in this case, since as far as I can tell the exception thrown doesn't include any detailed error (and is just a Tins::exception_base so no problem with slicing).

I think I could create a PR that should include some more detailed error information fairly quickly if you're willing to test it out. As far as I can tell you are using Windows which usually makes things more complicated, but hopefully it shouldn't be too bad in this case.

Not that this would help you actually making your code resolve the address, but I agree that it would be an improvement to include some more detailed error information anyway and it might help solve your actual issue.

Ri0ee commented 3 years ago

Yes, I'm using Windows 10 and heard that it could have some problems. I also have both npcap (for Wireshark) and winpcap (according to the tutorial) installed, unsure whether or not some problems could hide there.

I've also tried to manually resolve a domain name, using Sender.send_recv(), and while it sends DNS requests just fine, it is unable to catch the response (unlike Wireshark), response(sender.send_recv(packet, iface)) is null.

Also, it resolves hardware address just fine (both manual and Utils::resolve_hwaddr(); work fine). So it can catch the ARP reply, but not the UDP one.

I'm definitely willing to test it out more, so some changes would be appreciated.

mfontanini commented 3 years ago

Hostname resolution doesn't sniff/craft packets, it simply uses getaddrinfo. That exception could probably expose some better error message (e.g. using errno / whatever windows uses for this). You could try to copy the code I linked and see what error you're actually getting.

laudrup commented 3 years ago

@mfontanini Detailed error information can be retrieved from the return value from getaddrinfo and passing that to gai_strerror. My idea was to simply add that string to the exception message.

The same function should work on Windows, but according to the documentation is is not thread safe. The man page for the same function indicates that it is thread safe, at least the GNU implementation.

So it seems like it would be trivial to implement on Linux/BSD etc., but if thread safety is required (which I assume it is) quite a lot of work would be needed to translate the error code to a string unless a Windows Socket Error Code to string function already exists in this library?

mfontanini commented 3 years ago

Of course Windows had to make this complicated... I'm not sure if it's worth it to put this much effort into fixing this. That function is there just as a helper and it doesn't really use anything in libtins to achieve its purpose (to be honest I'm not sure why I added it in the first place).

They seem to recommend using WSAGetLastError for this but again, not sure if it's worth it to wrap all this code around ifdefs just to add a tiny bit more context into the error message.

laudrup commented 3 years ago

Windows always makes everything more complicated.

It wouldn't be just adding some #ifdefs but also calling FormatMessage correctly and ensuring locales, unicode etc. is handled correctly to get a std::string representation.

Even doing that you would most likely just end up with an "unknown error" string. Realizing that, you would try to map all the documented return values to some meaningful string representation looking at the documentation. That's when you realize that the actual error codes returned are not documented, so you end up with something like "Unknown error (0xdecafbad)" anyway.

So in short, it is definitely not simple.

If I were you I would mark the function as deprecated somehow and remove it in a coming release. The way the function is written right now is not much different from simply calling getaddrinfo, so it doesn't provide that much value anyway.

I think your library does a great job at crafting and parsing network packets, so it should focus on doing that. Making DNS lookups is quite a different task that would be better left to other libraries written for that purpose.

It's you library of course, but just sharing my opinion. It seems like we agree though :)