Only cache io_socket_proto if protocol is truthy #17157

p5pRT · 2019-09-23T01:03:11Z

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

Searchable as RT134446$

p5pRT · 2019-09-23T01:03:12Z

From afresh1@openbsd.org

Created by andrew@afresh1.com

When OpenBSD implemented support for SO_PROTOCOL to getsockopt(2)[1],
suddenly IO/t/cachepropagate-unix.t started failing the 'protocol match'
tests for `new_from_fd` sockets.

I had trouble deciding on whether to actually just remove the entire
cache of protocol from "sub socket" or just to avoid caching it when it
is falsy. This may cause $sock->protocol to be undef on some systems
that don't support SO_PROTOCOL where it was 0 before, but that is
arguably more correct.

In either case, this causes this test to pass because
$listener->protocol and $new->protocol now match.

$ perl dist/IO/t/cachepropagate-unix.t
1..15
ok 1 - stream socket created
ok 2 - protocol defined
ok 3 - domain defined
ok 4 - type defined
ok 5 - spawned a child
ok 6 - domain match
ok 7 - protocol match
ok 8 - type match
ok 9 - datagram socket created
ok 10 - protocol defined
ok 11 - domain defined
ok 12 - type defined
ok 13 - domain match
not ok 14 - protocol match
# Failed test 'protocol match'
# at dist/IO/t/cachepropagate-unix.t line 106.
# got: '1'
# expected: '0'
ok 15 - type match
# Looks like you failed 1 test of 15.

This test program on OpenBSD-current prints without the attached patch:

guess: 0
proto: 1

--- BEGIN testprogram.pl ---

#!/usr/bin/perl;
use strict;
use warnings;

use IO::Socket::UNIX;

use File::Temp qw< tempdir >;

my $socketpath = tempdir( CLEANUP => 1 ) . '/testsock';

my $old = IO::Socket::UNIX->new(Type => SOCK_DGRAM, Local => $socketpath);
print "guess: " . ( defined $old->protocol ? $old->protocol : 'undef' ) . "\n";
my $protocol = unpack "i", getsockopt($old, Socket::SOL_SOCKET, Socket::SO_PROTOCOL );
print "proto: " . ( defined $protocol ? $protocol : 'undef' ) . "\n";

unlink($socketpath);

--- END testprogram.pl ---

[1] openbsd/src@15b62b7

Perl Info


Flags:
    category=library
    severity=medium
    module=IO::Socket

Site configuration information for perl 5.30.0:

Configured by root at Thu Jan  1  0:00:00 UTC 1970.

Summary of my perl5 (revision 5 version 30 subversion 0) configuration:
   
  Platform:
    osname=openbsd
    osvers=6.6
    archname=amd64-openbsd
    uname='openbsd'
    config_args='-dsE -Dopenbsd_distribution=defined -Dccflags=-DNO_LOCALE_NUMERIC -DNO_LOCALE_COLLATE -Dmksymlinks'
    hint=recommended
    useposix=true
    d_sigaction=define
    useithreads=undef
    usemultiplicity=undef
    use64bitint=define
    use64bitall=define
    uselongdouble=undef
    usemymalloc=n
    default_inc_excludes_dot=define
    bincompat5005=undef
  Compiler:
    cc='cc'
    ccflags ='-DNO_LOCALE_NUMERIC -DNO_LOCALE_COLLATE -fno-strict-aliasing -fno-delete-null-pointer-checks -pipe -fstack-protector-strong -D_FORTIFY_SOURCE=2 -I/usr/local/include'
    optimize='-O2'
    cppflags='-DNO_LOCALE_NUMERIC -DNO_LOCALE_COLLATE -fno-strict-aliasing -fno-delete-null-pointer-checks -pipe -fstack-protector-strong -I/usr/local/include'
    ccversion=''
    gccversion='4.2.1 Compatible OpenBSD Clang 8.0.0 (tags/RELEASE_800/final)'
    gccosandvers=''
    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/lib
    libs=-lm -lc
    perllibs=-lm -lc
    libc=/usr/lib/libc.so.95.1
    so=so
    useshrplib=true
    libperl=libperl.so.20.0
    gnulibc_version=''
  Dynamic Linking:
    dlsrc=dl_dlopen.xs
    dlext=so
    d_dlsymun=undef
    ccdlflags='-Wl,-R/usr/libdata/perl5/amd64-openbsd/CORE'
    cccdlflags='-DPIC -fPIC '
    lddlflags='-shared -fPIC  -fstack-protector-strong -L/usr/local/lib'



