[PATCH] CYGWIN: Fix resolver debugging output

[PATCH] CYGWIN: Fix resolver debugging output

[Anton Lavrentiev]

- Use %S (instead of %s) when a wide-character output is due;
- Use native byte order for host and add port when doing I/O with DNS server;
- Use forward way for resolv.conf's "options" processing, so listing "debug" as a
  first option, will show all following option(s) as they are read;
- Re-evaluate debug output flag after each "options" processing as it may chance.
---
 winsup/cygwin/libc/minires-os-if.c |  6 +++---
 winsup/cygwin/libc/minires.c       | 32 ++++++++++++++++++------------
 2 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/winsup/cygwin/libc/minires-os-if.c b/winsup/cygwin/libc/minires-os-if.c
index 565158150..dedee3098 100644
--- a/winsup/cygwin/libc/minires-os-if.c
+++ b/winsup/cygwin/libc/minires-os-if.c
@@ -324,7 +324,7 @@ static void get_registry_dns_items(PUNICODE_STRING in, res_state statp,
 	     srch++);
 	*srch++ = 0;
 	if (numAddresses < DIM(statp->nsaddr_list)) {
-	  DPRINTF(debug, "server \"%s\"\n", ap);
+	  DPRINTF(debug, "registry server \"%s\"\n", ap);
 	  statp->nsaddr_list[numAddresses].sin_addr.s_addr = cygwin_inet_addr((char *) ap);
 	  if ( statp->nsaddr_list[numAddresses].sin_addr.s_addr != 0 )
 	    numAddresses++;
@@ -355,7 +355,7 @@ static void get_registry_dns(res_state statp)
   NTSTATUS status;
   const PCWSTR keyName = L"Tcpip\\Parameters";
 
-  DPRINTF(statp->options & RES_DEBUG, "key %s\n", keyName);
+  DPRINTF(statp->options & RES_DEBUG, "key %S\n", keyName);
   status = RtlCheckRegistryKey (RTL_REGISTRY_SERVICES, keyName);
   if (!NT_SUCCESS (status))
     {
@@ -460,7 +460,7 @@ void get_dns_info(res_state statp)
        pIPAddr;
        pIPAddr = pIPAddr->Next) {
     if (numAddresses < DIM(statp->nsaddr_list)) {
-	DPRINTF(debug, "server \"%s\"\n", pIPAddr->IpAddress.String);
+	DPRINTF(debug, "params server \"%s\"\n", pIPAddr->IpAddress.String);
       statp->nsaddr_list[numAddresses].sin_addr.s_addr = cygwin_inet_addr(pIPAddr->IpAddress.String);
       if (statp->nsaddr_list[numAddresses].sin_addr.s_addr != 0) {
 	numAddresses++;
diff --git a/winsup/cygwin/libc/minires.c b/winsup/cygwin/libc/minires.c
index 0979daae3..8c7e31218 100644
--- a/winsup/cygwin/libc/minires.c
+++ b/winsup/cygwin/libc/minires.c
@@ -73,7 +73,7 @@ void minires_get_search(char * string, res_state statp)
       statp->dnsrch[j] = strcpy(ptr, words[j]);
       statp->dnsrch[j+1] = NULL;
       ptr += sizes[j];
-      DPRINTF(debug, "search \"%s\"\n", words[j]);
+      DPRINTF(debug, "registry search \"%s\"\n", words[j]);
     }
     else if (j < MAXDNSRCH + 1)
       DPRINTF(debug, "no space for \"%s\"\n", words[j]);
@@ -86,12 +86,12 @@ Read options
 
 
 ***********************************************************************/
-static void get_options(res_state statp, int i, char **words)
+static void get_options(res_state statp, int n, char **words)
 {
   char *ptr;
-  int value;
+  int i, value;
 
-  while (i-- > 0) {
+  for (i = 0;  i < n;  ++i) {
     if (!strcasecmp("debug", words[i])) {
       statp->options |= RES_DEBUG;
       DPRINTF(statp->options & RES_DEBUG, "%s: 1\n", words[i]);
@@ -170,7 +170,7 @@ static void get_resolv(res_state statp)
   have_address = (statp->nscount != 0);
 
   while ( fgets(line, sizeof(line), fd) != 0) {
-    DPRINTF(debug, "resolv.conf %s", line);
+    DPRINTF(debug, _PATH_RESCONF " line: %s", line);
     if ((i = scanline(line, words, sizes, DIM(words))) > 0) {
       if (!have_address
 	  && !strncasecmp("nameserver", words[0], sizes[0])) {
@@ -186,7 +186,7 @@ static void get_resolv(res_state statp)
 	  else {
 	    statp->nsaddr_list[ns++].sin_addr.s_addr = address;
 	    statp->nscount++;
-	    DPRINTF(debug, "server \"%s\"\n", words[j]);
+	    DPRINTF(debug, "nameserver \"%s\"\n", words[j]);
 	  }
 	}
       }
@@ -208,8 +208,10 @@ static void get_resolv(res_state statp)
 	}
       }
       /* Options line */
-      else if (!strncasecmp("options", words[0], sizes[0]))
+      else if (!strncasecmp("options", words[0], sizes[0])) {
 	get_options(statp, i - 1, &words[1]);
+        debug = statp->options & RES_DEBUG;
+      }
     }
   }
   fclose(fd);
@@ -332,7 +334,7 @@ void res_nclose(res_state statp)
   if (statp->sockfd != -1) {
     res = close(statp->sockfd);
     DPRINTF(statp->options & RES_DEBUG, "close sockfd %d: %s\n",
-	    statp->sockfd, (res == 0)?"OK":strerror(errno));
+	    statp->sockfd, res == 0 ? "OK" : strerror(errno));
     statp->sockfd = -1;
   }
 }
@@ -503,8 +505,9 @@ int res_nsend( res_state statp, const unsigned char * MsgPtr,
     rslt = cygwin_sendto(statp->sockfd, MsgPtr, MsgLength, 0,
 			 (struct sockaddr *) &statp->nsaddr_list[wServ],
 			 sizeof(struct sockaddr_in));
-    DPRINTF(debug, "sendto: server %08x sockfd %d %s\n",
-	    statp->nsaddr_list[wServ].sin_addr.s_addr,
+    DPRINTF(debug, "sendto: server %08x:%hu sockfd %d %s\n",
+	    ntohl(statp->nsaddr_list[wServ].sin_addr.s_addr),
+	    ntohs(statp->nsaddr_list[wServ].sin_port),
 	    statp->sockfd, (rslt == MsgLength)?"OK":strerror(errno));
     if (rslt != MsgLength) {
       statp->res_h_errno = NETDB_INTERNAL;
@@ -519,8 +522,9 @@ int res_nsend( res_state statp, const unsigned char * MsgPtr,
     timeOut.tv_usec = 0;
     rslt = cygwin_select(statp->sockfd + 1, &fdset_read, NULL, NULL, &timeOut);
     if ( rslt == 0 ) { /* Timeout */
-      DPRINTF(statp->options & RES_DEBUG, "timeout for server %08x\n",
-	      statp->nsaddr_list[wServ].sin_addr.s_addr);
+      DPRINTF(statp->options & RES_DEBUG, "timeout for server %08x:%hu\n",
+	      ntohl(statp->nsaddr_list[wServ].sin_addr.s_addr),
+	      ntohs(statp->nsaddr_list[wServ].sin_port));
       continue;
     }
     else if ((rslt != 1) || (FD_ISSET(statp->sockfd, &fdset_read) == 0)) {
@@ -537,7 +541,9 @@ int res_nsend( res_state statp, const unsigned char * MsgPtr,
       statp->res_h_errno = NETDB_INTERNAL;
       return -1;
     }
-    DPRINTF(debug, "recvfrom: %d bytes from %08x\n", rslt, dnsSockAddr.sin_addr.s_addr);
+    DPRINTF(debug, "recvfrom: %d bytes from %08x:%hu\n", rslt, 
+            ntohl(dnsSockAddr.sin_addr.s_addr),
+            ntohs(dnsSockAddr.sin_port));
     /*
        Prepare to retry with tcp
     */
-- 
2.29.2

Re: [PATCH] CYGWIN: Fix resolver debugging output

[Corinna Vinschen]

Hi Anton,

On Jan 29 14:29, Anton Lavrentiev via Cygwin-patches wrote:
> - Use %S (instead of %s) when a wide-character output is due;

Please use %ls, %S is non-standard.

> - Use native byte order for host and add port when doing I/O with DNS server;

This puzzeled me a bit so I took another look into the original code.
Do I see this right that we have only limited IPv6 support in the
resolver code?  For instance, write_record appears to handle DNS_TYPE_A,
but not DNS_TYPE_AAAA.

CCing Pierre, hopefully he's still around.  Pierre, can you please
have a look and suggest how to go forward here?

> - Use forward way for resolv.conf's "options" processing, so listing "debug" as a
>   first option, will show all following option(s) as they are read;
> - Re-evaluate debug output flag after each "options" processing as it may chance.

Would you mind to split this into a patchset with patches for different
tasks?  ATM I'm a bit concerned about the ntoh{sl} calls, given the
noticable absence of IPv6 support...


Thanks,
Corinna

RE: [PATCH] CYGWIN: Fix resolver debugging output

[Lavrentiev, Anton (NIH/NLM/NCBI) [C]]

> Please use %ls, %S is non-standard.

Sure.

> For instance, write_record appears to handle DNS_TYPE_A,
> but not DNS_TYPE_AAAA.

I can add that, it's not a problem.  But indeed, reparsing of Windows packets,
does miss AAAA (as well as some other types, such as URI -- not sure if Windows
has it, though).

> Would you mind to split this into a patchset with patches for different
> tasks?  ATM I'm a bit concerned about the ntoh{sl} calls, given the
> noticable absence of IPv6 support...

Okay.  BTW, I added ntol/s only for output of *nameserver*'s IPv4:port, because
nameservers are IPv4 (even in glibc, AFAIK).  The _res structure (same in glibc)
has these addresses as "struct in_addr", meaning they are IPv4.  And so there's
no risk of running into any troubles, but reading the IP addresses in debug output
is much easier if they are in native order (and same goes for ports, even more).

Re: [PATCH] CYGWIN: Fix resolver debugging output

[Corinna Vinschen]

On Feb  1 14:23, Lavrentiev, Anton (NIH/NLM/NCBI) [C] via Cygwin-patches wrote:
> > Please use %ls, %S is non-standard.
> 
> Sure.
> 
> > For instance, write_record appears to handle DNS_TYPE_A,
> > but not DNS_TYPE_AAAA.
> 
> I can add that, it's not a problem.  But indeed, reparsing of Windows packets,
> does miss AAAA (as well as some other types, such as URI -- not sure if Windows
> has it, though).
> 
> > Would you mind to split this into a patchset with patches for different
> > tasks?  ATM I'm a bit concerned about the ntoh{sl} calls, given the
> > noticable absence of IPv6 support...
> 
> Okay.  BTW, I added ntol/s only for output of *nameserver*'s IPv4:port, because
> nameservers are IPv4 (even in glibc, AFAIK).  The _res structure (same in glibc)
> has these addresses as "struct in_addr", meaning they are IPv4.

But nameservers could be v6 addresses nevertheless, and the values are
stored in the _ext.nsaddrs member these days.  Our definition of
_res_state does not define all the members of _ext as GLibc defines,
though, and our resolver code doesn't use _ext at all, afaics.

> And so there's
> no risk of running into any troubles, but reading the IP addresses in debug output
> is much easier if they are in native order (and same goes for ports, even more).

Except, the value has no meaning for ipv6.


Corinna

RE: [PATCH] CYGWIN: Fix resolver debugging output

[Lavrentiev, Anton (NIH/NLM/NCBI) [C]]

> Except, the value has no meaning for ipv6.

It'll print all 0's :-)  But:

minires does not make use of the _ext field.  It does use the conventional nsaddr_list (which is IPv4),
but only if Windows native DNS API is not used: "osquery"(aka use_os)=0.

For debugging purposes, that is enough and very convenient (yet the output needed some tune-up, which I suggested in my patch).
But for practical purposes, only Windows API should be used in regular applications (which is the default, anyways, since
/etc/resolv.conf is not routinely provided, so "osquery=1" implicitly).

I'm not sure if improvements to use _ext by the minires own code would be any beneficial.

Having said that, AAAA replies should be made understood by the minires-if-os shim code
(and I can provide a patch for that, too).

Re: [PATCH] CYGWIN: Fix resolver debugging output

[Corinna Vinschen]

On Feb  1 15:46, Lavrentiev, Anton (NIH/NLM/NCBI) [C] via Cygwin-patches wrote:
> > Except, the value has no meaning for ipv6.
> 
> It'll print all 0's :-)  But:
> 
> minires does not make use of the _ext field.  It does use the conventional nsaddr_list (which is IPv4),
> but only if Windows native DNS API is not used: "osquery"(aka use_os)=0.
> 
> For debugging purposes, that is enough and very convenient (yet the output needed some tune-up, which I suggested in my patch).

Ok.

> But for practical purposes, only Windows API should be used in regular applications (which is the default, anyways, since
> /etc/resolv.conf is not routinely provided, so "osquery=1" implicitly).

Yeah, I think so, too.  Ideally we should have stripped out all code
providing non-Windows means (i. e., /etc/resolv.conf support) back when
this code was folded into Cygwin.  It just doesn't make sense, at least
not by default.

> I'm not sure if improvements to use _ext by the minires own code would be any beneficial.
> 
> Having said that, AAAA replies should be made understood by the minires-if-os shim code
> (and I can provide a patch for that, too).

That would be great!


Thanks,
Corinna

Re: [PATCH] CYGWIN: Fix resolver debugging output

[Brian Inglis]

On 2021-02-01 12:02, Corinna Vinschen via Cygwin-patches wrote:
> On Feb  1 15:46, Lavrentiev, Anton (NIH/NLM/NCBI) [C] via Cygwin-patches wrote:
>>> Except, the value has no meaning for ipv6.
>>
>> It'll print all 0's :-)  But: minires does not make use of the _ext field.
>> It does use the conventional nsaddr_list (which is IPv4), but only if
>> Windows native DNS API is not used: "osquery"(aka use_os)=0. >>
>> For debugging purposes, that is enough and very convenient (yet the output 
>> needed some tune-up, which I suggested in my patch).

> Ok.

>> But for practical purposes, only Windows API should be used in regular 
>> applications (which is the default, anyways, since /etc/resolv.conf is not
>> routinely provided, so "osquery=1" implicitly).
> Yeah, I think so, too.  Ideally we should have stripped out all code
> providing non-Windows means (i. e., /etc/resolv.conf support) back when
> this code was folded into Cygwin.  It just doesn't make sense, at least
> not by default.

I've been generating /etc/resolv.conf -> /run/resolvconf/resolv.conf for years 
in an /etc/postinstall/0p_...dash script from ipconfig /all output (not aware of 
a Cygwin source to get this interface info) also run at startup, with some 
customizations, and not noticed any issues so far. Should I be wary in future?

>> I'm not sure if improvements to use _ext by the minires own code would be 
>> any beneficial.
>> 
>> Having said that, AAAA replies should be made understood by the 
>> minires-if-os shim code (and I can provide a patch for that, too).

> That would be great!

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
[Data in binary units and prefixes, physical quantities in SI.]

RE: [PATCH] CYGWIN: Fix resolver debugging output

[Lavrentiev, Anton (NIH/NLM/NCBI) [C]]

> with some customizations, and not noticed any issues so far. Should I be wary in future?

Have you used the <resolv.h>/<arpa/nameser.h> APIs, or low-level DNS other than with getXbyY(), or maybe debugged DNS?

The changes are not to remove any existing functionality -- I don't see how that was not clear --
but to make it easier to use the API when you have to do a little extra, like seeing the nameserver
IP addresses in a more human-readable form.  But TBH, using native API (in the absence of /etc/resolv.conf,
or by using "options osquery" in it -- but then most of the stuff in that file is just simply ignored, JFYI),
is the best way of dealing with DNS in CYGWIN.  The minires.c implementation is rather simplistic (but it's
good when one needs to stay in full control / observe of what they are doing -- and for me is the development
stage, but I'll drop that and switch back to the OS API once everything is working as it should, for the project
that I'm dealing with right now).

Also, there was at least one bug.