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

Socket::getaddrinfo can now fail without returning error #221

Open
Yaribz opened this issue Oct 15, 2024 · 20 comments · May be fixed by Perl/perl5#22676
Open

Socket::getaddrinfo can now fail without returning error #221

Yaribz opened this issue Oct 15, 2024 · 20 comments · May be fixed by Perl/perl5#22676
Labels
help wanted upstream module Issue is due to an upstream module

Comments

@Yaribz
Copy link

Yaribz commented Oct 15, 2024

This code performs an invalid call to Socket::getaddrinfo (invalid value for socktype parameter):

use Socket qw'getaddrinfo';

my ($err)=getaddrinfo('perl.org', 80, {socktype => -1});
die "getaddrinfo error: $err\n" if($err);
print "OK.\n";

$err is supposed to contain an error message in this case, as seen in the result with Strawberry Perl 5.38.2.2 64bit:

getaddrinfo error: La prise en charge du type de socket spécifié n'existe pas dans cette famille d'adresses.

But the result with Strawberry Perl 5.40.0.1 64bit is:

OK.
@shawnlaffan
Copy link
Contributor

Confirmed.

Presumably related to #220

@sisyphus
Copy link

sisyphus commented Oct 15, 2024

There's some weird shit going on there.
StrawberryPerl-5.40.0 comes with Socket-2.038.
@Yaribz, if I re-install Socket-2.038 (same version) by running cpan -fi Socket then I find that your script dies - exactly as it should.
I think you will find that the script presented in #220 then also starts to work correctly, too.

This is not limited to Strawberry builds - I see the same in my own builds of 5.40.0 and the latest devel release (5.41.4).

