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

IO::Socket::INET doesn’t report bad PeerPort values #13466

Open
p5pRT opened this issue Dec 12, 2013 · 12 comments
Open

IO::Socket::INET doesn’t report bad PeerPort values #13466

p5pRT opened this issue Dec 12, 2013 · 12 comments

Comments

@p5pRT
Copy link

p5pRT commented Dec 12, 2013

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

Searchable as RT120764$

@p5pRT
Copy link
Author

p5pRT commented Dec 12, 2013

From @FGasper

Created by @FGasper

perl -MIO​::Socket​::INET -e 'IO​::Socket​::INET->new( PeerAddr => "google.com", PeerPort => "haha" ) || die "connect failed​: [$!]"'

The above example doesn�t populate $! as it should with a message about the invalid PeerPort value.

Note that the following​:
perl -MIO​::Socket​::INET -e 'IO​::Socket​::INET->new( PeerAddr => "@​@​@​", PeerPort => "80" ) || die "connect failed​: [$!]"'

...does produce an "Invalid argument" message in $!.

Perl Info

Flags:
    category=library
    severity=low
    module=IO::Socket::INET

Site configuration information for perl 5.14.3:

Configured by cPanel at Tue Mar  5 15:43:05 CST 2013.

Summary of my perl5 (revision 5 version 14 subversion 3) configuration:
   
  Platform:
    osname=linux, osvers=2.6.32-220.17.1.el6.i686, archname=i386-linux-64int
    uname='linux rpmb-32-centos-6 2.6.32-220.17.1.el6.i686 #1 smp tue may 15 22:09:39 bst 2012 i686 i686 i386 gnulinux '
    config_args='-des -Dusedevel -Darchname=i386-linux-64int -Dcc=/usr/bin/gcc -Dcpp=/usr/bin/cpp -DDEBUGGING=none -Doptimize=-Os -Dusemymalloc=n -Duseshrplib -Duselargefiles=yes -Duseposix=true -Dhint=recommended -Duseperlio=yes -Dccflags=-I/usr/local/cpanel/3rdparty/perl/514/include -L/usr/local/cpanel/3rdparty/perl/514/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -DAPPLLIB_EXP="/usr/local/cpanel" -Dcppflags=-I/usr/local/cpanel/3rdparty/perl/514/include -L/usr/local/cpanel/3rdparty/perl/514/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -Dldflags=-Wl,-rpath -Wl,/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/lib -Dprefix=/usr/local/cpanel/3rdparty/perl/514 -Dsiteprefix=/opt/cpanel/perl5/514 -Dsitebin=/opt/cpanel/perl5/514/bin -Dsitelib=/opt/cpanel/perl5/514/site_lib -Dusevendorprefix=true -Dvendorbin=/usr/local/cpanel/3rdparty/perl/514/bin -Dvendorprefix=/usr/local/cpanel/3rdparty/perl/514/lib/perl5 -Dvendorlib=/usr/local/cpanel/3rdparty/perl/514/lib/perl5/cpanel_lib -Dprivlib=/usr/local/cpanel/3rdparty/perl/514/lib/perl5/5.14.3 -Dman1dir=none -Dman3dir=none -Dscriptdir=/usr/local/cpanel/3rdparty/perl/514/bin -Dscriptdirexp=/usr/local/cpanel/3rdparty/perl/514/bin -Dsiteman1dir=none -Dsiteman3dir=none -Dinstallman1dir=none -Dversiononly=no -Dinstallusrbinperl=no -Dcf_by=cPanel -Dmyhostname=localhost -Dperladmin=root@localhost -Dcf_email=support@cpanel.net -Di_dbm=/usr/local/cpanel/3rdparty/include -Di_gdbm=/usr/local/cpanel/3rdparty/include -Di_ndbm=/usr/local/cpanel/3rdparty/include -Ud_dosuid -Uuserelocatableinc -Umad -Uusethreads -Uusemultiplicity -Uusesocks -Uuselongdouble -Ui_db -Aldflags=-L/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/lib -L/usr/lib -L/lib -lgdbm -Dlocincpth=/usr/local/cpanel/3rdparty/perl/514/include /usr/local/cpanel/3rdparty/include /usr/local/include  -Duse64bitint -Uuse64bitall -Acflags=-fPIC -DPIC -m32 -I/usr/local/cpanel/3rdparty/perl/514/include -I/usr/local/cpanel/3rdparty/include -Dlibpth=/usr/local/cpanel/3rdparty/perl/514/lib /usr/local/cpanel/3rdparty/lib /usr/local/lib /lib /usr/lib '
    hint=recommended, useposix=true, d_sigaction=define
    useithreads=undef, usemultiplicity=undef
    useperlio=define, d_sfio=undef, uselargefiles=define, usesocks=undef
    use64bitint=define, use64bitall=undef, uselongdouble=undef
    usemymalloc=n, bincompat5005=undef
  Compiler:
    cc='/usr/bin/gcc', ccflags ='-I/usr/local/cpanel/3rdparty/perl/514/include -L/usr/local/cpanel/3rdparty/perl/514/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -DAPPLLIB_EXP="/usr/local/cpanel" -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include -D_LARGEFILE_SOURCE -D_FILE_OFFSET_BITS=64',
    optimize='-Os',
    cppflags='-I/usr/local/cpanel/3rdparty/perl/514/include -L/usr/local/cpanel/3rdparty/perl/514/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -I/usr/local/cpanel/3rdparty/perl/514/include -L/usr/local/cpanel/3rdparty/perl/514/lib -I/usr/local/cpanel/3rdparty/include -L/usr/local/cpanel/3rdparty/lib -DAPPLLIB_EXP="/usr/local/cpanel" -fno-strict-aliasing -pipe -fstack-protector -I/usr/local/include'
    ccversion='', gccversion='4.4.6 20110731 (Red Hat 4.4.6-3)', gccosandvers=''
    intsize=4, longsize=4, ptrsize=4, doublesize=8, byteorder=12345678
    d_longlong=define, longlongsize=8, d_longdbl=define, longdblsize=12
    ivtype='long long', ivsize=8, nvtype='double', nvsize=8, Off_t='off_t', lseeksize=8
    alignbytes=4, prototype=define
  Linker and Libraries:
    ld='/usr/bin/gcc', ldflags ='-Wl,-rpath -Wl,/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/lib -L/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/lib -L/usr/lib -L/lib -lgdbm -fstack-protector -L/usr/local/lib'
    libpth=/usr/local/lib /lib /usr/lib
    libs=-lnsl -lgdbm -ldb -ldl -lm -lcrypt -lutil -lc
    perllibs=-lnsl -ldl -lm -lcrypt -lutil -lc
    libc=/lib/libc-2.12.so, so=so, useshrplib=true, libperl=libperl.so
    gnulibc_version='2.12'
  Dynamic Linking:
    dlsrc=dl_dlopen.xs, dlext=so, d_dlsymun=undef, ccdlflags='-Wl,-E -Wl,-rpath,/usr/local/cpanel/3rdparty/perl/514/lib/perl5/5.14.3/i386-linux-64int/CORE'
    cccdlflags='-fPIC', lddlflags='-shared -Os -L/usr/local/cpanel/3rdparty/perl/514/lib -L/usr/local/cpanel/3rdparty/lib -L/usr/lib -L/lib -L/usr/local/lib -fstack-protector'

