Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PATCH] socket SOCK_CLOEXEC #15889

Open
p5pRT opened this issue Feb 24, 2017 · 9 comments
Open

[PATCH] socket SOCK_CLOEXEC #15889

p5pRT opened this issue Feb 24, 2017 · 9 comments

Comments

@p5pRT
Copy link

p5pRT commented Feb 24, 2017

Migrated from rt.perl.org#130851 (status was 'open')

Searchable as RT130851$

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2017

From alexander.bluhm@gmx.net

Created by alexander.bluhm@gmx.net

When analyzing system calls, I found that Perl socket always does
an additional fcntl(F_SETFD,FD_CLOEXEC). Modern socket(2) syscalls
support the SOCK_CLOEXEC flag to do this atomically. In general
this avoids a threading race, I did it to save the additional
syscall. I had to add logic for socket descriptor 0, 1, 2, but
this case should not happen in practice.

I know that my patch is only a micro optimization, but it should
not do any harm.

I have startet to implement this for socket(2). If you agree that
this is useful, I will also do it for similar operations. I have
started a new test file t/io/cloexec.t so I can add other cases
later.

To get all tests pass, I had to adjust my email in AUTHORS

Perl Info

Flags:
    category=core
    severity=low
    Type=Patch
    PatchStatus=HasPatch

Site configuration information for perl 5.25.11:

Configured by bluhm at Thu Feb 23 16:17:51 CET 2017.

Summary of my perl5 (revision 5 version 25 subversion 11) configuration:
  Commit id: 6ea25c1667c047465291ad52a7ff90f00bc97777
  Platform:
    osname=openbsd
    osvers=6.0
    archname=OpenBSD.amd64-openbsd
    uname='openbsd t430s.bluhm.invalid 6.0 generic.mp#2 amd64 '
    config_args='-de -Dusedevel'
    hint=previous
    useposix=true
    d_sigaction=define
    useithreads=undef
    usemultiplicity=undef
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    bincompat5005=undef
  Compiler:
    cc='cc'
    ccflags ='-fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include -D_FORTIFY_SOURCE=2'
    optimize='-O2'
    cppflags='-fno-strict-aliasing -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion=''
    gccversion='4.2.1 20070719 '
    gccosandvers='openbsd6.0'
    intsize=4
    longsize=8
    ptrsize=8
    doublesize=8
    byteorder=12345678
    doublekind=3
    d_longlong=define
    longlongsize=8
    d_longdbl=define
    longdblsize=16
    longdblkind=3
    ivtype='long'
    ivsize=8
    nvtype='double'
    nvsize=8
    Off_t='off_t'
    lseeksize=8
    alignbytes=8
    prototype=define
  Linker and Libraries:
    ld='cc'
    ldflags ='-Wl,-E  -fstack-protector-strong -L/usr/local/lib'
    libpth=/usr/lib /usr/local/lib /usr/lib
    libs=-lpthread -lgdbm -lm -lutil -lc
    perllibs=-lpthread -lm -lutil -lc
    libc=/usr/lib/libc.a
    so=so
    useshrplib=false
    libperl=libperl.a
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=so
    d_dlsymun=undef
    ccdlflags=' '
    cccdlflags='-DPIC -fPIC '
    lddlflags='-shared -fPIC  -L/usr/local/lib -fstack-protector-strong'



@INC for perl 5.25.11:
    lib
    /home/bluhm/localperl/lib/site_perl/5.25.10/OpenBSD.amd64-openbsd
    /home/bluhm/localperl/lib/site_perl/5.25.10
    /home/bluhm/localperl/lib/5.25.10/OpenBSD.amd64-openbsd
    /home/bluhm/localperl/lib/5.25.10


Environment for perl 5.25.11:
    HOME=/home/bluhm
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/bluhm/bin:/bin:/sbin:/usr/bin:/usr/sbin:/usr/X11R6/bin:/usr/local/bin:/usr/local/sbin:/usr/games:/usr/ports/infrastructure/bin
    PERL_BADLANG (unset)
    SHELL=/bin/ksh

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2017

From alexander.bluhm@gmx.net

0001-Use-SOCK_CLOEXEC-when-creating-a-socket.patch
From 6ea25c1667c047465291ad52a7ff90f00bc97777 Mon Sep 17 00:00:00 2001
From: Alexander Bluhm <bluhm@cpan.org>
Date: Tue, 21 Feb 2017 19:12:29 +0100
Subject: [PATCH] Use SOCK_CLOEXEC when creating a socket

