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

XSLoader may load relative paths #15418

Closed
p5pRT opened this issue Jul 3, 2016 · 13 comments
Closed

XSLoader may load relative paths #15418

p5pRT opened this issue Jul 3, 2016 · 13 comments

Comments

@p5pRT
Copy link

p5pRT commented Jul 3, 2016

Migrated from rt.perl.org#128528 (status was 'resolved')

Searchable as RT128528$

@p5pRT
Copy link
Author

p5pRT commented Jul 3, 2016

From @cpansprout

See <https://rt.cpan.org/Ticket/Display.html?id=115808>.  I have already pushed a fix for this as 08e3451. If the current CPAN maintainer of XSLoader is unavailable, then what is our next step? Can someone else make a release?

Should I go ahead and push my fix to maint-5.2x?

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2016

From @tonycoz

On Sun Jul 03 11​:24​:54 2016, sprout wrote​:

See <https://rt.cpan.org/Ticket/Display.html?id=115808>.  I have
already pushed a fix for this as 08e3451. If the current CPAN
maintainer of XSLoader is unavailable, then what is our next step?
Can someone else make a release?

Should I go ahead and push my fix to maint-5.2x?

+ # Does this look like a relative path?
+ if ($modlibname !~ m|^[\\/]|) {
+ # Someone may have a #line directive that changes the file name, or

Absolute paths might not start with a / or \ on Win32, VMS and (I think) AmigaOS.

Unfortunately loading File​::Spec calls back into XSLoader, so you can't use File​::Spec->file_name_is_absolute

Tony

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2016

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

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2016

From @craigberry

On Sun, Jul 3, 2016 at 7​:11 PM, Tony Cook via RT
<perl5-security-report@​perl.org> wrote​:

On Sun Jul 03 11​:24​:54 2016, sprout wrote​:

See <https://rt.cpan.org/Ticket/Display.html?id=115808>.  I have
already pushed a fix for this as 08e3451. If the current CPAN
maintainer of XSLoader is unavailable, then what is our next step?
Can someone else make a release?

Should I go ahead and push my fix to maint-5.2x?

+ # Does this look like a relative path?
+ if ($modlibname !~ m|^[\\/]|) {
+ # Someone may have a #line directive that changes the file name, or

Absolute paths might not start with a / or \ on Win32, VMS and (I think) AmigaOS.

Unfortunately loading File​::Spec calls back into XSLoader, so you can't use File​::Spec->file_name_is_absolute

The package filenames returned by caller() as well as @​INC entries are
stored in Unix format on VMS. If XSLoader​::load is called from
somewhere other than a package, then it's possible to get a filename
from caller() in native syntax, but I'm not sure why anyone would do
that. So I think we're ok there.

On Windows, it looks like both @​INC and the package filenames have
forward slashes, but they do still have the device name at the
beginning of absolute paths, so you have C​:/Perl/lib/... etc. So I
believe relative paths would fail to be identified as such by the
current code.

Do we really have a problem with relative paths as such, or do we have
a problem with package filenames that begin with "(eval" specifically?

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2016

From @craigberry

On Sun, Jul 3, 2016 at 10​:28 PM, Craig A. Berry <craig.a.berry@​gmail.com> wrote​:

On Windows, it looks like both @​INC and the package filenames have
forward slashes, but they do still have the device name at the
beginning of absolute paths, so you have C​:/Perl/lib/... etc. So I
believe relative paths would fail to be identified as such by the
current code.

I think I said that backwards -- it's absolute paths that will be
considered relative.

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2016

From @jhi

On Monday-201607-04 3​:11, Tony Cook via RT wrote​:

On Sun Jul 03 11​:24​:54 2016, sprout wrote​:

See <https://rt.cpan.org/Ticket/Display.html?id=115808>.  I have
already pushed a fix for this as 08e3451. If the current CPAN
maintainer of XSLoader is unavailable, then what is our next step?
Can someone else make a release?

Should I go ahead and push my fix to maint-5.2x?

+ # Does this look like a relative path?
+ if ($modlibname !~ m|^[\\/]|) {
+ # Someone may have a #line directive that changes the file name, or

Absolute paths might not start with a / or \ on Win32, VMS and (I think) AmigaOS.

IIUC yes for AmigaOS, it uses the same kind of "multi-root" (volume)
syntax as Win32 or VMS. Would something like m{^\w+​:[/\]} work for
those?

Unfortunately loading File​::Spec calls back into XSLoader, so you
can't use File​::Spec->file_name_is_absolute

Which of course feels kind of stupid. Cwd, I am guessing without
looking. (Or maybe some Win32 things?)

(Cwd being an XS has always been kind of strange, it leads into
silly dependencies. Being core/builtin would make more sense.)

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2016

From @demerphq

On 4 Jul 2016 02​:19, "Jarkko Hietaniemi" <jhi@​iki.fi> wrote​:

On Monday-201607-04 3​:11, Tony Cook via RT wrote​:

On Sun Jul 03 11​:24​:54 2016, sprout wrote​:

See <https://rt.cpan.org/Ticket/Display.html?id=115808>.  I have
already pushed a fix for this as 08e3451. If the current CPAN
maintainer of XSLoader is unavailable, then what is our next step?
Can someone else make a release?

Should I go ahead and push my fix to maint-5.2x?

+ # Does this look like a relative path?
+ if ($modlibname !~ m|^[\\/]|) {
+ # Someone may have a #line directive that changes the file
name, or

Absolute paths might not start with a / or \ on Win32, VMS and (I think)
AmigaOS.

IIUC yes for AmigaOS, it uses the same kind of "multi-root" (volume)
syntax as Win32 or VMS. Would something like m{^\w+​:[/\]} work for
those?

Unfortunately loading File​::Spec calls back into XSLoader, so you can't
use File​::Spec->file_name_is_absolute

Which of course feels kind of stupid. Cwd, I am guessing without
looking. (Or maybe some Win32 things?)

(Cwd being an XS has always been kind of strange, it leads into
silly dependencies. Being core/builtin would make more sense

++ to that...

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2016

From @cpansprout

On Jul 4, 2016, at 9​:57 AM, "yves orton via RT" <perl5-security-report@​perl.org> wrote​:

On 4 Jul 2016 02​:19, "Jarkko Hietaniemi" <jhi@​iki.fi> wrote​:

On Monday-201607-04 3​:11, Tony Cook via RT wrote​:

On Sun Jul 03 11​:24​:54 2016, sprout wrote​:

See <https://rt.cpan.org/Ticket/Display.html?id=115808>.  I have
already pushed a fix for this as 08e3451. If the current CPAN
maintainer of XSLoader is unavailable, then what is our next step?
Can someone else make a release?

Should I go ahead and push my fix to maint-5.2x?

+ # Does this look like a relative path?
+ if ($modlibname !~ m|^[\\/]|) {
+ # Someone may have a #line directive that changes the file
name, or

Absolute paths might not start with a / or \ on Win32, VMS and (I think)
AmigaOS.

IIUC yes for AmigaOS, it uses the same kind of "multi-root" (volume)
syntax as Win32 or VMS. Would something like m{^\w+​:[/\]} work for
those?

Unfortunately loading File​::Spec calls back into XSLoader, so you can't
use File​::Spec->file_name_is_absolute

I just looked through all the various implementations of file_name_is_absolute and came up with commit v5.25.2-95-ga651dcd, now pushed to blead. As I pointed out in the commit message, it’s not the end of the world if we mistakenly view some paths as relative, but it will be slightly slower.

@p5pRT
Copy link
Author

p5pRT commented Jul 4, 2016

From @cpansprout

On Jul 3, 2016, at 8​:29 PM, "Craig Berry via RT" <perl5-security-report@​perl.org> wrote​:

Do we really have a problem with relative paths as such, or do we have
a problem with package filenames that begin with "(eval" specifically?

‘(eval’ specifically, but that just reveals the problem. There are numerous reasons why one might use #line directives to change the file name. XSLoader’s documentation does not warn users about it, and, quite frankly, I don’t think users should have to worry about it.

@p5pRT
Copy link
Author

p5pRT commented Jan 23, 2017

From @tonycoz

On Sun, 03 Jul 2016 11​:24​:54 -0700, sprout wrote​:

See <https://rt.cpan.org/Ticket/Display.html?id=115808>.  I have
already pushed a fix for this as 08e3451. If the current CPAN
maintainer of XSLoader is unavailable, then what is our next step?
Can someone else make a release?

Should I go ahead and push my fix to maint-5.2x?

This is fixed in blead and on CPAN - does it need to be fixed anywhere else
to resolve this?

Tony

@p5pRT
Copy link
Author

p5pRT commented Feb 21, 2017

From @iabyn

On Sun, Jan 22, 2017 at 08​:19​:29PM -0800, Tony Cook via RT wrote​:

On Sun, 03 Jul 2016 11​:24​:54 -0700, sprout wrote​:

See <https://rt.cpan.org/Ticket/Display.html?id=115808>.  I have
already pushed a fix for this as 08e3451. If the current CPAN
maintainer of XSLoader is unavailable, then what is our next step?
Can someone else make a release?

Should I go ahead and push my fix to maint-5.2x?

This is fixed in blead and on CPAN - does it need to be fixed anywhere else
to resolve this?

The two XSloader fixes (don't load relative paths, and recognize drive
letters) have been cherry-picked into both maint-5.24 and maint-5.22.
So I think this ticket can be closed.

I also think it can be moved to the public queue, as the XS loader issue
has been public since last July.

--
The warp engines start playing up a bit, but seem to sort themselves out
after a while without any intervention from boy genius Wesley Crusher.
  -- Things That Never Happen in "Star Trek" #17

@p5pRT
Copy link
Author

p5pRT commented Feb 27, 2017

From @iabyn

On Tue, Feb 21, 2017 at 10​:31​:01AM +0000, Dave Mitchell wrote​:

On Sun, Jan 22, 2017 at 08​:19​:29PM -0800, Tony Cook via RT wrote​:

On Sun, 03 Jul 2016 11​:24​:54 -0700, sprout wrote​:

See <https://rt.cpan.org/Ticket/Display.html?id=115808>.  I have
already pushed a fix for this as 08e3451. If the current CPAN
maintainer of XSLoader is unavailable, then what is our next step?
Can someone else make a release?

Should I go ahead and push my fix to maint-5.2x?

This is fixed in blead and on CPAN - does it need to be fixed anywhere else
to resolve this?

The two XSloader fixes (don't load relative paths, and recognize drive
letters) have been cherry-picked into both maint-5.24 and maint-5.22.
So I think this ticket can be closed.

I also think it can be moved to the public queue, as the XS loader issue
has been public since last July.

which I am now doing.

--
That he said that that that that is is is debatable, is debatable.

@p5pRT
Copy link
Author

p5pRT commented Feb 27, 2017

@iabyn - Status changed from 'open' to 'resolved'

@p5pRT p5pRT closed this as completed Feb 27, 2017
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

1 participant