Locally applied patches:
    cPanel patches
    cPanel INC path changes
    Disabled some unstable tests on a kvm build server
    Cherry pick of 49498ca from p5p (RT 113514)


@INC for perl 5.14.3:
    /usr/local/cpanel
    /usr/local/cpanel
    /usr/local/cpanel/3rdparty/perl/514/lib/perl5/cpanel_lib/i386-linux-64int
    /usr/local/cpanel/3rdparty/perl/514/lib/perl5/cpanel_lib
    /usr/local/cpanel/3rdparty/perl/514/lib/perl5/5.14.3/i386-linux-64int
    /usr/local/cpanel/3rdparty/perl/514/lib/perl5/5.14.3
    /opt/cpanel/perl5/514/site_lib/i386-linux-64int
    /opt/cpanel/perl5/514/site_lib
    .


Environment for perl 5.14.3:
    HOME=/root
    LANG=en_US.UTF-8
    LANGUAGE (unset)
    LD_LIBRARY_PATH (unset)
    LOGDIR (unset)
    PATH=/root/.opt/bin:/usr/local/cpanel/3rdparty/perl/514/bin:/usr/local/cpanel/3rdparty/bin:/usr/local/cpanel/3rdparty/node/bin:/usr/local/sbin:/usr/local/bin:/sbin:/bin:/usr/sbin:/usr/bin
    PERL5LIB=/usr/local/cpanel
    PERL_BADLANG (unset)
    SHELL=/bin/bash

@p5pRT
Copy link
Author

p5pRT commented Dec 12, 2013

From @tonycoz

On Wed Dec 11 16​:19​:59 2013, felipe@​felipegasper.com wrote​:

[Please describe your issue here]

perl -MIO​::Socket​::INET -e 'IO​::Socket​::INET->new( PeerAddr =>
"google.com", PeerPort => "haha" ) || die "connect failed​: [$!]"'

The above example doesn�t populate $! as it should with a message
about the invalid PeerPort value.