If the system provides the SOCK_CLOEXEC close-on-exec flag for
sockets, use this feature to avoid an unnecessary fnctl(F_SETFD)
system call.  In the very uncommon case that a system file descriptor
is created, clear the FD_CLOEXEC flag afterwards.
---
 AUTHORS                 |  2 +-
 MANIFEST                |  1 +
 Porting/checkAUTHORS.pl |  3 +++
 pp_sys.c                | 10 +++++++++-
 t/io/cloexec.t          | 43 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 57 insertions(+), 2 deletions(-)
 create mode 100644 t/io/cloexec.t

diff --git a/AUTHORS b/AUTHORS
index 4e4756b494..260ea42073 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -60,7 +60,7 @@ Alex Solovey			<a.solovey@gmail.com>
 Alex Vandiver			<alexmv@mit.edu>
 Alex Waugh			<alex@alexwaugh.com>
 Alexander Alekseev		<alex@alemate.ru>
-Alexander Bluhm			<alexander_bluhm@genua.de>
+Alexander Bluhm			<bluhm@cpan.org>
 Alexander D'Archangel		<darksuji@gmail.com>
 Alexander Gernler		<alexander_gernler@genua.de>
 Alexander Gough			<alex-p5p@earth.li>
diff --git a/MANIFEST b/MANIFEST
index ef1d98472e..605ff2eb91 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -5309,6 +5309,7 @@ t/harness			Finer diagnostics from test suite
 t/io/argv.t			See if ARGV stuff works
 t/io/binmode.t			See if binmode() works
 t/io/bom.t			See if scripts can start with a byte order mark
+t/io/cloexec.t			See if file descriptor has FD_CLOEXEC flag
 t/io/closepid.t			See if close works for subprocesses
 t/io/crlf.t			See if :crlf works
 t/io/crlf_through.t		See if pipe passes data intact with :crlf
diff --git a/Porting/checkAUTHORS.pl b/Porting/checkAUTHORS.pl
index cb8863c79a..bdd179924b 100755
--- a/Porting/checkAUTHORS.pl
+++ b/Porting/checkAUTHORS.pl
@@ -563,6 +563,9 @@ bert\100alum.mit.edu                    bert\100genscan.com
 bigbang7\100gmail.com                   ddascalescu+github\100gmail.com
 blgl\100stacken.kth.se                  blgl\100hagernas.com
 +                                       2bfjdsla52kztwejndzdstsxl9athp\100gmail.com
+bluhm\100cpan.org                       alexander.bluhm\100gmx.net
++                                       alexander_bluhm\100genua.de
++                                       bluhm\100openbsd.org
 brian.d.foy\100gmail.com                bdfoy\100cpan.org
 BQW10602\100nifty.com                   sadahiro\100cpan.org
 bulk88\100hotmail.com                   bulk88
diff --git a/pp_sys.c b/pp_sys.c
index 7a5703515c..d8c004afd1 100644
--- a/pp_sys.c
+++ b/pp_sys.c
@@ -2512,7 +2512,7 @@ PP(pp_socket)
 {
     dSP;
     const int protocol = POPi;
-    const int type = POPi;
+    int type = POPi;
     const int domain = POPi;
     GV * const gv = MUTABLE_GV(POPs);
     IO * const io = GvIOn(gv);
@@ -2521,6 +2521,9 @@ PP(pp_socket)
     if (IoIFP(io))
 	do_close(gv, FALSE);
 
+#if defined(SOCK_CLOEXEC)
+    type |= SOCK_CLOEXEC;
+#endif
     TAINT_PROPER("socket");
     fd = PerlSock_socket(domain, type, protocol);
     if (fd < 0) {
@@ -2536,10 +2539,15 @@ PP(pp_socket)
 	RETPUSHUNDEF;
     }
 #if defined(HAS_FCNTL) && defined(F_SETFD) && defined(FD_CLOEXEC)
+#if defined(SOCK_CLOEXEC)
+    if (fd <= PL_maxsysfd && fcntl(fd, F_SETFD, 0) < 0)
+	RETPUSHUNDEF;
+#else
     /* ensure close-on-exec */
     if (fd > PL_maxsysfd && fcntl(fd, F_SETFD, FD_CLOEXEC) < 0)
 	RETPUSHUNDEF;
 #endif
+#endif
 
     RETPUSHYES;
 }
