[PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[David Edmondson]

When trying to test Lars' async-dns changes I encountered memory
corruption whenever the addresses returned for a host included IPv6
addresses. This turns out to be a bug in the mainline code which is just
more likely to be tickled by the async-dns changes because they convert
addresses to lisp objects whenever `make-network-process' is called (the
current code doesn't do that).

Here is a proposed fix:

From c9995971d47e78475fb5ec9d9b34d2187941227f Mon Sep 17 00:00:00 2001
Date: Sat, 6 Feb 2016 16:02:23 +0000
Subject: [PATCH] * src/process.c (conv_lisp_to_sockaddr): Correctly convert
 AF_INET6 addresses

AF_INET6 addresses are converted to a list of 16 bit quantities by
conv_sockaddr_to_lisp(). conv_lisp_to_sockaddr() should follow the
same scheme rather than expecting a (longer) list of 8 bit quantities.
---
 src/process.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/process.c b/src/process.c
index e1ebdff..1eac5e1 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2372,7 +2372,7 @@ conv_lisp_to_sockaddr (int family, Lisp_Object address, struct sockaddr *sa, int
 	{
 	  struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *) sa;
 	  uint16_t *ip6 = (uint16_t *)&sin6->sin6_addr;
-	  len = sizeof (sin6->sin6_addr) + 1;
+	  len = sizeof (sin6->sin6_addr) / 2 + 1;
 	  hostport = XINT (p->contents[--len]);
 	  sin6->sin6_port = htons (hostport);
 	  for (i = 0; i < len; i++)
-- 
2.6.3

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Lars Ingebrigtsen]

David Edmondson <dme@dme.org> writes:

> AF_INET6 addresses are converted to a list of 16 bit quantities by
> conv_sockaddr_to_lisp(). conv_lisp_to_sockaddr() should follow the
> same scheme rather than expecting a (longer) list of 8 bit quantities.

Hm...  The way IPv6 addresses in the IP lists are distinguished from
IPv4 addresses is by the length of the lists.  For instance:

get_lisp_to_sockaddr_size (Lisp_Object address, int *familyp)
{
  register struct Lisp_Vector *p;

  if (VECTORP (address))
    {
      p = XVECTOR (address);
      if (p->header.size == 5)
	{
	  *familyp = AF_INET;
	  return sizeof (struct sockaddr_in);
	}
#ifdef AF_INET6
      else if (p->header.size == 9)
	{
	  *familyp = AF_INET6;
	  return sizeof (struct sockaddr_in6);
	}
#endif
    }

So the list should be 8 8-bit numbers, not 4 16-bit numbers.  So I think
the error is in both conv_sockaddr_to_lisp (which should create 8 byte
arrays) and conv_lisp_to_sockaddr (which should treat the numbers as
bytes).

I think?

Weird that this hasn't been seen before at all...  aren't anybody using
IPv6?

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

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Hm...  The way IPv6 addresses in the IP lists are distinguished from
> IPv4 addresses is by the length of the lists.  For instance:

D'oh.  Brain fart on my part.  IPv6 is 128 bit, not 64 bit.  So I think
your patch is completely correct.

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

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[John Wiegley]

>>>>> David Edmondson <dme@dme.org> writes:

> When trying to test Lars' async-dns changes I encountered memory corruption
> whenever the addresses returned for a host included IPv6 addresses. This
> turns out to be a bug in the mainline code which is just more likely to be
> tickled by the async-dns changes because they convert addresses to lisp
> objects whenever `make-network-process' is called (the current code doesn't
> do that).

Lars, it's the discovery of bugs like this that lead me to conclude two
things:

 1. I'd like much more testing of this feature before we consider it for
    release.

 2. We need a suite of tests for this feature, because it makes a rather
    fundamental change, and I'd like some coverage to ensure that others
    working in similar areas won't break it in future.

-- 
John Wiegley                  GPG fingerprint = 4710 CF98 AF9B 327B B80F
http://newartisans.com                          60E1 46C4 BD1A 7AC1 4BA2

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Lars Ingebrigtsen]

John Wiegley <jwiegley@gmail.com> writes:

>  2. We need a suite of tests for this feature, because it makes a rather
>     fundamental change, and I'd like some coverage to ensure that others
>     working in similar areas won't break it in future.

Sure.  Network tests have to be done locally though, right?  I don't
think we have the infrastructure to set up a TLS server in Emacs, so
that part is kinda hard to test, but there's a lot of other tests we can
do with Emacs as both server and client, of course.

If we were to have tests that connected to real servers out there, that
would be even more realistic, but would be fragile, and not kind to
those servers, I guess.

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

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 08 Feb 2016 12:29:13 +1100
> 
> John Wiegley <jwiegley@gmail.com> writes:
> 
> >  2. We need a suite of tests for this feature, because it makes a rather
> >     fundamental change, and I'd like some coverage to ensure that others
> >     working in similar areas won't break it in future.
> 
> Sure.  Network tests have to be done locally though, right?  I don't
> think we have the infrastructure to set up a TLS server in Emacs, so
> that part is kinda hard to test, but there's a lot of other tests we can
> do with Emacs as both server and client, of course.

Can't something like that be done with Perl or Python, or even (gasp!)
with another copy of Emacs, on the same machine?

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> Sure.  Network tests have to be done locally though, right?  I don't
> think we have the infrastructure to set up a TLS server in Emacs, so
> that part is kinda hard to test, but there's a lot of other tests we can
> do with Emacs as both server and client, of course.

I've now added a bunch of network tests (that may perhaps be refined a
bit -- there's a few `sit-for's in there that may be fragile and result
in the tests failing on some systems, I guess).

Anybody have an idea as how to do TLS tests?  Er...  is there some
command-line functions that could be used to set up an ad-hoc TLS
server?

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

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Can't something like that be done with Perl or Python, or even (gasp!)
> with another copy of Emacs, on the same machine?

Unfortunately we haven't implemented support for TLS servers in our
gnutls.c.  :-/  We should, though.

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

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Lars Ingebrigtsen]

OpenSSL s_server seems to offer a pretty simple way to set up a TLS
server...

openssl s_server [-help] [-accept port] [-naccept count] [-context id] [-verify depth] [-Verify depth] [-crl_check] [-crl_check_all] [-cert filename] [-certform DER|PEM] [-key keyfile] [-keyform DER|PEM] [-pass arg] [-dcert filename] [-dcertform DER|PEM] [-dkey keyfile] [-dkeyform DER|PEM] [-dpass arg] [-dhparam filename] [-nbio] [-nbio_test] [-crlf] [-debug] [-msg] [-state] [-CApath directory] [-CAfile filename] [-no-CAfile] [-no-CApath] [-attime timestamp] [-check_ss_sig] [-explicit_policy] [-extended_crl] [-ignore_critical] [-inhibit_any] [-inhibit_map] [-issuer_checks] [-partial_chain] [-policy arg] [-policy_check] [-policy_print] [-purpose purpose] [-suiteB_128] [-suiteB_128_only] [-suiteB_192] [-trusted_first] [-no_alt_chains] [-use_deltas] [-verify_depth num] [-verify_return_error] [-verify_email email] [-verify_hostname hostname] [-verify_ip ip] [-verify_name name] [-x509_strict] [-nocert] [-cipher cipherlist] [-serverpref] [-quiet] [-ssl3] [-tls1] [-dtls] [-dtls1] [-dtls1_2] [-listen] [-async] [-no_ssl3] [-no_tls1] [-no_dhe] [-bugs] [-comp] [-no_comp] [-brief] [-www] [-WWW] [-HTTP] [-engine id] [-tlsextdebug] [-no_ticket] [-id_prefix arg] [-rand file(s)] [-serverinfo file] [-no_resumption_on_reneg] [-status] [-status_verbose] [-status_timeout nsec] [-status_url url] [-nextprotoneg protocols]

Eek.

But I'll test it out and see whether it's usable for our purposes.

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

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Lars Ingebrigtsen]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> But I'll test it out and see whether it's usable for our purposes.

Hey, that was amazingly easy to use.  :-)

How does one say in ert that a test shouldn't be run unless ... we
... should?  I mean, on systems without openssl, there's no point in
running tests that rely on it being there...

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

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Michael Albinus]

Lars Ingebrigtsen <larsi@gnus.org> writes:

> How does one say in ert that a test shouldn't be run unless ... we
> ... should?  I mean, on systems without openssl, there's no point in
> running tests that rely on it being there...

(skip-unless (executable-find "openssl"))

Best regards, Michael.

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Lars Ingebrigtsen]

Michael Albinus <michael.albinus@gmx.de> writes:

> Lars Ingebrigtsen <larsi@gnus.org> writes:
>
>> How does one say in ert that a test shouldn't be run unless ... we
>> ... should?  I mean, on systems without openssl, there's no point in
>> running tests that rely on it being there...
>
> (skip-unless (executable-find "openssl"))

Great; thanks.

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

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Mon, 08 Feb 2016 15:07:21 +1100
> Cc: emacs-devel@gnu.org
> 
> Lars Ingebrigtsen <larsi@gnus.org> writes:
> 
> > But I'll test [openssl] out and see whether it's usable for our purposes.
> 
> Hey, that was amazingly easy to use.  :-)

Is it possible to use gnutls-serv instead, or at least as an option?
You require Emacs to be compiled with GnuTLS, so I think the
probability of having gnutls-serv installed is higher than having
openssl, certainly on non-Posix hosts.  Besides, openssl is less
friendly to the GNU project, both legally and otherwise.

Please?

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Is it possible to use gnutls-serv instead, or at least as an option?
> You require Emacs to be compiled with GnuTLS, so I think the
> probability of having gnutls-serv installed is higher than having
> openssl, certainly on non-Posix hosts.  Besides, openssl is less
> friendly to the GNU project, both legally and otherwise.

Good idea.  I've now changed the test to use gnutls-serv.

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

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Tue, 09 Feb 2016 10:17:22 +1100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Is it possible to use gnutls-serv instead, or at least as an option?
> > You require Emacs to be compiled with GnuTLS, so I think the
> > probability of having gnutls-serv installed is higher than having
> > openssl, certainly on non-Posix hosts.  Besides, openssl is less
> > friendly to the GNU project, both legally and otherwise.
> 
> Good idea.  I've now changed the test to use gnutls-serv.

Thanks.  It seems to work fine on MS-Windows (after fixing a minor
gotcha), but I have a problem with connect-to-tls-ipv6-nowait: it
fails because the Windows build doesn't support IPv6, but if I force
it to be skipped, the next test, echo-server-nowait, fails because it
receives an empty string instead of "echo".  However, when
connect-to-tls-ipv6-nowait is left to run (and fail), then
echo-server-nowait succeeds.  So there seems to be some strange
connection between them, but I cannot see what is it.  Can you tell
what am I missing?

Thanks.

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> Thanks.  It seems to work fine on MS-Windows (after fixing a minor
> gotcha), but I have a problem with connect-to-tls-ipv6-nowait: it
> fails because the Windows build doesn't support IPv6, but if I force
> it to be skipped, the next test, echo-server-nowait, fails because it
> receives an empty string instead of "echo".

That's very strange.  If I remove connect-to-tls-ipv6-nowait from the
file and say "make lisp/net/network-stream-tests" then all the remaining
tests pass.

And echo-server-nowait doesn't talk to the same port at the TLS tests,
so that's doubly weird.

> However, when connect-to-tls-ipv6-nowait is left to run (and fail),
> then echo-server-nowait succeeds.

When it fails, is the gnutls-serv process still running?  While making
the tests, I noticed that sometimes the gnutls-serv processes were still
running -- even after the tests had finished, and the Emacs it had
spawned had died.  It's like gnutls-serv detaches itself from the parent
process.  Skimming the man page doesn't show any way to make it not do
that.

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

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Date: Wed, 10 Feb 2016 10:07:46 +1100
> Cc: emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Thanks.  It seems to work fine on MS-Windows (after fixing a minor
> > gotcha), but I have a problem with connect-to-tls-ipv6-nowait: it
> > fails because the Windows build doesn't support IPv6, but if I force
> > it to be skipped, the next test, echo-server-nowait, fails because it
> > receives an empty string instead of "echo".
> 
> That's very strange.  If I remove connect-to-tls-ipv6-nowait from the
> file and say "make lisp/net/network-stream-tests" then all the remaining
> tests pass.

Here, echo-server-nowait fails if I do that.

> And echo-server-nowait doesn't talk to the same port at the TLS tests,
> so that's doubly weird.

Yep.  Any ideas besides some weird timing problem?

> > However, when connect-to-tls-ipv6-nowait is left to run (and fail),
> > then echo-server-nowait succeeds.
> 
> When it fails, is the gnutls-serv process still running?

By "it" do you mean connect-to-tls-ipv6-nowait?  If so, AFAICT
gnutls-serv exits immediately when the failure is reported.

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Eli Zaretskii]

> Date: Wed, 10 Feb 2016 22:08:17 +0200
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > That's very strange.  If I remove connect-to-tls-ipv6-nowait from the
> > file and say "make lisp/net/network-stream-tests" then all the remaining
> > tests pass.
> 
> Here, echo-server-nowait fails if I do that.
> 
> > And echo-server-nowait doesn't talk to the same port at the TLS tests,
> > so that's doubly weird.
> 
> Yep.  Any ideas besides some weird timing problem?
> 
> > > However, when connect-to-tls-ipv6-nowait is left to run (and fail),
> > > then echo-server-nowait succeeds.
> > 
> > When it fails, is the gnutls-serv process still running?
> 
> By "it" do you mean connect-to-tls-ipv6-nowait?  If so, AFAICT
> gnutls-serv exits immediately when the failure is reported.

It's indeed some weird problem with timing or maybe with events in
batch mode.  The patch below (which also skips the IPv6 test on
MS-Windows) fixes it.  Do you see any problems with it?

diff --git a/test/lisp/net/network-stream-tests.el b/test/lisp/net/network-stream-tests.el
index 92c5370..d3f229e 100644
--- a/test/lisp/net/network-stream-tests.el
+++ b/test/lisp/net/network-stream-tests.el
@@ -192,6 +192,7 @@ make-tls-server
     (setq status (gnutls-peer-status proc))
     (should (consp status))
     (delete-process proc)
+    (sleep-for 0.1)
     (let ((issuer (plist-get (plist-get status :certificate) :issuer)))
       (should (stringp issuer))
       (setq issuer (split-string issuer ","))
@@ -200,6 +201,7 @@ make-tls-server
 (ert-deftest connect-to-tls-ipv6-nowait ()
   (skip-unless (executable-find "gnutls-serv"))
   (skip-unless (gnutls-available-p))
+  (skip-unless (not (eq system-type 'windows-nt)))
   (let ((server (make-tls-server))
         (times 0)
         proc status)
@@ -226,6 +228,7 @@ make-tls-server
     (setq status (gnutls-peer-status proc))
     (should (consp status))
     (delete-process proc)
+    (sleep-for 0.1)
     (let ((issuer (plist-get (plist-get status :certificate) :issuer)))
       (should (stringp issuer))
       (setq issuer (split-string issuer ","))

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Lars Ingebrigtsen]

Eli Zaretskii <eliz@gnu.org> writes:

> It's indeed some weird problem with timing or maybe with events in
> batch mode.  The patch below (which also skips the IPv6 test on
> MS-Windows) fixes it.  Do you see any problems with it?

No, looks good.  But it would also be good to find out why there are
these problems with networking (or events) in batch mode (on Windows),
if you find the time...

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

Re: [PATCH] fix conv_lisp_to_sockaddr for AF_INET6 addresses

[Eli Zaretskii]

> From: Lars Ingebrigtsen <larsi@gnus.org>
> Cc: emacs-devel@gnu.org
> Date: Thu, 11 Feb 2016 14:54:07 +1100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > It's indeed some weird problem with timing or maybe with events in
> > batch mode.  The patch below (which also skips the IPv6 test on
> > MS-Windows) fixes it.  Do you see any problems with it?
> 
> No, looks good.

OK, I pushed it.

> But it would also be good to find out why there are these problems
> with networking (or events) in batch mode (on Windows), if you find
> the time...

I will add this to my todo.

Thanks.