@INC for perl 5.30.0:
    /usr/local/libdata/perl5/site_perl/amd64-openbsd
    /usr/local/libdata/perl5/site_perl
    /usr/libdata/perl5/amd64-openbsd
    /usr/libdata/perl5


Environment for perl 5.30.0:
    HOME=/home/afresh1
    LANG (unset)
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/home/afresh1/bin:/home/afresh1/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 · 2019-09-23T01:03:12Z

From afresh1@openbsd.org

0001-Only-cache-io_socket_proto-if-protocol-is-truthy.patch

From fb7c0942ed53c49d3bbe8d153622055b3433fb65 Mon Sep 17 00:00:00 2001
From: Andrew Hewus Fresh <afresh1@openbsd.org>
Date: Sun, 22 Sep 2019 17:36:47 -0700
Subject: [PATCH] Only cache io_socket_proto if protocol is truthy

According to socket(2):

    A value of 0 for protocol will let the system select an appropriate
    protocol for the requested socket type.

While linux, and possibly others, return 0 for this, OpenBSD recently
got support for SO_PROTOCOL in getsockopt(2) and instead of returning
0, indicating it will choose the correct protocol, it instead returns
the protocol that was chosen.  That means caching "0" as the chosen
protocol means that what $sock->protocol returns is not the same as what
getsockopt($sock, SOL_SOCKET, SO_PROTOCOL) returns.

While there is ongoing discussion about whether returning 0 vs something
else is correct, the current situation shows that this value is
implementation specific and we should not cache something that might not
be right.
---
 dist/IO/lib/IO/Socket.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/dist/IO/lib/IO/Socket.pm b/dist/IO/lib/IO/Socket.pm
index 28fa1ec149..0d93575ccf 100644
--- a/dist/IO/lib/IO/Socket.pm
+++ b/dist/IO/lib/IO/Socket.pm
@@ -82,7 +82,12 @@ sub socket {
 
     ${*$sock}{'io_socket_domain'} = $domain;
     ${*$sock}{'io_socket_type'}   = $type;
-    ${*$sock}{'io_socket_proto'}  = $protocol;
+
+    # "A value of 0 for protocol will let the system select an
+    # appropriate protocol"
+    # so we need to look up what the system selected,
+    # not cache PF_UNSPEC.
+    ${*$sock}{'io_socket_proto'} = $protocol if $protocol;
 
     $sock;
 }
-- 
2.22.0

p5pRT · 2019-09-23T01:58:25Z

From @jkeenan

On Mon, 23 Sep 2019 01:03:12 GMT, afresh1@openbsd.org wrote:

This is a bug report for perl from andrew@afresh1.com,
generated with the help of perlbug 1.41 running under perl 5.30.0.

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

When OpenBSD implemented support for SO_PROTOCOL to getsockopt(2)[1],
suddenly IO/t/cachepropagate-unix.t started failing the 'protocol
match'
tests for `new_from_fd` sockets.

I had trouble deciding on whether to actually just remove the entire
cache of protocol from "sub socket" or just to avoid caching it when
it
is falsy. This may cause $sock->protocol to be undef on some systems
that don't support SO_PROTOCOL where it was 0 before, but that is
arguably more correct.

In either case, this causes this test to pass because
$listener->protocol and $new->protocol now match.

$ perl dist/IO/t/cachepropagate-unix.t
1..15
ok 1 - stream socket created
ok 2 - protocol defined
ok 3 - domain defined
ok 4 - type defined
ok 5 - spawned a child
ok 6 - domain match
ok 7 - protocol match
ok 8 - type match
ok 9 - datagram socket created
ok 10 - protocol defined
ok 11 - domain defined
ok 12 - type defined
ok 13 - domain match
not ok 14 - protocol match
# Failed test 'protocol match'
# at dist/IO/t/cachepropagate-unix.t line 106.
# got: '1'
# expected: '0'
ok 15 - type match
# Looks like you failed 1 test of 15.

This test program on OpenBSD-current prints without the attached
patch:

guess: 0
proto: 1

--- BEGIN testprogram.pl ---

#!/usr/bin/perl;
use strict;
use warnings;

use IO::Socket::UNIX;