diff --git a/t/io/cloexec.t b/t/io/cloexec.t
new file mode 100644
index 0000000000..1dc695b50d
--- /dev/null
+++ b/t/io/cloexec.t
@@ -0,0 +1,43 @@
+#!perl
+
+# test wether close-on-exec file descriptor flag is set initially
+
+BEGIN {
+    chdir 't' if -d 't';
+    @INC = '../lib';
+    require "./test.pl";
+    skip_all_if_miniperl();
+    skip_all_without_config('d_fcntl');
+    require Config; import Config;
+    require Fcntl; import Fcntl, qw(F_GETFD FD_CLOEXEC);
+}
+
+use strict;
+
+plan tests => 8;
+
+SKIP: {
+    $Config{d_socket} or skip("No socket", 3);
+    $Config{extensions} =~ /\bSocket\b/ or skip("No Socket", 3);
+    require Socket; import Socket, qw(PF_INET SOCK_STREAM);
+    ok(socket(my $sock, Socket::PF_INET(), Socket::SOCK_STREAM(), 0),
+	"socket", "socket PF_INET SOCK_STREAM failed: $!");
+    my $flags = fcntl($sock, F_GETFD, 0);
+    ok($flags, "fcntl socket", "fcntl F_GETFD failed: $!");
+    is($flags & FD_CLOEXEC, FD_CLOEXEC, "socket FD_CLOEXEC",
+	"FD_CLOEXEC not set: $flags");
+}
+
+SKIP: {
+    $Config{d_socket} or skip("No socket", 5);
+    $Config{extensions} =~ /\bSocket\b/ or skip("No Socket", 5);
+    require Socket; import Socket, qw(PF_INET SOCK_STREAM);
+    ok(close(STDIN), "close STDIN failed: $!");
+    ok(socket(my $sock, Socket::PF_INET(), Socket::SOCK_STREAM(), 0),
+	"socket", "socket PF_INET SOCK_STREAM failed: $!");
+    is($sock->fileno(), 0, "socket file number not zero with closed stdin");
+    my $flags = fcntl($sock, F_GETFD, 0);
+    ok($flags, "fcntl socket", "fcntl F_GETFD failed: $!");
+    is($flags & FD_CLOEXEC, 0, "socket FD_CLOEXEC",
+	"FD_CLOEXEC set: $flags");
+}
-- 
2.11.1

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2017

From zefram@fysh.org

Alexander Bluhm wrote​:

When analyzing system calls, I found that Perl socket always does
an additional fcntl(F_SETFD,FD_CLOEXEC). Modern socket(2) syscalls
support the SOCK_CLOEXEC flag to do this atomically.

It is certainly a good idea to use SOCK_CLOEXEC where it is available,
but more logic is required than your patch supplies. Having the
SOCK_CLOEXEC macro defined at compile time does not guarantee that the
flag will be supported by the kernel at runtime. If it's not supported,
then socket(2) may either yield EINVAL or possibly ignore the flag,
returning a non-CLOEXEC fd. It is necessary to check for these outcomes
at runtime, checking for the latter by an fcntl(2) F_GETFD, and fall back
to the non-SOCK_CLOEXEC behaviour if it's not supported. The check need
only be done on the first socket(2) call​: if SOCK_CLOEXEC worked once
then one can assume that it'll work for subsequent calls in the same run.

Similar comments apply to the use of O_CLOEXEC with open(2), for which
a corresponding patch would be welcome.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Feb 24, 2017

The RT System itself - Status changed from 'new' to 'open'

@p5pRT
Copy link
Author

p5pRT commented Feb 25, 2017

From zefram@fysh.org

The logic required is approximately​:

  int socket_cloexec(int domain, int type, int protocol)
  {
  int sock;
  #ifdef SOCK_CLOEXEC
  static int strategy = 0;
  switch (strategy) {
  case 0​:
  sock = socket(domain, type | SOCK_CLOEXEC, protocol);
  if (sock != -1) {
  int fdflags = fcntl(sock, F_GETFD);
  if (fdflags != -1 && (fdflags & FD_CLOEXEC)) {
  strategy = 1;
  } else {
  strategy = 2;
  fcntl(sock, F_SETFD, FD_CLOEXEC);
  }
  } else if (errno == EINVAL) {
  sock = socket(domain, type, protocol);
  if (sock != -1) {
  strategy = 2;
  fcntl(sock, F_SETFD, FD_CLOEXEC);
  } else if (errno != EINVAL) {
  strategy = 2;
  }
  }
  return sock;
  case 1​:
  return socket(domain, type | SOCK_CLOEXEC, protocol);
  }
  #endif
  sock = socket(domain, type, protocol);
  if (sock != -1)
  fcntl(sock, F_SETFD, FD_CLOEXEC);
  return sock;
  }

