[PATCH 0/1] MR4152: dnsapi: Handle IP address as DNS name in DnsQuery_UTF8().
Fixes Tekken 8 (early access) being unable to find other players for match. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4152
From: Paul Gofman <pgofman(a)codeweavers.com> --- dlls/dnsapi/query.c | 39 +++++++++++++++++++++++++++++++++++++++ dlls/dnsapi/tests/query.c | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/dlls/dnsapi/query.c b/dlls/dnsapi/query.c index 1802e93dd75..9ae1bc6edfd 100644 --- a/dlls/dnsapi/query.c +++ b/dlls/dnsapi/query.c @@ -21,11 +21,15 @@ #include <stdarg.h> #include "windef.h" #include "winbase.h" +#include "winternl.h" #include "winerror.h" #include "winnls.h" #include "windns.h" #include "nb30.h" #include "ws2def.h" +#include "in6addr.h" +#include "inaddr.h" +#include "ip2string.h" #include "wine/debug.h" #include "dnsapi.h" @@ -170,6 +174,8 @@ DNS_STATUS WINAPI DnsQuery_UTF8( const char *name, WORD type, DWORD options, voi unsigned char answer[4096]; DWORD len = sizeof(answer); struct query_params query_params = { name, type, options, answer, &len }; + DNS_RECORDA *r; + const char *end; TRACE( "(%s, %s, %#lx, %p, %p, %p)\n", debugstr_a(name), debugstr_type( type ), options, servers, result, reserved ); @@ -177,6 +183,39 @@ DNS_STATUS WINAPI DnsQuery_UTF8( const char *name, WORD type, DWORD options, voi if (!name || !result) return ERROR_INVALID_PARAMETER; + if (type == DNS_TYPE_A) + { + struct in_addr addr; + + if (!RtlIpv4StringToAddressA(name, TRUE, &end, &addr) && !*end && (r = calloc(1, sizeof(*r)))) + { + ret = ERROR_SUCCESS; + r->Data.A.IpAddress = addr.s_addr; + r->wDataLength = sizeof(r->Data.A); + } + } + else if (type == DNS_TYPE_AAAA) + { + struct in6_addr addr; + + if (!RtlIpv6StringToAddressA(name, &end, &addr) && !*end && (r = calloc(1, sizeof(*r)))) + { + ret = ERROR_SUCCESS; + memcpy(&r->Data.AAAA.Ip6Address, &addr, sizeof(r->Data.AAAA.Ip6Address)); + r->wDataLength = sizeof(r->Data.AAAA); + } + } + if (!ret) + { + r->wType = type; + r->dwTtl = 604800; + r->pName = strdup(name); + r->Flags.S.Reserved = 0x20; + r->Flags.S.CharSet = DnsCharSetUtf8; + *result = r; + return ret; + } + if ((ret = RESOLV_CALL( set_serverlist, servers ))) return ret; ret = RESOLV_CALL( query, &query_params ); diff --git a/dlls/dnsapi/tests/query.c b/dlls/dnsapi/tests/query.c index 61b7ef093c3..51438924aa0 100644 --- a/dlls/dnsapi/tests/query.c +++ b/dlls/dnsapi/tests/query.c @@ -66,6 +66,45 @@ static void test_DnsQuery(void) DNS_RECORDW *rec, *ptr; DNS_STATUS status; + /* IP in name. */ + status = DnsQuery_W(L" 192.168.111.11", DNS_TYPE_A, 0, NULL, &rec, NULL); + ok(status != ERROR_SUCCESS, "got %lu.\n", status); + status = DnsQuery_W(L"192.168.111.11 ", DNS_TYPE_A, 0, NULL, &rec, NULL); + ok(status != ERROR_SUCCESS, "got %lu.\n", status); + + status = DnsQuery_W(L"192.168.111.11", DNS_TYPE_A, 0, NULL, &rec, NULL); + ok(!status, "got %lu.\n", status); + ok(rec->wType == DNS_TYPE_A, "got %#x.\n", rec->wType); + ok(rec->wDataLength == sizeof(rec->Data.A), "got %u.\n", rec->wDataLength); + ok(rec->Data.A.IpAddress == 0x0b6fa8c0, "got %#lx.\n", rec->Data.A.IpAddress); + ok(!rec->pNext, "got %p.\n", rec->pNext); + ok(rec->dwTtl == 604800, "got %lu.\n", rec->dwTtl); + ok(!wcscmp(rec->pName, L"192.168.111.11"), "got %s.\n", debugstr_w(rec->pName)); + ok(!rec->Flags.S.Section, "got %u.\n", rec->Flags.S.Section); + ok(!rec->Flags.S.Delete, "got %u.\n", rec->Flags.S.Delete); + ok(rec->Flags.S.CharSet == DnsCharSetUnicode, "got %u.\n", rec->Flags.S.CharSet); + ok(!rec->Flags.S.Unused, "got %u.\n", rec->Flags.S.Unused); + ok(rec->Flags.S.Reserved == 0x20, "got %u.\n", rec->Flags.S.Reserved); + DnsRecordListFree(rec, DnsFreeRecordList); + + status = DnsQuery_W(L"2001:db8:3333:4444:5555:6666:7777:8888", DNS_TYPE_AAAA, 0, NULL, &rec, NULL); + ok(!status, "got %lu.\n", status); + ok(rec->wType == DNS_TYPE_AAAA, "got %#x.\n", rec->wType); + ok(rec->wDataLength == sizeof(rec->Data.AAAA), "got %u.\n", rec->wDataLength); + ok(rec->Data.AAAA.Ip6Address.IP6Dword[0] == 0xb80d0120, "got %#lx.\n", rec->Data.AAAA.Ip6Address.IP6Dword[0]); + ok(rec->Data.AAAA.Ip6Address.IP6Dword[1] == 0x44443333, "got %#lx.\n", rec->Data.AAAA.Ip6Address.IP6Dword[1]); + ok(rec->Data.AAAA.Ip6Address.IP6Dword[2] == 0x66665555, "got %#lx.\n", rec->Data.AAAA.Ip6Address.IP6Dword[2]); + ok(rec->Data.AAAA.Ip6Address.IP6Dword[3] == 0x88887777, "got %#lx.\n", rec->Data.AAAA.Ip6Address.IP6Dword[3]); + ok(!rec->pNext, "got %p.\n", rec->pNext); + ok(rec->dwTtl == 604800, "got %lu.\n", rec->dwTtl); + ok(!wcscmp(rec->pName, L"2001:db8:3333:4444:5555:6666:7777:8888"), "got %s.\n", debugstr_w(rec->pName)); + ok(!rec->Flags.S.Section, "got %u.\n", rec->Flags.S.Section); + ok(!rec->Flags.S.Delete, "got %u.\n", rec->Flags.S.Delete); + ok(rec->Flags.S.CharSet == DnsCharSetUnicode, "got %u.\n", rec->Flags.S.CharSet); + ok(!rec->Flags.S.Unused, "got %u.\n", rec->Flags.S.Unused); + ok(rec->Flags.S.Reserved == 0x20, "got %u.\n", rec->Flags.S.Reserved); + DnsRecordListFree(rec, DnsFreeRecordList); + rec = NULL; status = DnsQuery_W(L"winehq.org", DNS_TYPE_A, DNS_QUERY_STANDARD, NULL, &rec, NULL); if (status == ERROR_TIMEOUT) -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/4152
Jinoh Kang (@iamahuman) commented about dlls/dnsapi/query.c:
if (!name || !result) return ERROR_INVALID_PARAMETER;
+ if (type == DNS_TYPE_A) + { + struct in_addr addr; + + if (!RtlIpv4StringToAddressA(name, TRUE, &end, &addr) && !*end && (r = calloc(1, sizeof(*r)))) + { + ret = ERROR_SUCCESS; + r->Data.A.IpAddress = addr.s_addr; + r->wDataLength = sizeof(r->Data.A); + }
The failure mode of allocation seems a bit off. When we fail to `calloc`, we have three choices of behaviors: 1. Crash. 2. Fail with error. 3. Revert to the old behavior of delegating to actual DNS query. Here we do (3), which seems to be the most confusing choice here.[^1] Can we just drop the NULL check or handle the error properly? (Ditto for IPv6.) [^1]: Why? If the allocation ever fails, it will unpredictably return bogus NXDOMAIN answer to the application. If it never fails, then there is no reason for the NULL check in the first place. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4152#note_49541
On Sat Oct 21 09:26:42 2023 +0000, Jinoh Kang wrote:
The failure mode of allocation seems a bit off. When we fail to `calloc`, we have three choices of behaviors: 1. Crash. 2. Fail with error. 3. Revert to the old behavior of delegating to actual DNS query, which will mask the alloc failure with some other query error. Here we do (3), which seems to be the most confusing choice here.[^1] Can we just drop the NULL check or handle the error properly? (Ditto for IPv6.) [^1]: Why? If the allocation ever fails, it will unpredictably return bogus NXDOMAIN answer to the application. If it never fails, then there is no reason for the NULL check in the first place. Will this nit change anything in practice? If a few bytes can't be allocated, it is done already, abstract perfection of such a place doesn't worth any extra line of code. Regarding dropping the null check, I think the common rule in Wine is to check the allocations. I'd personally would agree that in many cases just skipping the check and crashing would make more sense than putting code for any handling the error just anyhow, but I am just following what I think is the rule here.
Doesn't matter, but I think it won't get an unpredicatable result on the main path. It will fail to allocate memory later and get an error, or, in case of miraculous getting memory back, it will get an error anyway being unable to find it (both errors, ERROR_OUT_OF_MEMORY or what it would get, are not the correct result anyway). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4152#note_49549
Regarding dropping the null check, I think the common rule in Wine is to check the allocations. I'd personally would agree that in many cases just skipping the check and crashing would make more sense than putting code for any handling the error just anyhow, but I am just following what I think is the rule here.
Well, that rule sounds absurd and contradictory. 1. We should check for NULL. 2. Except NULL doesn't happen much in practice, so we shouldn't really check for NULL anyway. Correct me if I'm wrong, I don't think such rule (pretend to check for NULL) has ever been in enforcement for upstream patches. In fact, I've got https://gitlab.winehq.org/wine/wine/-/merge_requests/3324 upstreamed before; not sure if this generalizes to this case too. Otherwise, is `assert()`-ing an option?
or, in case of miraculous getting memory back,
This could be more probable than you think. For example, another thread might momentarily allocate a large amount of memory (towards RSS/VM ulimit) and then free it. If we race, we'll get a spontaneous allocation failure.
it will get an error anyway being unable to find it (both errors, ERROR_OUT_OF_MEMORY or what it would get, are not the correct result anyway).
Depends on how the app handles the error; for example, it could retry on ERROR_OUT_OF_MEMORY but not whatever NXDOMAIN is. Also, the behavior (3) has additional side effect of potentially generating a packet over a network, in a program that would not otherwise emit any packets (it only resolves "127.0.0.1"). Granted, you're not making the rules here, and existing wine code doesn't strictly abide by graceful allocation failure handling either. I was merely puzzled by how such confusing rule is in place. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/4152#note_49551
participants (3)
-
Jinoh Kang (＠iamahuman) -
Paul Gofman
-
Paul Gofman (＠gofman)