use File::Temp qw< tempdir >;

my $socketpath = tempdir( CLEANUP => 1 ) . '/testsock';

my $old = IO::Socket::UNIX->new(Type => SOCK_DGRAM, Local =>
$socketpath);
print "guess: " . ( defined $old->protocol ? $old->protocol : 'undef'
) . "\n";
my $protocol = unpack "i", getsockopt($old, Socket::SOL_SOCKET,
Socket::SO_PROTOCOL );
print "proto: " . ( defined $protocol ? $protocol : 'undef' ) . "\n";

unlink($socketpath);

--- END testprogram.pl ---

[1]
openbsd/src@15b62b7

Applied patch attached to a branch for smoke-testing:
smoke-me/jkeenan/afresh/134446-socket

Thank you very much.

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

p5pRT · 2019-09-23T01:58:25Z

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

p5pRT · 2019-09-23T11:45:26Z

From @jkeenan

On Mon, 23 Sep 2019 01:58:25 GMT, jkeenan wrote:

On Mon, 23 Sep 2019 01:03:12 GMT, afresh1@openbsd.org wrote:

This is a bug report for perl from andrew@afresh1.com,
generated with the help of perlbug 1.41 running under perl 5.30.0.

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

When OpenBSD implemented support for SO_PROTOCOL to getsockopt(2)[1],
suddenly IO/t/cachepropagate-unix.t started failing the 'protocol
match'
tests for `new_from_fd` sockets.

I had trouble deciding on whether to actually just remove the entire
cache of protocol from "sub socket" or just to avoid caching it when
it
is falsy. This may cause $sock->protocol to be undef on some systems
that don't support SO_PROTOCOL where it was 0 before, but that is
arguably more correct.

In either case, this causes this test to pass because
$listener->protocol and $new->protocol now match.

$ perl dist/IO/t/cachepropagate-unix.t
1..15
ok 1 - stream socket created
ok 2 - protocol defined
ok 3 - domain defined
ok 4 - type defined
ok 5 - spawned a child
ok 6 - domain match
ok 7 - protocol match
ok 8 - type match
ok 9 - datagram socket created
ok 10 - protocol defined
ok 11 - domain defined
ok 12 - type defined
ok 13 - domain match
not ok 14 - protocol match
# Failed test 'protocol match'
# at dist/IO/t/cachepropagate-unix.t line 106.
# got: '1'
# expected: '0'
ok 15 - type match
# Looks like you failed 1 test of 15.

This test program on OpenBSD-current prints without the attached
patch:

guess: 0
proto: 1

--- BEGIN testprogram.pl ---

#!/usr/bin/perl;
use strict;
use warnings;

use IO::Socket::UNIX;

use File::Temp qw< tempdir >;

my $socketpath = tempdir( CLEANUP => 1 ) . '/testsock';

my $old = IO::Socket::UNIX->new(Type => SOCK_DGRAM, Local =>
$socketpath);
print "guess: " . ( defined $old->protocol ? $old->protocol : 'undef'
) . "\n";
my $protocol = unpack "i", getsockopt($old, Socket::SOL_SOCKET,
Socket::SO_PROTOCOL );
print "proto: " . ( defined $protocol ? $protocol : 'undef' ) . "\n";

unlink($socketpath);

--- END testprogram.pl ---

[1]
openbsd/src@15b62b7

Applied patch attached to a branch for smoke-testing:
smoke-me/jkeenan/afresh/134446-socket

Thank you very much.

With the patch the branch is experiencing failures in dist/IO/t/cachepropagate-unix.t on all our NetBSD smoke-testing rigs. It's experiencing other failures on one of them (http://perl5.test-smoke.org/report/95354 but ignore the failures in syslog.t).

Thank you very much.

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

p5pRT · 2019-09-23T11:53:57Z

From @jkeenan

On Mon, 23 Sep 2019 11:45:26 GMT, jkeenan wrote:

On Mon, 23 Sep 2019 01:58:25 GMT, jkeenan wrote:

On Mon, 23 Sep 2019 01:03:12 GMT, afresh1@openbsd.org wrote:

This is a bug report for perl from andrew@afresh1.com,
generated with the help of perlbug 1.41 running under perl 5.30.0.

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

When OpenBSD implemented support for SO_PROTOCOL to
getsockopt(2)[1],
suddenly IO/t/cachepropagate-unix.t started failing the 'protocol
match'
tests for `new_from_fd` sockets.

I had trouble deciding on whether to actually just remove the
entire
cache of protocol from "sub socket" or just to avoid caching it
when
it
is falsy. This may cause $sock->protocol to be undef on some
systems
that don't support SO_PROTOCOL where it was 0 before, but that is
arguably more correct.

In either case, this causes this test to pass because
$listener->protocol and $new->protocol now match.

$ perl dist/IO/t/cachepropagate-unix.t
1..15
ok 1 - stream socket created
ok 2 - protocol defined
ok 3 - domain defined
ok 4 - type defined
ok 5 - spawned a child
ok 6 - domain match
ok 7 - protocol match
ok 8 - type match
ok 9 - datagram socket created
ok 10 - protocol defined
ok 11 - domain defined
ok 12 - type defined
ok 13 - domain match
not ok 14 - protocol match
# Failed test 'protocol match'
# at dist/IO/t/cachepropagate-unix.t line 106.
# got: '1'
# expected: '0'
ok 15 - type match
# Looks like you failed 1 test of 15.

This test program on OpenBSD-current prints without the attached
patch:

guess: 0
proto: 1

--- BEGIN testprogram.pl ---

#!/usr/bin/perl;
use strict;
use warnings;

use IO::Socket::UNIX;

use File::Temp qw< tempdir >;

my $socketpath = tempdir( CLEANUP => 1 ) . '/testsock';

my $old = IO::Socket::UNIX->new(Type => SOCK_DGRAM, Local =>
$socketpath);
print "guess: " . ( defined $old->protocol ? $old->protocol :
'undef'
) . "\n";
my $protocol = unpack "i", getsockopt($old, Socket::SOL_SOCKET,
Socket::SO_PROTOCOL );
print "proto: " . ( defined $protocol ? $protocol : 'undef' ) .
"\n";

unlink($socketpath);

--- END testprogram.pl ---

[1]
openbsd/src@15b62b7

Applied patch attached to a branch for smoke-testing:
smoke-me/jkeenan/afresh/134446-socket

Thank you very much.

With the patch the branch is experiencing failures in
dist/IO/t/cachepropagate-unix.t on all our NetBSD smoke-testing rigs.
It's experiencing other failures on one of them (http://perl5.test-
smoke.org/report/95354 but ignore the failures in syslog.t).

Thank you very much.

In the preceding smoke-test report, most of the failures may be resource constraints. However, here's what I got for the test in question:

#####
[perl-reporter-08] $ cd t;./perl harness -v ../dist/IO/t/cachepropagate-unix.t; cd -

ok 1 - stream socket created
not ok 2 - protocol defined

# Failed test 'protocol defined'
# at t/cachepropagate-unix.t line 43.
ok 3 - domain defined
ok 4 - type defined
ok 5 - spawned a child
ok 6 - domain match
ok 7 # skip no Socket::SO_PROTOCOL
ok 8 - type match
ok 9 - datagram socket created
not ok 10 - protocol defined

# Failed test 'protocol defined'
# at t/cachepropagate-unix.t line 93.
ok 11 - domain defined
ok 12 - type defined
ok 13 - domain match
ok 14 # skip no Socket::SO_PROTOCOL
ok 15 - type match
# Looks like you failed 2 tests of 15.
Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/15 subtests
(less 2 skipped subtests: 11 okay)

Test Summary Report

../dist/IO/t/cachepropagate-unix.t (Wstat: 512 Tests: 15 Failed: 2)
Failed tests: 2, 10
Non-zero exit status: 2
Files=1, Tests=15, 1 wallclock secs ( 0.03 usr 0.01 sys + 0.18 cusr 0.04 csys = 0.26 CPU)
Result: FAIL
#####

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

p5pRT · 2019-09-23T15:57:29Z

From andrew@afresh1.com

On Mon, Sep 23, 2019 at 04:53:57AM -0700, James E Keenan via RT wrote:

On Mon, 23 Sep 2019 11:45:26 GMT, jkeenan wrote:

On Mon, 23 Sep 2019 01:58:25 GMT, jkeenan wrote:

On Mon, 23 Sep 2019 01:03:12 GMT, afresh1@openbsd.org wrote:

This may cause $sock->protocol to be undef on some systems
that don't support SO_PROTOCOL where it was 0 before, but that is
arguably more correct.

<SNIP>

With the patch the branch is experiencing failures in
dist/IO/t/cachepropagate-unix.t on all our NetBSD smoke-testing rigs.
It's experiencing other failures on one of them (http://perl5.test-
smoke.org/report/95354 but ignore the failures in syslog.t).

Thank you very much.

In the preceding smoke-test report, most of the failures may be
resource constraints. However, here's what I got for the test in
question:

This is kinda what I expected, not sure what the correct solution is.
Not sure why it's OK for `$sock->protocol` to return undef when you use
`new_from_fd` or after `accept` but not when you use `new`.

It would be easy to add in some `skip` calls if `$listener->protocol`
returned undef in the test, and since I would expect calling
`->protocol` to be a shortcut to `getsockopt`, that's the semantics I
would expect to have, but is that what is wanted? If so, the whole
cache line in `->socket` should be removed, which of course might cause
*other* problems if you expect `->protocol` to tell you what was
requested and not the value from `getsockopt`.

I'm not sure how to handle the fact that `->protocol` can return either
the requested protocol *or* the `getsockopt(..., SO_PROTOCOL)` with no
way to tell the difference.

#####
[perl-reporter-08] $ cd t;./perl harness -v ../dist/IO/t/cachepropagate-unix.t; cd -

ok 1 - stream socket created
not ok 2 - protocol defined

# Failed test 'protocol defined'
# at t/cachepropagate-unix.t line 43.
ok 3 - domain defined
ok 4 - type defined
ok 5 - spawned a child
ok 6 - domain match
ok 7 # skip no Socket::SO_PROTOCOL
ok 8 - type match
ok 9 - datagram socket created
not ok 10 - protocol defined

# Failed test 'protocol defined'
# at t/cachepropagate-unix.t line 93.
ok 11 - domain defined
ok 12 - type defined
ok 13 - domain match
ok 14 # skip no Socket::SO_PROTOCOL
ok 15 - type match
# Looks like you failed 2 tests of 15.
Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/15 subtests
(less 2 skipped subtests: 11 okay)

Test Summary Report
-------------------
../dist/IO/t/cachepropagate-unix.t (Wstat: 512 Tests: 15 Failed: 2)
Failed tests: 2, 10
Non-zero exit status: 2
Files=1, Tests=15, 1 wallclock secs ( 0.03 usr 0.01 sys + 0.18 cusr 0.04 csys = 0.26 CPU)
Result: FAIL
#####

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

--
andrew - http://afresh1.com

Speed matters.
Almost as much as some things, and nowhere near as much as others.
-- Nick Holland

I'm sure there's a better fix, but at the moment I don't know what it is. Perl/perl5#17157

tonycoz · 2020-04-07T05:49:18Z

Fixed by 6212a44 and a few other follow-up commits.

p5pRT added Severity Medium type-library labels Oct 19, 2019

afresh1 added a commit to afresh1/OpenBSD-perl that referenced this issue Nov 11, 2019

Skip tests that fails on newer OpenBSD

7e84768

I'm sure there's a better fix, but at the moment I don't know what it is. Perl/perl5#17157

tonycoz closed this as completed Apr 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only cache io_socket_proto if protocol is truthy #17157

Only cache io_socket_proto if protocol is truthy #17157

p5pRT commented Sep 23, 2019

p5pRT commented Sep 23, 2019

p5pRT commented Sep 23, 2019

p5pRT commented Sep 23, 2019

p5pRT commented Sep 23, 2019

p5pRT commented Sep 23, 2019

p5pRT commented Sep 23, 2019

p5pRT commented Sep 23, 2019

tonycoz commented Apr 7, 2020

Only cache io_socket_proto if protocol is truthy #17157

Only cache io_socket_proto if protocol is truthy #17157

Comments

p5pRT commented Sep 23, 2019

p5pRT commented Sep 23, 2019

From afresh1@openbsd.org

Created by andrew@afresh1.com

p5pRT commented Sep 23, 2019

From afresh1@openbsd.org

p5pRT commented Sep 23, 2019

From @jkeenan

p5pRT commented Sep 23, 2019

p5pRT commented Sep 23, 2019

From @jkeenan

p5pRT commented Sep 23, 2019

From @jkeenan

p5pRT commented Sep 23, 2019

From andrew@afresh1.com

tonycoz commented Apr 7, 2020