I haven't tested this code, but it's based on similar code that I wrote
for other syscalls in Hash​::SharedMem. I recommend separating the code
out in this manner, into a socket_cloexec() function, even if there's
only one caller.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2017

From @jkeenan

On Fri, 24 Feb 2017 13​:12​:12 GMT, alexander.bluhm@​gmx.net wrote​:

This is a bug report for perl from alexander.bluhm@​gmx.net,
generated with the help of perlbug 1.40 running under perl 5.25.11.

-----------------------------------------------------------------
[Please describe your issue here]

When analyzing system calls, I found that Perl socket always does
an additional fcntl(F_SETFD,FD_CLOEXEC). Modern socket(2) syscalls
support the SOCK_CLOEXEC flag to do this atomically. In general
this avoids a threading race, I did it to save the additional
syscall. I had to add logic for socket descriptor 0, 1, 2, but
this case should not happen in practice.

I know that my patch is only a micro optimization, but it should
not do any harm.

I have startet to implement this for socket(2). If you agree that
this is useful, I will also do it for similar operations. I have
started a new test file t/io/cloexec.t so I can add other cases
later.

To get all tests pass, I had to adjust my email in AUTHORS

Given code freeze, I'm marking this ticket and its patch for consideration post-5.26.0 release.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Feb 26, 2017

From [Unknown Contact. See original ticket]

On Fri, 24 Feb 2017 13​:12​:12 GMT, alexander.bluhm@​gmx.net wrote​:

This is a bug report for perl from alexander.bluhm@​gmx.net,
generated with the help of perlbug 1.40 running under perl 5.25.11.

-----------------------------------------------------------------
[Please describe your issue here]

When analyzing system calls, I found that Perl socket always does
an additional fcntl(F_SETFD,FD_CLOEXEC). Modern socket(2) syscalls
support the SOCK_CLOEXEC flag to do this atomically. In general
this avoids a threading race, I did it to save the additional
syscall. I had to add logic for socket descriptor 0, 1, 2, but
this case should not happen in practice.

I know that my patch is only a micro optimization, but it should
not do any harm.

I have startet to implement this for socket(2). If you agree that
this is useful, I will also do it for similar operations. I have
started a new test file t/io/cloexec.t so I can add other cases
later.

To get all tests pass, I had to adjust my email in AUTHORS

Given code freeze, I'm marking this ticket and its patch for consideration post-5.26.0 release.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Jun 1, 2017

From @jkeenan

On Sun, 26 Feb 2017 21​:31​:55 GMT, jkeenan wrote​:

On Fri, 24 Feb 2017 13​:12​:12 GMT, alexander.bluhm@​gmx.net wrote​:

This is a bug report for perl from alexander.bluhm@​gmx.net,
generated with the help of perlbug 1.40 running under perl 5.25.11.

-----------------------------------------------------------------
[Please describe your issue here]

When analyzing system calls, I found that Perl socket always does
an additional fcntl(F_SETFD,FD_CLOEXEC). Modern socket(2) syscalls
support the SOCK_CLOEXEC flag to do this atomically. In general
this avoids a threading race, I did it to save the additional
syscall. I had to add logic for socket descriptor 0, 1, 2, but
this case should not happen in practice.

I know that my patch is only a micro optimization, but it should
not do any harm.

I have startet to implement this for socket(2). If you agree that
this is useful, I will also do it for similar operations. I have
started a new test file t/io/cloexec.t so I can add other cases
later.

To get all tests pass, I had to adjust my email in AUTHORS

Given code freeze, I'm marking this ticket and its patch for
consideration post-5.26.0 release.

With perl-5.26.0 having been released, this ticket is now up for consideration.

Thank you very much.

--
James E Keenan (jkeenan@​cpan.org)

@p5pRT
Copy link
Author

p5pRT commented Dec 22, 2017

From zefram@fysh.org

Fixed with a comprehensive set of close-on-exec changes in commit
842c213.

-zefram

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants