bug#46641: process-tests assume network connection

bug#46641: process-tests assume network connection

[Glenn Morris]

Package: emacs
Version: 27.1

Some process-tests fail if the system has no network connection.
I don't know what the appropriate skip-unless condition to test for
network access is.

Ref: https://bugs.debian.org/982969

bug#46641: process-tests assume network connection

[Robert Pluim]

>>>>> On Fri, 19 Feb 2021 12:59:13 -0500, Glenn Morris <rgm@gnu.org> said:

    Glenn> Package: emacs
    Glenn> Version: 27.1

    Glenn> Some process-tests fail if the system has no network connection.
    Glenn> I don't know what the appropriate skip-unless condition to test for
    Glenn> network access is.

    Glenn> Ref: https://bugs.debian.org/982969

So Debian deliberately cripple their test environment, run the network
tests for an editor which can do network access, and we have to adapt
our tests? I am not amused.

I guess we could wrap them all in

(skip-unless (dns-query "google.com"))

instead of the getenv "EMACS_HYDRA_CI" stuff, assuming Debian donʼt
complain about that as well.

Robert

PS And since when did 'does not pass all tests' become 'fails to build
from source'?

bug#46641: process-tests assume network connection

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

>     Glenn> Some process-tests fail if the system has no network connection.
>     Glenn> I don't know what the appropriate skip-unless condition to test for
>     Glenn> network access is.
>
>     Glenn> Ref: https://bugs.debian.org/982969
>
> So Debian deliberately cripple their test environment, run the network
> tests for an editor which can do network access, and we have to adapt
> our tests? I am not amused.
>
> I guess we could wrap them all in
>
> (skip-unless (dns-query "google.com"))

It'd be nice if Emacs did have a predicate for "is there any network
here?"  But I don't know what that would look like.  That is, there's a
difference between having a local network (i.e., 127.0.0.1), and being
on the Internet.  

But if any of our tests require Emacs to be on a functioning internet
connection, they should indeed be guarded by something like the
`skip-unless' you propose.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46641: process-tests assume network connection

[Philipp]

> Am 21.02.2021 um 12:11 schrieb Robert Pluim <rpluim@gmail.com>:
> 
>>>>>> On Fri, 19 Feb 2021 12:59:13 -0500, Glenn Morris <rgm@gnu.org> said:
> 
>    Glenn> Package: emacs
>    Glenn> Version: 27.1
> 
>    Glenn> Some process-tests fail if the system has no network connection.
>    Glenn> I don't know what the appropriate skip-unless condition to test for
>    Glenn> network access is.
> 
>    Glenn> Ref: https://bugs.debian.org/982969
> 
> So Debian deliberately cripple their test environment, run the network
> tests for an editor which can do network access, and we have to adapt
> our tests? I am not amused.

This is pretty common for CI systems.  Accessing the network is a security risk, and in addition tends to make tests unreproducible.

bug#46641: process-tests assume network connection

[Philipp]

> Am 21.02.2021 um 14:41 schrieb Lars Ingebrigtsen <larsi@gnus.org>:
> 
> Robert Pluim <rpluim@gmail.com> writes:
> 
>>    Glenn> Some process-tests fail if the system has no network connection.
>>    Glenn> I don't know what the appropriate skip-unless condition to test for
>>    Glenn> network access is.
>> 
>>    Glenn> Ref: https://bugs.debian.org/982969
>> 
>> So Debian deliberately cripple their test environment, run the network
>> tests for an editor which can do network access, and we have to adapt
>> our tests? I am not amused.
>> 
>> I guess we could wrap them all in
>> 
>> (skip-unless (dns-query "google.com"))
> 
> It'd be nice if Emacs did have a predicate for "is there any network
> here?"

This isn’t a yes/no question.  For example, it’s often a good idea to put tests into a network namespace that only has a loopback interface.  In that case, there’s some network (the loopback interface), but that still doesn’t allow Internet access.

bug#46641: process-tests assume network connection

[Robert Pluim]

>>>>> On Sun, 21 Feb 2021 14:41:11 +0100, Lars Ingebrigtsen <larsi@gnus.org> said:

    >> I guess we could wrap them all in
    >> 
    >> (skip-unless (dns-query "google.com"))

    Lars> It'd be nice if Emacs did have a predicate for "is there any network
    Lars> here?"  But I don't know what that would look like.  That is, there's a
    Lars> difference between having a local network (i.e., 127.0.0.1), and being
    Lars> on the Internet.  

    Lars> But if any of our tests require Emacs to be on a functioning internet
    Lars> connection, they should indeed be guarded by something like the
    Lars> `skip-unless' you propose.

Well, the tests in question are asking 'does emacs have a correctly
functioning internet connection', so making that a prerequisite for
the test seems kind of redundant, but we can do it. But first:
disabling my network connection causes dns-query to hang, so something
like this is needed, I think (we can skip the first hunk if you want):

diff --git a/lisp/net/dns.el b/lisp/net/dns.el
index 2045d4dfca..598ceebab8 100644
--- a/lisp/net/dns.el
+++ b/lisp/net/dns.el
@@ -332,7 +332,7 @@ dns-set-servers
 	  (setq dns-servers (nreverse dns-servers))))
       (when (executable-find "nslookup")
 	(with-temp-buffer
-	  (call-process "nslookup" nil t nil "localhost")
+	  (call-process "nslookup" nil t nil "-retry=0" "-timeout=2" "localhost")
 	  (goto-char (point-min))
           (when (re-search-forward
 	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\|[[:xdigit:]:]*\\)" nil t)
@@ -496,15 +496,17 @@ dns-query
   "Query a DNS server for NAME of TYPE.
 If FULL, return the entire record returned.
 If REVERSE, look up an IP address."
-  (let ((result nil))
-    (dns-query-asynchronous
-     name
-     (lambda (response)
-       (setq result (list response)))
-     type full reverse)
-    ;; Loop until we get the callback.
-    (while (not result)
-      (sleep-for 0.01))
+  (let ((result nil)
+        (query-started
+         (dns-query-asynchronous
+          name
+          (lambda (response)
+            (setq result (list response)))
+          type full reverse)))
+    (if query-started
+        ;; Loop until we get the callback.
+        (while (not result)
+          (sleep-for 0.01)))
     (car result)))
 
 (provide 'dns)

bug#46641: process-tests assume network connection

[Robert Pluim]

>>>>> On Sun, 21 Feb 2021 15:37:27 +0100, Philipp <p.stephani2@gmail.com> said:

    Philipp> This is pretty common for CI systems.  Accessing the network is a
    Philipp> security risk, and in addition tends to make tests unreproducible.

I can give you the second one, but in what way is eg doing a DNS lookup a
'security risk'? Weʼre not talking about setting up a listening server
on a public IP here.

Robert

bug#46641: process-tests assume network connection

[Robert Pluim]

>>>>> On Sun, 21 Feb 2021 17:19:12 +0100, Robert Pluim <rpluim@gmail.com> said:

    Robert> Well, the tests in question are asking 'does emacs have a correctly
    Robert> functioning internet connection', so making that a prerequisite for
    Robert> the test seems kind of redundant, but we can do it. But first:
    Robert> disabling my network connection causes dns-query to hang, so something
    Robert> like this is needed, I think (we can skip the first hunk if you want):

Ah, the wonders of running diff when you haven't tested the result
from a clean emacs. This one actually works.

diff --git a/lisp/net/dns.el b/lisp/net/dns.el
index 2045d4dfca..3ae7469798 100644
--- a/lisp/net/dns.el
+++ b/lisp/net/dns.el
@@ -332,7 +332,7 @@ dns-set-servers
 	  (setq dns-servers (nreverse dns-servers))))
       (when (executable-find "nslookup")
 	(with-temp-buffer
-	  (call-process "nslookup" nil t nil "localhost")
+	  (call-process "nslookup" nil t nil "-retry=0" "-timeout=2" "localhost")
 	  (goto-char (point-min))
           (when (re-search-forward
 	   "^Address:[ \t]*\\([0-9]+\\.[0-9]+\\.[0-9]+\\.[0-9]+\\|[[:xdigit:]:]*\\)" nil t)
@@ -496,15 +496,17 @@ dns-query
   "Query a DNS server for NAME of TYPE.
 If FULL, return the entire record returned.
 If REVERSE, look up an IP address."
-  (let ((result nil))
-    (dns-query-asynchronous
-     name
-     (lambda (response)
-       (setq result (list response)))
-     type full reverse)
-    ;; Loop until we get the callback.
-    (while (not result)
-      (sleep-for 0.01))
+  (let* ((result nil)
+         (query-started
+          (dns-query-asynchronous
+           name
+           (lambda (response)
+             (setq result (list response)))
+           type full reverse)))
+    (if query-started
+        ;; Loop until we get the callback.
+        (while (not result)
+          (sleep-for 0.01)))
     (car result)))
 
 (provide 'dns)

bug#46641: process-tests assume network connection

[Lars Ingebrigtsen]

Robert Pluim <rpluim@gmail.com> writes:

> Ah, the wonders of running diff when you haven't tested the result
> from a clean emacs. This one actually works.

Haven't tested the patch, but it makes sense to me.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#46641: process-tests assume network connection

[Philipp]

> Am 21.02.2021 um 17:21 schrieb Robert Pluim <rpluim@gmail.com>:
> 
>>>>>> On Sun, 21 Feb 2021 15:37:27 +0100, Philipp <p.stephani2@gmail.com> said:
> 
>    Philipp> This is pretty common for CI systems.  Accessing the network is a
>    Philipp> security risk, and in addition tends to make tests unreproducible.
> 
> I can give you the second one, but in what way is eg doing a DNS lookup a
> 'security risk'? Weʼre not talking about setting up a listening server
> on a public IP here.

A CI system will typically run arbitrary code that’s not under the control of the CI system itself.  Therefore, the CI system needs to prevent any malicious behavior of the system under test.  Since the code being tested is opaque, the CI system can’t really decide whether it’s malicious or not, so it has to conservatively assume that any network access is malicious.  While it might be possible to prevent more specific behavior (like creating a listening socket), that tends to be more complex, so the simpler and safer „no network at all“ tends to be a reasonable choice.

bug#46641: process-tests assume network connection

[Robert Pluim]

tags 46641 fixed 
close 46641 28.1
quit

>>>>> On Sun, 21 Feb 2021 19:32:25 +0100, Lars Ingebrigtsen <larsi@gnus.org> said:

    Lars> Robert Pluim <rpluim@gmail.com> writes:
    >> Ah, the wonders of running diff when you haven't tested the result
    >> from a clean emacs. This one actually works.

    Lars> Haven't tested the patch, but it makes sense to me.

Pushed as 934dcc2157

And the requisite changes to the test suite I pushed as a728135a2b

Robert