bug#46641: process-tests assume network connection

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

bug#46641: process-tests assume network connection

Glenn Morris-3
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



Reply | Threaded
Open this post in threaded view
|

bug#46641: process-tests assume network connection

Lars Ingebrigtsen
Robert Pluim <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

bug#46641: process-tests assume network connection

Philipp Stephani
In reply to this post by Glenn Morris-3


> Am 21.02.2021 um 12:11 schrieb Robert Pluim <[hidden email]>:
>
>>>>>> On Fri, 19 Feb 2021 12:59:13 -0500, Glenn Morris <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

bug#46641: process-tests assume network connection

Philipp Stephani
In reply to this post by Lars Ingebrigtsen


> Am 21.02.2021 um 14:41 schrieb Lars Ingebrigtsen <[hidden email]>:
>
> Robert Pluim <[hidden email]> 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.




Reply | Threaded
Open this post in threaded view
|

bug#46641: process-tests assume network connection

Robert Pluim
In reply to this post by Lars Ingebrigtsen
>>>>> On Sun, 21 Feb 2021 14:41:11 +0100, Lars Ingebrigtsen <[hidden email]> 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)



Reply | Threaded
Open this post in threaded view
|

bug#46641: process-tests assume network connection

Robert Pluim
In reply to this post by Philipp Stephani
>>>>> On Sun, 21 Feb 2021 15:37:27 +0100, Philipp <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

bug#46641: process-tests assume network connection

Robert Pluim
In reply to this post by Robert Pluim
>>>>> On Sun, 21 Feb 2021 17:19:12 +0100, Robert Pluim <[hidden email]> 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)



Reply | Threaded
Open this post in threaded view
|

bug#46641: process-tests assume network connection

Lars Ingebrigtsen
Robert Pluim <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

bug#46641: process-tests assume network connection

Philipp Stephani
In reply to this post by Robert Pluim


> Am 21.02.2021 um 17:21 schrieb Robert Pluim <[hidden email]>:
>
>>>>>> On Sun, 21 Feb 2021 15:37:27 +0100, Philipp <[hidden email]> 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.


Reply | Threaded
Open this post in threaded view
|

bug#46641: process-tests assume network connection

Robert Pluim
In reply to this post by Lars Ingebrigtsen
tags 46641 fixed
close 46641 28.1
quit

>>>>> On Sun, 21 Feb 2021 19:32:25 +0100, Lars Ingebrigtsen <[hidden email]> said:

    Lars> Robert Pluim <[hidden email]> 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