Note that the following​:
perl -MIO​::Socket​::INET -e 'IO​::Socket​::INET->new( PeerAddr => "@​@​@​",
PeerPort => "80" ) || die "connect failed​: [$!]"'

...does produce an "Invalid argument" message in $!.

The IO modules generally report errors in $@​​:

$ perl -MIO​::Socket​::INET -e 'IO​::Socket​::INET->new( PeerAddr => "google.com", PeerPort => "haha" ) || die "connect failed​: [$@​]"'
connect failed​: [IO​::Socket​::INET​: Bad service 'haha'] at -e line 1.

That isn't very well[1] documented, which may be the bug here.

Tony

[1] at all? It's demonstrated in an example fo IO​::Socket​::INET->connect(), but I didn't see anything more explicit

@p5pRT
Copy link
Author

p5pRT commented Dec 12, 2013

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

@p5pRT
Copy link
Author

p5pRT commented Dec 12, 2013

From @leonerd

On Wed, 11 Dec 2013 21​:42​:45 -0600
Felipe Gasper <felipe@​felipegasper.com> wrote​:

It should put I/O errors in $!, should it not, like open() et al.? $@​
is supposed to be for eval() exclusively .. at, least, that seems to
be the norm?

Unfortunately you can't put a string in $!. All you can put in $! is a
number that refers to a libc errno constant. If the error you have is
something new and different, which libc doesn't have a constant for,
you are out of luck with $!.

--
Paul "LeoNerd" Evans

leonerd@​leonerd.org.uk
ICQ# 4135350 | Registered Linux# 179460
http​://www.leonerd.org.uk/

@p5pRT
Copy link
Author

p5pRT commented Dec 12, 2013

From @FGasper

On 12.12.13 9​:17 AM, Paul LeoNerd Evans via RT wrote​:

On Wed, 11 Dec 2013 21​:42​:45 -0600
Felipe Gasper <felipe@​felipegasper.com> wrote​:

It should put I/O errors in $!, should it not, like open() et al.? $@​
is supposed to be for eval() exclusively .. at, least, that seems to
be the norm?

Unfortunately you can't put a string in $!. All you can put in $! is a
number that refers to a libc errno constant. If the error you have is
something new and different, which libc doesn't have a constant for,
you are out of luck with $!.

Agh -- duh.

Maybe a package global, then, a la DBI?

$@​ is called $EVAL_ERROR in English.pm, so this pattern​:

local $EVAL_ERROR;
IO​::Socket​::INET->new( .. ) or die $EVAL_ERROR;

...would be unfortunate to �enshrine�.

Much better, IMO​:

IO​::Socket​::INET->new( .. ) or die $IO​::Socket​::INET​::errstr;

-FG

@p5pRT
Copy link
Author

p5pRT commented Dec 12, 2013

From @leonerd

On Thu, 12 Dec 2013 09​:50​:21 -0600
Felipe Gasper <felipe@​felipegasper.com> wrote​:

Much better, IMO​:

IO​::Socket​::INET->new( .. ) or die $IO​::Socket​::INET​::errstr;

Except it isn't IO​::Socket​::INET's error value, it's IO​::Socket's. Or
subclasses. Subclassing is awkward here.

Plus all the existing code

--
Paul "LeoNerd" Evans

leonerd@​leonerd.org.uk
ICQ# 4135350 | Registered Linux# 179460
http​://www.leonerd.org.uk/

@p5pRT
Copy link
Author

p5pRT commented Dec 12, 2013

From @FGasper

On 12/12/13 9​:56 AM, Paul LeoNerd Evans via RT wrote​:

On Thu, 12 Dec 2013 09​:50​:21 -0600
Felipe Gasper <felipe@​felipegasper.com> wrote​:

Much better, IMO​:

IO​::Socket​::INET->new( .. ) or die $IO​::Socket​::INET​::errstr;

Except it isn't IO​::Socket​::INET's error value, it's IO​::Socket's. Or
subclasses. Subclassing is awkward here.

Where the error comes from is an implementation detail. The caller
invokes IO​::Socket​::INET, so it should reasonably expect to find the
error reported from there as well, even if it originates someplace else.

This can easily be abstracted.

For subclassing​: IO​::Socket​::INET->errstr()

Plus all the existing code

Existing code can stay as-is; just deprecate use of $@​ and $! for this.

-F

@p5pRT
Copy link
Author

p5pRT commented Dec 13, 2013

From @rjbs

* Felipe Gasper <felipe@​felipegasper.com> [2013-12-12T11​:37​:32]

Plus all the existing code

Existing code can stay as-is; just deprecate use of $@​ and $! for this.

What does "deprecate" mean as you use it here?

I don't see how this could be usefully changed. The amount of code already
checking $@​ is likely to be quiet large. I think it would be hard to set any
sort of date after which these libraries would stop assigning to $@​.

If that's he case, what's the point?

--
rjbs

@p5pRT
Copy link
Author

p5pRT commented Dec 13, 2013

From @FGasper

On 12/12/13 8​:55 PM, Ricardo Signes via RT wrote​:

* Felipe Gasper <felipe@​felipegasper.com> [2013-12-12T11​:37​:32]

Plus all the existing code

Existing code can stay as-is; just deprecate use of $@​ and $! for this.

What does "deprecate" mean as you use it here?

I don't see how this could be usefully changed. The amount of code already
checking $@​ is likely to be quiet large. I think it would be hard to set any
sort of date after which these libraries would stop assigning to $@​.

If that's he case, what's the point?

Maybe �deprecate� isn�t what I really mean. Sorry.

My proposals​:

1) Create and document IO​::Socket​::INET->err() and
IO​::Socket​::INET->errstr() for constructor errors for that class.
(Possibly a similar pattern for other IO​::Socket modules.)

2) Leave undocumented, but preserve indefinitely, assignments to $! and $@​.

3) Document that IO​::Socket​::INET​::new() can overwrite those globals, so
callers should â��local $@​â�� whenever instantiating this class.
(Optionally, provide a �noPunctuationGlobals� option that prevents
affecting $! and $@​.)

In other words, moving forward, callers should stop looking at $! and $@​
for where errors go.

Basically, IO​::Socket​::INET errors should not look like they come from
some other layer. There�s no eval() in play as far as the caller is
concerned (implementation details are a different concern), so the
caller definitely should not check $@​. If $! canâ��t handle arbitrary
text, then that can�t be the place, either.

The pattern that I propose is familiar from DBI and seems to me to
respect proper encapsulation.

At the *very* minimum, as Tony pointed out in his response initially,
there needs to be documentation of where to find these errors. If they
stay in $!/$@​, then the POD needs to document how to retrieve those
errors reliably. â��$! || $@​â�� is kind of ugly â�¦ made worse by that stuff
from $@​ seems to have a "$module​: " prefix, while things from $! do not.

-FG

@p5pRT
Copy link
Author

p5pRT commented Dec 13, 2013

From zefram@fysh.org

Felipe Gasper wrote​:

1) Create and document IO​::Socket​::INET->err() and
IO​::Socket​::INET->errstr() for constructor errors for that class.
(Possibly a similar pattern for other IO​::Socket modules.)

It's not sensible to propose anything for IO​::Socket​::INET in isolation.
Most of its behaviour actually comes from IO​::Socket and IO​::Socket's
ancestors. You need to consider the whole hierarchy of I/O handle
classes.

Furthermore, any mechanism based on shared static storage locations just
looks like repeating the mistakes of the past. Sure, you might avoid
the shared $! and $@​, but you're inventing new globals that will be
shared between all IO​::Socket​::INET constructions. Or all IO​::Socket
constructions, or all IO​::Handle constructions, if ->err and ->errstr
are inherited. Any version of this suffers the same fundamental problem.

The way an error is signalled should be tied to the construction
operation. Fortunately we've already got a well-defined way to do that​:
throw an exception. For a new facility, you can define and use structured
exception objects rather than flat strings.

(Optionally, provide a "noPunctuationGlobals" option that
prevents affecting $! and $@​.)

Obviously, throwing an exception is inconsistent with the existing API, so
it would need to be enabled by such a constructor option. Anything short
of an explicit option won't achieve any meaningful shift in the API.
The existing $!/$@​ mechanism needs to remain supported indefinitely.

-zefram

@p5pRT
Copy link
Author

p5pRT commented Dec 13, 2013

From @FGasper

On 13.12.13 2​:13 PM, Zefram wrote​:

Felipe Gasper wrote​:

1) Create and document IO​::Socket​::INET->err() and
IO​::Socket​::INET->errstr() for constructor errors for that class.
(Possibly a similar pattern for other IO​::Socket modules.)

The way an error is signalled should be tied to the construction
operation. Fortunately we've already got a well-defined way to do that​:
throw an exception. For a new facility, you can define and use structured
exception objects rather than flat strings.

I like this idea much better, yes.

(Optionally, provide a "noPunctuationGlobals" option that
prevents affecting $! and $@​.)

Obviously, throwing an exception is inconsistent with the existing API, so
it would need to be enabled by such a constructor option. Anything short
of an explicit option won't achieve any meaningful shift in the API.
The existing $!/$@​ mechanism needs to remain supported indefinitely.

Again, agreed.

-FG

@FGasper
Copy link
Contributor

FGasper commented Jan 31, 2020

This task could be repurposed as “Add error reporting to IO::Socket::* documentation.”

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

No branches or pull requests

2 participants