unofficial mirror of guile-devel@gnu.org 
 help / color / mirror / Atom feed
* [PATCH] Allow null bytes in UNIX sockets.
@ 2021-03-29 15:37 Leo Prikler
  2021-03-29 19:26 ` Maxime Devos
  0 siblings, 1 reply; 3+ messages in thread
From: Leo Prikler @ 2021-03-29 15:37 UTC (permalink / raw)
  To: guile-devel

The current socket address constructors all assume, that there are no
null bytes in the socket path.  This assumption does not hold in Linux,
which uses an initial null byte to demarcate abstract sockets and
ignores all further null bytes [1].

[1] https://www.man7.org/linux/man-pages/man7/unix.7.html

* libguile/sockets.c (scm_fill_sockaddr)[HAVE_UNIX_DOMAIN_SOCKETS]:
Use scm_to_locale_stringn to construct c_address.
Use memcpy instead of strcpy and calculate size directly instead of
using SUN_LEN.
(_scm_from_sockaddr): Copy the entire path up to the limits imposed by
addr_size.
* test-suite/tests/00-socket.test: ("make-socket-address"): Add case for
abstract unix sockets.
("AF_UNIX/SOCK_STREAM"): Add abstract socket versions of bind, listen,
connect and accept.
---
 libguile/socket.c               | 18 +++++---
 test-suite/tests/00-socket.test | 78 +++++++++++++++++++++++++++++++--
 2 files changed, 86 insertions(+), 10 deletions(-)

diff --git a/libguile/socket.c b/libguile/socket.c
index 9b87c0c14..ffb3de8a1 100644
--- a/libguile/socket.c
+++ b/libguile/socket.c
@@ -804,10 +804,11 @@ scm_fill_sockaddr (int fam, SCM address, SCM *args, int which_arg,
 	struct sockaddr_un *soka;
 	int addr_size;
 	char *c_address;
+	size_t c_address_size;
 
 	scm_dynwind_begin (0);
 
-	c_address = scm_to_locale_string (address);
+	c_address = scm_to_locale_stringn (address, &c_address_size);
 	scm_dynwind_free (c_address);
 
 	/* the static buffer size in sockaddr_un seems to be arbitrary
@@ -817,12 +818,14 @@ scm_fill_sockaddr (int fam, SCM address, SCM *args, int which_arg,
 	   connect/bind etc., to fail.  sun_path is always the last
 	   member of the structure.  */
 	addr_size = sizeof (struct sockaddr_un)
-	  + MAX (0, strlen (c_address) + 1 - (sizeof soka->sun_path));
+	  + MAX (0, c_address_size + 1 - (sizeof soka->sun_path));
 	soka = (struct sockaddr_un *) scm_malloc (addr_size);
-	memset (soka, 0, addr_size);  /* for sun_len: see sin_len above. */
+	memset (soka, 0, addr_size);
 	soka->sun_family = AF_UNIX;
-	strcpy (soka->sun_path, c_address);
-	*size = SUN_LEN (soka);
+        /* we accept 0-bytes here (used for abstract sockets in Linux);
+           therefore do not use strlen() or SUN_LEN!  */
+	memcpy (soka->sun_path, c_address, c_address_size);
+	*size = offsetof (struct sockaddr_un, sun_path) + c_address_size;
 
 	scm_dynwind_end ();
 	return (struct sockaddr *) soka;
@@ -1036,7 +1039,10 @@ _scm_from_sockaddr (const scm_t_max_sockaddr *address, unsigned addr_size,
 	if (addr_size <= offsetof (struct sockaddr_un, sun_path))
 	  SCM_SIMPLE_VECTOR_SET(result, 1, SCM_BOOL_F);
 	else
-	  SCM_SIMPLE_VECTOR_SET(result, 1, scm_from_locale_string (nad->sun_path));
+          {
+            size_t path_size = addr_size - offsetof (struct sockaddr_un, sun_path);
+            SCM_SIMPLE_VECTOR_SET(result, 1, scm_from_locale_stringn(nad->sun_path, path_size));
+          }
       }
       break;
 #endif
diff --git a/test-suite/tests/00-socket.test b/test-suite/tests/00-socket.test
index 027bd8519..5196b4b7d 100644
--- a/test-suite/tests/00-socket.test
+++ b/test-suite/tests/00-socket.test
@@ -128,10 +128,15 @@
 	       (= (sockaddr:flowinfo sa*) 1)))))
 
   (if (defined? 'AF_UNIX)
-      (pass-if "AF_UNIX"
-	(let ((sa (make-socket-address AF_UNIX "/tmp/unix-socket")))
-	  (and (= (sockaddr:fam sa) AF_UNIX)
-	       (string=? (sockaddr:path sa) "/tmp/unix-socket"))))))
+      (begin
+        (pass-if "AF_UNIX"
+	  (let ((sa (make-socket-address AF_UNIX "/tmp/unix-socket")))
+	    (and (= (sockaddr:fam sa) AF_UNIX)
+	         (string=? (sockaddr:path sa) "/tmp/unix-socket"))))
+        (pass-if "AF_UNIX abstract"
+          (let ((sa (make-socket-address AF_UNIX "\x00/tmp/abstract-socket")))
+	    (and (= (sockaddr:fam sa) AF_UNIX)
+	         (string=? (sockaddr:path sa) "\x00/tmp/abstract-socket")))))))
 
 ;;;
 ;;; setsockopt
@@ -319,6 +324,71 @@
 
 	#t)
 
+      ;; testing `bind', `listen' and `connect' on abstract stream-oriented sockets
+
+      (let ((server-socket (socket AF_UNIX SOCK_STREAM 0))
+	    (server-bound? #f)
+	    (server-listening? #f)
+	    (server-pid #f)
+	    (path (temp-file-path)))
+
+	(false-if-exception (delete-file path))
+        (set! path (string-append "\x00" path))
+
+	(pass-if "bind abstract"
+	  (catch 'system-error
+	    (lambda ()
+	      (bind server-socket AF_UNIX path)
+	      (set! server-bound? #t)
+	      #t)
+	    (lambda args
+	      (let ((errno (system-error-errno args)))
+		(cond ((= errno EADDRINUSE) (throw 'unresolved))
+		      (else (apply throw args)))))))
+
+	(pass-if "listen abstract"
+	  (if (not server-bound?)
+	      (throw 'unresolved)
+	      (begin
+		(listen server-socket 123)
+		(set! server-listening? #t)
+		#t)))
+
+	(force-output (current-output-port))
+	(force-output (current-error-port))
+	(when server-listening?
+          (let ((pid (primitive-fork-if-available)))
+	    ;; Spawn a server process.
+	    (case pid
+	      ((-1)  ;; fork not available
+               #f)
+	      ((0)   ;; the kid:  serve one connection and exit
+	       (let serve ((conn
+			    (false-if-exception (accept server-socket)))
+			   (count 0))
+		 (if (not conn)
+		     (exit 1)
+	             (exit 0))))
+	      (else  ;; the parent
+	       (set! server-pid pid)
+	       #t))))
+
+	(pass-if "connect abstract"
+	  (if (not server-pid)
+	      (throw 'unresolved)
+	      (let ((s (socket AF_UNIX SOCK_STREAM 0)))
+                (display "connect abstract\n")
+		(connect s AF_UNIX path)
+		#t)))
+
+	(pass-if "accept abstract"
+	  (if (not server-pid)
+	      (throw 'unresolved)
+              (begin
+	        (let ((status (cdr (waitpid server-pid))))
+		  (eqv? 0 (status:exit-val status))))))
+
+	#t)
 
       ;; Testing `send', `recv!' & co. on stream-oriented sockets (with
       ;; a bit of duplication with the above.)
-- 
2.31.0




^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] Allow null bytes in UNIX sockets.
  2021-03-29 15:37 [PATCH] Allow null bytes in UNIX sockets Leo Prikler
@ 2021-03-29 19:26 ` Maxime Devos
  2021-05-15 16:30   ` Leo Prikler
  0 siblings, 1 reply; 3+ messages in thread
From: Maxime Devos @ 2021-03-29 19:26 UTC (permalink / raw)
  To: Leo Prikler, guile-devel

[-- Attachment #1: Type: text/plain, Size: 1684 bytes --]

On Mon, 2021-03-29 at 17:37 +0200, Leo Prikler wrote:
> The current socket address constructors all assume, that there are no
> null bytes in the socket path.  This assumption does not hold in Linux,
> which uses an initial null byte to demarcate abstract sockets and
> ignores all further null bytes [1].
> 
> [1] https://www.man7.org/linux/man-pages/man7/unix.7.html
> 

This is necessary to connect to dbus (I forgot the proper capitalisation)
in some set-ups.  I tried implementing this at some point, but Guile
crashed and I gave up.

Thank you for looking into this!  One comment below.

> [...]
> diff --git a/test-suite/tests/00-socket.test b/test-suite/tests/00-socket.test
> index 027bd8519..5196b4b7d 100644
> --- a/test-suite/tests/00-socket.test
> +++ b/test-suite/tests/00-socket.test
> @@ -128,10 +128,15 @@
>  	       (= (sockaddr:flowinfo sa*) 1)))))
>  
>    (if (defined? 'AF_UNIX)
> -      (pass-if "AF_UNIX"
> -	(let ((sa (make-socket-address AF_UNIX "/tmp/unix-socket")))
> -	  (and (= (sockaddr:fam sa) AF_UNIX)
> -	       (string=? (sockaddr:path sa) "/tmp/unix-socket"))))))
> +      (begin
> +        (pass-if "AF_UNIX"
> +	  (let ((sa (make-socket-address AF_UNIX "/tmp/unix-socket")))
> +	    (and (= (sockaddr:fam sa) AF_UNIX)
> +	         (string=? (sockaddr:path sa) "/tmp/unix-socket"))))
> +        (pass-if "AF_UNIX abstract"
> +          (let ((sa (make-socket-address AF_UNIX "\x00/tmp/abstract-socket")))
> +	    (and (= (sockaddr:fam sa) AF_UNIX)
> +	         (string=? (sockaddr:path sa) "\x00/tmp/abstract-socket")))))))

Shouldn't this code use $TMPDIR or some variable like that?

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] Allow null bytes in UNIX sockets.
  2021-03-29 19:26 ` Maxime Devos
@ 2021-05-15 16:30   ` Leo Prikler
  0 siblings, 0 replies; 3+ messages in thread
From: Leo Prikler @ 2021-05-15 16:30 UTC (permalink / raw)
  To: Maxime Devos, Leo Prikler, guile-devel

Sorry for the late reply.  It appears that mail got lost in some server
and I had to manually retrieve it.

Am Montag, den 29.03.2021, 21:26 +0200 schrieb Maxime Devos:
> On Mon, 2021-03-29 at 17:37 +0200, Leo Prikler wrote:
> > The current socket address constructors all assume, that there are
> > no
> > null bytes in the socket path.  This assumption does not hold in
> > Linux,
> > which uses an initial null byte to demarcate abstract sockets and
> > ignores all further null bytes [1].
> > 
> > [1] https://www.man7.org/linux/man-pages/man7/unix.7.html
> > 
> 
> This is necessary to connect to dbus (I forgot the proper
> capitalisation)
> in some set-ups.  I tried implementing this at some point, but Guile
> crashed and I gave up.
> 
> Thank you for looking into this!  One comment below.
> 
> > [...]
> > diff --git a/test-suite/tests/00-socket.test b/test-suite/tests/00-
> > socket.test
> > index 027bd8519..5196b4b7d 100644
> > --- a/test-suite/tests/00-socket.test
> > +++ b/test-suite/tests/00-socket.test
> > @@ -128,10 +128,15 @@
> >  	       (= (sockaddr:flowinfo sa*) 1)))))
> >  
> >    (if (defined? 'AF_UNIX)
> > -      (pass-if "AF_UNIX"
> > -	(let ((sa (make-socket-address AF_UNIX "/tmp/unix-socket")))
> > -	  (and (= (sockaddr:fam sa) AF_UNIX)
> > -	       (string=? (sockaddr:path sa) "/tmp/unix-socket"))))))
> > +      (begin
> > +        (pass-if "AF_UNIX"
> > +	  (let ((sa (make-socket-address AF_UNIX "/tmp/unix-socket")))
> > +	    (and (= (sockaddr:fam sa) AF_UNIX)
> > +	         (string=? (sockaddr:path sa) "/tmp/unix-socket"))))
> > +        (pass-if "AF_UNIX abstract"
> > +          (let ((sa (make-socket-address AF_UNIX
> > "\x00/tmp/abstract-socket")))
> > +	    (and (= (sockaddr:fam sa) AF_UNIX)
> > +	         (string=? (sockaddr:path sa) "\x00/tmp/abstract-
> > socket")))))))
> 
> Shouldn't this code use $TMPDIR or some variable like that?
Those "/tmp"s already exist within Guile source, so meh.  I don't think
there's a difference with abstract sockets, since they don't exist in
the filesystem anyway.

Regards,
Leo




^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-05-15 16:30 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-29 15:37 [PATCH] Allow null bytes in UNIX sockets Leo Prikler
2021-03-29 19:26 ` Maxime Devos
2021-05-15 16:30   ` Leo Prikler

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).