bug#40343: 28.0.50; dns-set-servers fails on IPv6 available Windows

bug#40343: 28.0.50; dns-set-servers fails on IPv6 available Windows

[Kazuhiro Ito]

dns-set-servers function fails to set dns-servers on Windows with IPv6
available network.

(progn
  (require 'dns)
  (dns-set-servers)
  dns-servers)

-> (nil)

The reason is that Windows's nslookup program returns server's IPv6
address if availeble but dns-set-servers function assumes that
server's address is always IPv4.

> C:\>nslookup localhost
> サーバー:  server.lan
> Address:  xxxx:xxxx:xxxx::1
>
> 名前:    localhost.lan
> Addresses:  ::1
>           127.0.0.1

# "サーバー" and "名前" are Japanese translations for "Server" and
# "Name", respectively.

From dns-set-servers definition in lisp/net/dns.el

> (re-search-forward
>  "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\)" nil t)

-- 
Kazuhiro Ito

bug#40343: 28.0.50; dns-set-servers fails on IPv6 available Windows

[Robert Pluim]

>>>>> On Tue, 31 Mar 2020 17:46:47 +0900, Kazuhiro Ito <kzhr@d1.dion.ne.jp> said:

    Kazuhiro> dns-set-servers function fails to set dns-servers on Windows with IPv6
    Kazuhiro> available network.

    Kazuhiro> (progn
    Kazuhiro>   (require 'dns)
    Kazuhiro>   (dns-set-servers)
    Kazuhiro>   dns-servers)

    -> (nil)

dns.el isnʼt going to work on Windows anyway, since Emacs doesnʼt
support UDP network processes on Windows. You can use
'network-lookup-address-info' (builtin) or 'nslookup-host' (from
net-utils.el) instead.

    Kazuhiro> From dns-set-servers definition in lisp/net/dns.el

    >> (re-search-forward
    >> "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\)" nil t)

That could be fixed, I suppose.

Robert

bug#40343: 28.0.50; dns-set-servers fails on IPv6 available Windows

[Kazuhiro Ito]

>     Kazuhiro> dns-set-servers function fails to set dns-servers on Windows with IPv6
>     Kazuhiro> available network.
> 
>     Kazuhiro> (progn
>     Kazuhiro>   (require 'dns)
>     Kazuhiro>   (dns-set-servers)
>     Kazuhiro>   dns-servers)
> 
>     -> (nil)
> 
> dns.el isnʼt going to work on Windows anyway, since Emacs doesnʼt
> support UDP network processes on Windows.

Cygwin's Emacs, which seems to support UDP network processes, uses
Windows's nslookup.exe, so has this problem too.  

# Actually I don't directly use dns.el.  It is gravatar.el which uses
# dns.el.

-- 
Kazuhiro Ito

bug#40343: 28.0.50; dns-set-servers fails on IPv6 available Windows

[Robert Pluim]

>>>>> On Tue, 31 Mar 2020 20:06:50 +0900, Kazuhiro Ito <kzhr@d1.dion.ne.jp> said:

    Kazuhiro> dns-set-servers function fails to set dns-servers on Windows with IPv6
    Kazuhiro> available network.
    >> 
    Kazuhiro> (progn
    Kazuhiro> (require 'dns)
    Kazuhiro> (dns-set-servers)
    Kazuhiro> dns-servers)
    >> 
    -> (nil)
    >> 
    >> dns.el isnʼt going to work on Windows anyway, since Emacs doesnʼt
    >> support UDP network processes on Windows.

    Kazuhiro> Cygwin's Emacs, which seems to support UDP network processes, uses
    Kazuhiro> Windows's nslookup.exe, so has this problem too.  

Thatʼs not a Windows Emacs :-)

    Kazuhiro> # Actually I don't directly use dns.el.  It is gravatar.el which uses
    Kazuhiro> # dns.el.

True. We should teach emacs about res_query.

Does this work for you:

diff --git a/lisp/net/dns.el b/lisp/net/dns.el
index 78d4827162..fd2ea99ac6 100644
--- a/lisp/net/dns.el
+++ b/lisp/net/dns.el
@@ -328,7 +328,7 @@ dns-set-servers
 	  (call-process "nslookup" nil t nil "localhost")
 	  (goto-char (point-min))
 	  (re-search-forward
-	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\)" nil t)
+	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\)\\|\\([[:xdigit:]:]*\\)" nil t)
 	  (setq dns-servers (list (match-string 1))))))
   (when (fboundp 'network-interface-list)
     (setq dns-servers-valid-for-interfaces (network-interface-list))))

bug#40343: 28.0.50; dns-set-servers fails on IPv6 available Windows

[Kazuhiro Ito]

> Does this work for you:
> 
> diff --git a/lisp/net/dns.el b/lisp/net/dns.el
> index 78d4827162..fd2ea99ac6 100644
> --- a/lisp/net/dns.el
> +++ b/lisp/net/dns.el
> @@ -328,7 +328,7 @@ dns-set-servers
>  	  (call-process "nslookup" nil t nil "localhost")
>  	  (goto-char (point-min))
>  	  (re-search-forward
> -	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\)" nil t)
> +	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\)\\|\\([[:xdigit:]:]*\\)" nil t)
>  	  (setq dns-servers (list (match-string 1))))))
>    (when (fboundp 'network-interface-list)
>      (setq dns-servers-valid-for-interfaces (network-interface-list))))

I needed to modify your regexp as below.  Additionally, I have
question that whether we should consider the case nslookup doesn't
return DNS server address, i.e. the case that re-search-forward didn't
match anything.  In that case, (match-string 1) at next line returns
inconstant value.  But according to docstring of dns-set-servers,
dns-servers should be set to nil.

diff --git a/lisp/net/dns.el b/lisp/net/dns.el
index 78d4827162..92db9a5bac 100644
--- a/lisp/net/dns.el
+++ b/lisp/net/dns.el
@@ -328,7 +328,7 @@ dns-set-servers
 	  (call-process "nslookup" nil t nil "localhost")
 	  (goto-char (point-min))
 	  (re-search-forward
-	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\)" nil t)
+	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\|[[:xdigit:]:]*\\)" nil t)
 	  (setq dns-servers (list (match-string 1))))))
   (when (fboundp 'network-interface-list)
     (setq dns-servers-valid-for-interfaces (network-interface-list))))

-- 
Kazuhiro Ito

bug#40343: 28.0.50; dns-set-servers fails on IPv6 available Windows

[Robert Pluim]

>>>>> On Tue, 31 Mar 2020 23:22:33 +0900, Kazuhiro Ito <kzhr@d1.dion.ne.jp> said:

    >> Does this work for you:
    >> 
    >> diff --git a/lisp/net/dns.el b/lisp/net/dns.el
    >> index 78d4827162..fd2ea99ac6 100644
    >> --- a/lisp/net/dns.el
    >> +++ b/lisp/net/dns.el
    >> @@ -328,7 +328,7 @@ dns-set-servers
    >> (call-process "nslookup" nil t nil "localhost")
    >> (goto-char (point-min))
    >> (re-search-forward
    >> -	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\)" nil t)
    >> +	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\)\\|\\([[:xdigit:]:]*\\)" nil t)
    >> (setq dns-servers (list (match-string 1))))))
    >> (when (fboundp 'network-interface-list)
    >> (setq dns-servers-valid-for-interfaces (network-interface-list))))

    Kazuhiro> I needed to modify your regexp as below.

Ah yes, I got the grouping wrong.

    Kazuhiro> Additionally, I have
    Kazuhiro> question that whether we should consider the case nslookup doesn't
    Kazuhiro> return DNS server address, i.e. the case that re-search-forward didn't
    Kazuhiro> match anything.  In that case, (match-string 1) at next line returns
    Kazuhiro> inconstant value.  But according to docstring of dns-set-servers,
    Kazuhiro> dns-servers should be set to nil.

The dns lookup will fail in either case, no? So Iʼm not sure it really
matters.

Robert

bug#40343: 28.0.50; dns-set-servers fails on IPv6 available Windows

[Kazuhiro Ito]

>     Kazuhiro> Additionally, I have
>     Kazuhiro> question that whether we should consider the case nslookup doesn't
>     Kazuhiro> return DNS server address, i.e. the case that re-search-forward didn't
>     Kazuhiro> match anything.  In that case, (match-string 1) at next line returns
>     Kazuhiro> inconstant value.  But according to docstring of dns-set-servers,
>     Kazuhiro> dns-servers should be set to nil.
> 
> The dns lookup will fail in either case, no? So Iʼm not sure it really
> matters.

For example, when I tested on my standalone Debian (sid) box,
evaluating the below code just after starting Emacs returns annoying
result.

(progn
  (require 'dns)
  (dns-set-servers)
  dns-servers)

-> ("c")

We need to validate dns-servers's value to judge whether
dns-set-servers succeeded contorary docstring of dns-set-servers,
which says dns-servers is non-nil when dns-set-servers succeeded.  I
think docstring or actual behavior of dns-set-servers should be fixed.


I noticed another problem in dns.el.  dns-query function returns
message's string ("No DNS server configuration found") when
dns-servers is nil.

-- 
Kazuhiro Ito

bug#40343: 28.0.50; dns-set-servers fails on IPv6 available Windows

[Robert Pluim]

>>>>> On Wed, 01 Apr 2020 22:43:20 +0900, Kazuhiro Ito <kzhr@d1.dion.ne.jp> said:

    Kazuhiro> Additionally, I have
    Kazuhiro> question that whether we should consider the case nslookup doesn't
    Kazuhiro> return DNS server address, i.e. the case that re-search-forward didn't
    Kazuhiro> match anything.  In that case, (match-string 1) at next line returns
    Kazuhiro> inconstant value.  But according to docstring of dns-set-servers,
    Kazuhiro> dns-servers should be set to nil.
    >> 
    >> The dns lookup will fail in either case, no? So Iʼm not sure it really
    >> matters.

    Kazuhiro> For example, when I tested on my standalone Debian (sid) box,
    Kazuhiro> evaluating the below code just after starting Emacs returns annoying
    Kazuhiro> result.

    Kazuhiro> (progn
    Kazuhiro>   (require 'dns)
    Kazuhiro>   (dns-set-servers)
    Kazuhiro>   dns-servers)

    -> ("c")

Yes, but the API exposed by dns.el is 'dns-query' (and maybe
'dns-query-cached'). 'dns-set-servers' should probably be called
'dns--set-servers' to indicate that itʼs an internal function.

    Kazuhiro> We need to validate dns-servers's value to judge whether
    Kazuhiro> dns-set-servers succeeded contorary docstring of dns-set-servers,
    Kazuhiro> which says dns-servers is non-nil when dns-set-servers succeeded.  I
    Kazuhiro> think docstring or actual behavior of dns-set-servers should be fixed.

See previous paragraph: itʼs an internal function.

    Kazuhiro> I noticed another problem in dns.el.  dns-query function returns
    Kazuhiro> message's string ("No DNS server configuration found") when
    Kazuhiro> dns-servers is nil.

Yes, it should probably return nil, and this is actually a valid
reason for why 'dns-set-servers' should set dns-servers to nil on
failure. Can you determine why itʼs failing on your Debian box?

Robert

bug#40343: 28.0.50; dns-set-servers fails on IPv6 available Windows

[Kazuhiro Ito]

>     Kazuhiro> For example, when I tested on my standalone Debian (sid) box,
>     Kazuhiro> evaluating the below code just after starting Emacs returns annoying
>     Kazuhiro> result.
> 
>     Kazuhiro> (progn
>     Kazuhiro>   (require 'dns)
>     Kazuhiro>   (dns-set-servers)
>     Kazuhiro>   dns-servers)
> 
>     -> ("c")
> 
> Yes, but the API exposed by dns.el is 'dns-query' (and maybe
> 'dns-query-cached'). 'dns-set-servers' should probably be called
> 'dns--set-servers' to indicate that itʼs an internal function.
> 
>     Kazuhiro> We need to validate dns-servers's value to judge whether
>     Kazuhiro> dns-set-servers succeeded contorary docstring of dns-set-servers,
>     Kazuhiro> which says dns-servers is non-nil when dns-set-servers succeeded.  I
>     Kazuhiro> think docstring or actual behavior of dns-set-servers should be fixed.
> 
> See previous paragraph: itʼs an internal function.

I don't understand what you mean by "itʼs an internal function".  I
don't think it is okay that internal functions don't behave as
described in docstring.

>     Kazuhiro> I noticed another problem in dns.el.  dns-query function returns
>     Kazuhiro> message's string ("No DNS server configuration found") when
>     Kazuhiro> dns-servers is nil.
> 
> Yes, it should probably return nil, and this is actually a valid
> reason for why 'dns-set-servers' should set dns-servers to nil on
> failure. Can you determine why itʼs failing on your Debian box?

I tested on Debian box as below.

1. /etc/init.d/networking stop
2. remove nameserver entry from /etc/resolv.conf

On such condition, nslookup program outputs as below

> ;; connection timed out; no servers could be reached

Here is code snippet communicating with nslookup in dns-set-servers.

> (with-temp-buffer
>   (call-process "nslookup" nil t nil "localhost")
>   (goto-char (point-min))
>   (re-search-forward
>    "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\)" nil t)
>   (setq dns-servers (list (match-string 1))))

On the above condition, re-search-forward doesn't match anything.  But
next match-string is called unconditionally.  As far as I tested,
match-string's result when last search didn't match is undefined.  it
may return nil, string or raise an error.  Additionally, even if
match-string returns nil, dns-servers is never set to nil.  It is set
to (nil).

-- 
Kazuhiro Ito

bug#40343: 28.0.50; dns-set-servers fails on IPv6 available Windows

[Robert Pluim]

>>>>> On Wed, 01 Apr 2020 23:55:12 +0900, Kazuhiro Ito <kzhr@d1.dion.ne.jp> said:

    Kazuhiro> I tested on Debian box as below.

    Kazuhiro> 1. /etc/init.d/networking stop
    Kazuhiro> 2. remove nameserver entry from /etc/resolv.conf

    Kazuhiro> On such condition, nslookup program outputs as below

    >> ;; connection timed out; no servers could be reached

    Kazuhiro> Here is code snippet communicating with nslookup in dns-set-servers.

    >> (with-temp-buffer
    >> (call-process "nslookup" nil t nil "localhost")
    >> (goto-char (point-min))
    >> (re-search-forward
    >> "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\)" nil t)
    >> (setq dns-servers (list (match-string 1))))

    Kazuhiro> On the above condition, re-search-forward doesn't match anything.  But
    Kazuhiro> next match-string is called unconditionally.  As far as I tested,
    Kazuhiro> match-string's result when last search didn't match is undefined.  it
    Kazuhiro> may return nil, string or raise an error.  Additionally, even if
    Kazuhiro> match-string returns nil, dns-servers is never set to nil.  It is set
    Kazuhiro> to (nil).

Thanks for that. This should fix all those cases:

diff --git a/lisp/net/dns.el b/lisp/net/dns.el
index 78d4827162..177df4e332 100644
--- a/lisp/net/dns.el
+++ b/lisp/net/dns.el
@@ -315,8 +315,8 @@ dns-servers-up-to-date-p
 (defun dns-set-servers ()
   "Set `dns-servers' to a list of DNS servers or nil if none are found.
 Parses \"/etc/resolv.conf\" or calls \"nslookup\"."
+  (setq dns-servers nil)
   (or (when (file-exists-p "/etc/resolv.conf")
-	(setq dns-servers nil)
 	(with-temp-buffer
 	  (insert-file-contents "/etc/resolv.conf")
 	  (goto-char (point-min))
@@ -327,9 +327,9 @@ dns-set-servers
 	(with-temp-buffer
 	  (call-process "nslookup" nil t nil "localhost")
 	  (goto-char (point-min))
-	  (re-search-forward
-	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\)" nil t)
-	  (setq dns-servers (list (match-string 1))))))
+          (when (re-search-forward
+	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\|[[:xdigit:]:]*\\)" nil t)
+	      (setq dns-servers (list (match-string 1)))))))
   (when (fboundp 'network-interface-list)
     (setq dns-servers-valid-for-interfaces (network-interface-list))))
 
@@ -357,7 +357,9 @@ dns-make-network-process
   `(let ((server ,server)
 	 (coding-system-for-read 'binary)
 	 (coding-system-for-write 'binary))
-     (if (fboundp 'make-network-process)
+     (if (and
+          (fboundp 'make-network-process)
+          (featurep 'make-network-process '(:type datagram)))
 	 (make-network-process
 	  :name "dns"
 	  :coding 'binary
@@ -365,9 +367,9 @@ dns-make-network-process
 	  :host server
 	  :service "domain"
 	  :type 'datagram)
-       ;; Older versions of Emacs doesn't have
-       ;; `make-network-process', so we fall back on opening a TCP
-       ;; connection to the DNS server.
+       ;; Older versions of Emacs do not have `make-network-process',
+       ;; and on MS-Windows datagram sockets are not supported, so we
+       ;; fall back on opening a TCP connection to the DNS server.
        (open-network-stream "dns" (current-buffer) server "domain"))))
 
 (defvar dns-cache (make-vector 4096 0))
@@ -400,7 +402,9 @@ dns-query
 	  type 'PTR))
 
   (if (not dns-servers)
-      (message "No DNS server configuration found")
+      (progn
+        (message "No DNS server configuration found")
+        nil)
     (with-temp-buffer
       (set-buffer-multibyte nil)
       (let ((process (condition-case ()

bug#40343: 28.0.50; dns-set-servers fails on IPv6 available Windows

[Kazuhiro Ito]

> Thanks for that. This should fix all those cases:

Thank you for the patch.  It works well on Cygwin environment.  But I
noticed different problems in dns-query when TCP is used.

1. dns-write function needs to be called with TCP-P option.
2. First 2 bytes of received data (length field) should be truncated
for dns-read function.  Or dns-read function should accept TCP-P
option like dns-wrote function.


> diff --git a/lisp/net/dns.el b/lisp/net/dns.el
> index 78d4827162..177df4e332 100644
> --- a/lisp/net/dns.el
> +++ b/lisp/net/dns.el
> @@ -315,8 +315,8 @@ dns-servers-up-to-date-p
>  (defun dns-set-servers ()
>    "Set `dns-servers' to a list of DNS servers or nil if none are found.
>  Parses \"/etc/resolv.conf\" or calls \"nslookup\"."
> +  (setq dns-servers nil)
>    (or (when (file-exists-p "/etc/resolv.conf")
> -	(setq dns-servers nil)
>  	(with-temp-buffer
>  	  (insert-file-contents "/etc/resolv.conf")
>  	  (goto-char (point-min))
> @@ -327,9 +327,9 @@ dns-set-servers
>  	(with-temp-buffer
>  	  (call-process "nslookup" nil t nil "localhost")
>  	  (goto-char (point-min))
> -	  (re-search-forward
> -	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\)" nil t)
> -	  (setq dns-servers (list (match-string 1))))))
> +          (when (re-search-forward
> +	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\|[[:xdigit:]:]*\\)" nil t)
> +	      (setq dns-servers (list (match-string 1)))))))
>    (when (fboundp 'network-interface-list)
>      (setq dns-servers-valid-for-interfaces (network-interface-list))))
>  
> @@ -357,7 +357,9 @@ dns-make-network-process
>    `(let ((server ,server)
>  	 (coding-system-for-read 'binary)
>  	 (coding-system-for-write 'binary))
> -     (if (fboundp 'make-network-process)
> +     (if (and
> +          (fboundp 'make-network-process)
> +          (featurep 'make-network-process '(:type datagram)))
>  	 (make-network-process
>  	  :name "dns"
>  	  :coding 'binary
> @@ -365,9 +367,9 @@ dns-make-network-process
>  	  :host server
>  	  :service "domain"
>  	  :type 'datagram)
> -       ;; Older versions of Emacs doesn't have
> -       ;; `make-network-process', so we fall back on opening a TCP
> -       ;; connection to the DNS server.
> +       ;; Older versions of Emacs do not have `make-network-process',
> +       ;; and on MS-Windows datagram sockets are not supported, so we
> +       ;; fall back on opening a TCP connection to the DNS server.
>         (open-network-stream "dns" (current-buffer) server "domain"))))
>  
>  (defvar dns-cache (make-vector 4096 0))
> @@ -400,7 +402,9 @@ dns-query
>  	  type 'PTR))
>  
>    (if (not dns-servers)
> -      (message "No DNS server configuration found")
> +      (progn
> +        (message "No DNS server configuration found")
> +        nil)
>      (with-temp-buffer
>        (set-buffer-multibyte nil)
>        (let ((process (condition-case ()

-- 
Kazuhiro Ito

bug#40343: 28.0.50; dns-set-servers fails on IPv6 available Windows

[Robert Pluim]

>>>>> On Fri, 03 Apr 2020 23:08:10 +0900, Kazuhiro Ito <kzhr@d1.dion.ne.jp> said:

    >> Thanks for that. This should fix all those cases:
    Kazuhiro> Thank you for the patch.  It works well on Cygwin environment.  But I
    Kazuhiro> noticed different problems in dns-query when TCP is used.

Thanks for checking, pushed to master as 00f7744c1b

Closing the bug.

    Kazuhiro> 1. dns-write function needs to be called with TCP-P option.
    Kazuhiro> 2. First 2 bytes of received data (length field) should be truncated
    Kazuhiro> for dns-read function.  Or dns-read function should accept TCP-P
    Kazuhiro> option like dns-wrote function.

Indeed, looks like thatʼs never worked. Iʼll fix those separately.

Robert