The "perl Makefile.PL" part of the Socket module build does some probing to see what's available.
I think that probing might be yielding differing results - depending upon whether Socket is being built as part of the perl build, or whether it's being built separately by a perl that has already been created.
(I'll investigate further tomorrow.)

UPDATE:
Had a bit of a look tonight, anyway.
The "probing" does not take place when the Socket module is built as part of the perl build.
Consequently, during the perl build, Socket.c is compiled without the following definitions:

-DHAS_GAI_STRERROR -DHAS_GETADDRINFO -DHAS_GETNAMEINFO -DHAS_SOCKADDR_IN6 -DHAS_IP_MREQ -DHAS_IP_MREQ_SOURCE -DHAS_IPV6_MREQ

When building the Socket module after perl has been built, the "probing" that is done adds those definitions to the gcc command that compiles Socket.c, and thereby extra functionality is built in.

For now, it looks to me that re-installing the Socket module is a correct workaround.

@Yaribz
Copy link
Author

Yaribz commented Oct 15, 2024

@Yaribz, if I re-install Socket-2.038 (same version) by running cpan -fi Socket then I find that your script dies - exactly as it should.
I think you will find that the script presented in #220 then also starts to work correctly, too.

Yes indeed, thanks for the investigation and the workaround.

The "probing" does not take place when the Socket module is built as part of the perl build.

Do you have an idea why the Socket module included with Strawberry Perl 5.38 was fine ? Perl core modules might have been (re)built in a second pass in previous Strawberry releases ?

@shawnlaffan
Copy link
Contributor

Do you have an idea why the Socket module included with Strawberry Perl 5.38 was fine ? Perl core modules might have been (re)built in a second pass in previous Strawberry releases ?

The Strawberry Perl build process has a step to update any of the dual-life modules (see below). These are under the .../per/vendir/lib dir.

However, Socket.pm is not in the vendor/lib dir for either 5.38.2.2 or 5.40.0.1. So it must be some other reason.

perl -MSocket -E"say $^V; say $INC{'Socket.pm'}; say $Socket::VERSION"
v5.38.2
C:/perls/5.38.2.2_PDL/perl/lib/Socket.pm
2.037
perl -MSocket -E"say $^V; say $INC{'Socket.pm'}; say $Socket::VERSION"
v5.40.0
C:/perls/5.40.0.1_PDL/perl/lib/Socket.pm
2.038

### NEXT STEP ###########################
{
plugin => 'Perl::Dist::Strawberry::Step::UpgradeCpanModules',
exceptions => [
# possible 'do' options: ignore_testfailure | skiptest | skip - e.g.
#{ do=>'ignore_testfailure', distribution=>'ExtUtils-MakeMaker-6.72' },
#{ do=>'ignore_testfailure', distribution=>qr/^IPC-Cmd-/ },
{ do=>'ignore_testfailure', distribution=>qr/^Net-Ping-/ }, # 2.72 fails
{ do=>'ignore_testfailure', distribution=>qr/^Archive-Tar-/ }, # 3.02 fails
]
},

@shawnlaffan
Copy link
Contributor

However, Socket.pm is not in the vendor/lib dir for either 5.38.2.2 or 5.40.0.1. So it must be some other reason

Actually, it turns out that the cpan upgrade step installs into .../c/perl/lib, so it would seem that Socket was updated as part of the 5.38.2 build and not the 5.40.0 build.

@mohawk2
Copy link

mohawk2 commented Oct 15, 2024

Are these exceptions and this (to me) unexpected installation idea being fed back to main-Perl? My best-case scenario is that the SP build stuff gets incorporated into their CI so that any commits that cause a problem are visible instantly, somewhat like PDL has.

@shawnlaffan
Copy link
Contributor

The exceptions are modules that fail to install for some reason.

Usually there are upstream reports (e.g. https://rt.cpan.org/Public/Bug/Display.html?id=149072). We do raise these when building new versions of SP.

Failures also trigger reports on cpantesters, but obviously after an SP release.

@mohawk2
Copy link

mohawk2 commented Oct 15, 2024

No push towards making more CI for main Perl so that this sort of thing becomes impossible?

@shawnlaffan
Copy link
Contributor

No push towards making more CI for main Perl so that this sort of thing becomes impossible?

I lack the tuits for that but it would be a welcome contribution.

FWIW, there is a Windows CI on the main Perl repo. It uses an older version of winlibs than we use for SP but it should flag many errors.
https://github.com/Perl/perl5/blob/19a540392dc1dea55a673834a3c690d1dd5dee6a/.github/workflows/testsuite.yml#L516-L559

That said, it won't identify issues with dual-life modules. That comes back to the smoke testers and BBC reports.

@mohawk2
Copy link

mohawk2 commented Oct 15, 2024

What (approximately) would a contributor need to change about the current Windows CI (which I have looked over in the past) to make it be using the latest winlibs? I think I'm offering to PR that ;-)

@sisyphus
Copy link

sisyphus commented Oct 16, 2024

The main problem with Socket is that the source on CPAN differs from the source that is being included in the perl source.
That is not allowed - the fact that they both report the same version number (currently 2.038) means that they must be identical.
Update: Hmmm .... maybe they are identical, after all. And it's just some difference in the build environment that's creating the issue.

To complicate matters, the test suite for both variants reports no test failures - and it's the same test suite that gets run in both cases.
Clearly, there is some deficiency in the test coverage, and CI testing that looks merely at the test results has no way of currently detecting this issue, irrespective of winlibs gcc version.

I'm fairly sure this situation has existed since Socket-2.036, maybe even earlier.

I can file an Issue (or PR) against the Socket distro re the lack of synchronicity once I've had a proper look at it - but it won't get fixed in blead until Socket is re-synced by the Socket maintainer.
Hopefully that would be in time for perl-5.42.

@shawnlaffan
Copy link
Contributor

Thanks Rob.

@mohawk2 - if you mean CI for the main perl5 repo then it should be a case of changing the winlibs URL to a more recent version (full list at https://winlibs.com/).
https://github.com/Perl/perl5/blob/19a540392dc1dea55a673834a3c690d1dd5dee6a/.github/workflows/testsuite.yml#L529

That won't fix issues with symlinks and the like given we set those as part of the Strawberry Perl config using code in this repo.

'config_H.gc' => {
I_DBM => 'define',
I_GDBM => 'define',
I_NDBM => 'define',
HAS_BUILTIN_CHOOSE_EXPR => 'define',
HAS_SYMLINK => 'define',
HAS_ISFINITE => 'define', # part of https://github.com/Perl/perl5/pull/22257
},
'config.gc' => { # see Step.pm for list of default updates
d_builtin_choose_expr => 'define',
d_mkstemp => 'define',
d_ndbm => 'define',
d_symlink => 'define', # many cpan modules fail tests when defined
i_db => 'define',
i_dbm => 'define',
i_gdbm => 'define',
i_ndbm => 'define',
d_isfinite => 'define', # part of https://github.com/Perl/perl5/pull/22257
osvers => '10',
},

if ($new eq 'config_H.gc' and ref($dst) =~ /HASH/) {
$self->boss->message(5, "_patch_file: using hash of values to update config_H.gc'\n");
$self->_update_config_H_gc ("$dir/win32/config_H.gc", $dst);
}
elsif ($new eq 'config.gc' and ref($dst) =~ /HASH/) {
$self->boss->message(5, "_patch_file: using hash of values to update config.gc'\n");
$self->_update_config_gc ("$dir/win32/config.gc", $dst);
}

It would be useful if those were tested at the Perl source. The code to change the settings is not overly complex so it could be added as an additional config on that repo.

@sisyphus
Copy link

It looks like the issue arises, not because of any difference in the sources, but because of a difference in the 2 environments.
I gather that cpan/Socket/Makefile.PL does not get executed by perl during the perl build
Not surprising given there's no "perl" to execute it .... right ?
And the cpan/Socket/Makefile is generated some other way that skips the probing ?
I don't know how to deal with this.
Anyone ??

@mohawk2
Copy link

mohawk2 commented Oct 16, 2024

It looks like the issue arises, not because of any difference in the sources, but because of a difference in the 2 environments. I gather that cpan/Socket/Makefile.PL does not get executed by perl during the perl build Not surprising given there's no "perl" to execute it .... right ? And the cpan/Socket/Makefile is generated some other way that skips the probing ? I don't know how to deal with this. Anyone ??

I think it's time to open an issue in Perl.

@shawnlaffan shawnlaffan added the upstream module Issue is due to an upstream module label Oct 16, 2024
@sisyphus
Copy link

I think it's time to open an issue in Perl.

Turns out I just need to prepare the PR I mooted at the end of https://rt.cpan.org/Public/Bug/Display.html?id=141325.
That seems to work quite well, and correctly handles the 2 scripts that @Yaribz provided.

I think I became a little concerned about what happens if those config keys are defined, but the underlying system doesn't support one or more of them.
However, nearly 3 years later on, that should now be less of a consideration.
(Of course, I then eventually forgot all about it.)

Anyway - I'll just make the PR, and then it's up to the powers that be whether it gets merged.

I'll post a link to it here (when it's done) later today.

sisyphus pushed a commit to sisyphus/perl5 that referenced this issue Oct 17, 2024
@Yaribz
Copy link
Author

Yaribz commented Oct 17, 2024

Thanks a lot ! I'm still unsure I really understand why the Socket module included with Strawberry Perl 5.38.2 doesn't suffer from the problem though.

@sisyphus
Copy link

sisyphus commented Oct 17, 2024

I'm still unsure I really understand why the Socket module included with Strawberry Perl 5.38.2 doesn't suffer from the problem though.

Me, too.
One can usually take it for granted that the modules in perl/lib were built as part of the perl build - but not in the case of this SP-5.38.2 perl/lib/Socket installation, it seems.
The second Socket installation in my SP-5.38.2 perl/site/lib must have been one that I had installed myself (probably at some time in the last few days ;-)
There are 2 installations of the Socket module in Strawberry Perl 5.38.2.
One is in perl/lib - it was built during the building of Strawberry Perl itself, and therefore suffers the problem.
The other is in perl/site/lib - it was built and installed (by Strawberry Perl 5.38.2) after Strawberry Perl 5.38.2 had been built. and therefore does not suffer the problem.

When you use Socket; it is the (good) one in perl/site/lib that gets loaded.

If you rename perl/site/lib/Socket.pm to perl/site/lib/Socket.pm_hide, then use Socket; should load (the bad) Socket.pm in perl/lib, and the problem should become evident again.

@shawnlaffan
Copy link
Contributor

Actually, Perl 5.38 comes with Socket 2.036.
https://perldoc.perl.org/5.38.0/Socket

SP 5.38.2.2 was built after Socket 2.037 was released and so it was upgraded as part of the SP 5.38 build process.

Perl 5.40 comes with Socket 2.038. This is the current version on CPAN and so it is not updated as part of the SP 5.40 build process.

Assuming a new release of SP 5.40.0 is not needed I'll add a rebuild to the 5.40.1 build config when relevant.

@sisyphus
Copy link

SP 5.38.2.2 was built after Socket 2.037 was released and so it was upgraded as part of the SP 5.38 build process.

I think that if the vendor decides to upgrade a perl core module after perl has been built, it is more transparent if the upgrade is placed in perl/vendor/lib rather than overwriting the existing perl/lib version.

@shawnlaffan
Copy link
Contributor

SP 5.38.2.2 was built after Socket 2.037 was released and so it was upgraded as part of the SP 5.38 build process.

I think that if the vendor decides to upgrade a perl core module after perl has been built, it is more transparent if the upgrade is placed in perl/vendor/lib rather than overwriting the existing perl/lib version.

FWIW, it's been this way for at least twelve years. I'd be in favour of the change, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted upstream module Issue is due to an upstream module
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants