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

Increase the fallback value of MAXPATHLEN #11915

Closed
p5pRT opened this issue Jan 28, 2012 · 25 comments
Closed

Increase the fallback value of MAXPATHLEN #11915

p5pRT opened this issue Jan 28, 2012 · 25 comments

Comments

@p5pRT
Copy link

p5pRT commented Jan 28, 2012

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

Searchable as RT109262$

@p5pRT
Copy link
Author

p5pRT commented Jan 28, 2012

From @jmdh

The attached patch addresses a problem reported as
<http​://bugs.debian.org/cgi-bin/bugreport.cgi?bug=656869> where, even
though no limit is defined in GNU/Hurd, MAXPATHLEN is limited to 1024.

Increasing this to 4096 creates parity between GNU/Hurd and
GNU/Linux. I'd be interested in opinions about whether this is sensible
to apply.

Thanks,
Dominic.

--
Dominic Hargreaves | http​://www.larted.org.uk/~dom/
PGP key 5178E2A5 from the.earth.li (keyserver,web,email)

@p5pRT
Copy link
Author

p5pRT commented Jan 28, 2012

From @jmdh

0001-Increase-the-fallback-value-of-MAXPATHLEN.patch
From cebdc646ef3126f43ae87d56da6d504fa80e4088 Mon Sep 17 00:00:00 2001
From: Dominic Hargreaves <dom@earth.li>
Date: Sat, 28 Jan 2012 14:00:39 +0000
Subject: [PATCH] Increase the fallback value of MAXPATHLEN

On systems without MAXPATHLEN or PATH_MAX defined (GNU/Hurd is an example
of such a system), set MAXPATHLEN to 4096 rather than 1024; this increase
creates parity with Linux.
---
 ext/File-Glob/bsd_glob.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ext/File-Glob/bsd_glob.c b/ext/File-Glob/bsd_glob.c
index 5019af1..1e9ead3 100644
--- a/ext/File-Glob/bsd_glob.c
+++ b/ext/File-Glob/bsd_glob.c
@@ -83,7 +83,7 @@ static char sscsid[]=  "$OpenBSD: glob.c,v 1.8.10.1 2001/04/10 jason Exp $";
 #  ifdef PATH_MAX
 #    define	MAXPATHLEN	PATH_MAX
 #  else
-#    define	MAXPATHLEN	1024
+#    define	MAXPATHLEN	4096
 #  endif
 #endif
 
-- 
1.7.7.3

@p5pRT
Copy link
Author

p5pRT commented Jan 28, 2012

From @cpansprout

On Sat Jan 28 06​:22​:08 2012, dom wrote​:

The attached patch addresses a problem reported as
<http​://bugs.debian.org/cgi-bin/bugreport.cgi?bug=656869> where, even
though no limit is defined in GNU/Hurd, MAXPATHLEN is limited to 1024.

Increasing this to 4096 creates parity between GNU/Hurd and
GNU/Linux. I'd be interested in opinions about whether this is sensible
to apply.

I’m not an expert in this area (in fact, I’m not an expert in any OS
interaction), but as an outsider, if you will, I must ask​: What happens
if MAXPATHLEN is set to a number larger than the system supports? Would
that result in a buffer overrun, or merely an unnecessarily large
allocation?

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jan 28, 2012

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

@p5pRT
Copy link
Author

p5pRT commented Jan 28, 2012

From @Leont

On Sat, Jan 28, 2012 at 9​:22 PM, Father Chrysostomos via RT
<perlbug-followup@​perl.org> wrote​:

I’m not an expert in this area (in fact, I’m not an expert in any OS
interaction), but as an outsider, if you will, I must ask​:  What happens
if MAXPATHLEN is set to a number larger than the system supports?  Would
that result in a buffer overrun, or merely an unnecessarily large
allocation?

In POSIX, system calls taking a path should return a ENAMETOOLONG
error in such a case.

Leon

@p5pRT
Copy link
Author

p5pRT commented Jan 28, 2012

From Mark@Overmeer.net

* Dominic Hargreaves (perlbug-followup@​perl.org) [120128 14​:22]​:

# New Ticket Created by Dominic Hargreaves
# Please include the string​: [perl #109262]
# in the subject line of all future correspondence about this issue.
# <URL​: https://rt-archive.perl.org/perl5/Ticket/Display.html?id=109262 >

The attached patch addresses a problem reported as
<http​://bugs.debian.org/cgi-bin/bugreport.cgi?bug=656869> where, even
though no limit is defined in GNU/Hurd, MAXPATHLEN is limited to 1024.

Increasing this to 4096 creates parity between GNU/Hurd and
GNU/Linux. I'd be interested in opinions about whether this is sensible
to apply.

The C constant _POSIX_PATH_MAX gives the minimal value for PATH_MAX to
be POSIX compliant (now 256, in the earlier posix versions 255)
Does pathconf('/etc',_PC_PATH_MAX) produce a nice value?
--
Regards,

  MarkOv


  Mark Overmeer MSc MARKOV Solutions
  Mark@​Overmeer.net solutions@​overmeer.net
http​://Mark.Overmeer.net http​://solutions.overmeer.net

@p5pRT
Copy link
Author

p5pRT commented Jan 28, 2012

From @jmdh

On Sat, Jan 28, 2012 at 02​:42​:17PM -0800, Mark Overmeer via RT wrote​:

* Dominic Hargreaves (perlbug-followup@​perl.org) [120128 14​:22]​:

The attached patch addresses a problem reported as
<http​://bugs.debian.org/cgi-bin/bugreport.cgi?bug=656869> where, even
though no limit is defined in GNU/Hurd, MAXPATHLEN is limited to 1024.

Increasing this to 4096 creates parity between GNU/Hurd and
GNU/Linux. I'd be interested in opinions about whether this is sensible
to apply.

The C constant _POSIX_PATH_MAX gives the minimal value for PATH_MAX to
be POSIX compliant (now 256, in the earlier posix versions 255)
Does pathconf('/etc',_PC_PATH_MAX) produce a nice value?

No, on GNU/Hurd 'POSIX​::pathconf( "/etc", &POSIX​::_PC_PATH_MAX )'
returns undef.

There is some more about this issue generally at

<http​://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html>

--
Dominic Hargreaves | http​://www.larted.org.uk/~dom/
PGP key 5178E2A5 from the.earth.li (keyserver,web,email)

@p5pRT
Copy link
Author

p5pRT commented Jan 28, 2012

From Mark@Overmeer.net

* Dominic Hargreaves (dom@​earth.li) [120128 23​:09]​:

On Sat, Jan 28, 2012 at 02​:42​:17PM -0800, Mark Overmeer via RT wrote​:

* Dominic Hargreaves (perlbug-followup@​perl.org) [120128 14​:22]​:

No, on GNU/Hurd 'POSIX​::pathconf( "/etc", &POSIX​::_PC_PATH_MAX )'
returns undef.

<http​://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html>

Not only PATH_MAX, but also many other global constants do not really
make sense. Especially the ancient ones. Pathconf() would stand a better
chance, because the length is obviously file-system dependent. If I
read the Hurd idea about it, I would say that it supports
  pathconf($fn, _PC_PATH_MAX)
returning "undef" as "unlimited".

You like to enlarge the default for MAXPATHLEN to an other artificial
value, where also a known PATH_MAX is probably too small. So, why not
remove the attempt to get a realistic value from the OS here?

  #define ALWAYS_LARGE_ENOUGH 32768
  Char *bufnext, *bufend, patbuf[ALWAYS_LARGE_ENOUGH];
  bufend = bufnext + ALWAYS_LARGE_ENOUGH - 1;

--
  MarkOv


drs Mark A.C.J. Overmeer MARKOV Solutions
  Mark@​Overmeer.net solutions@​overmeer.net
http​://Mark.Overmeer.net http​://solutions.overmeer.net

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2012

From @jmdh

On Sat, Jan 28, 2012 at 03​:34​:49PM -0800, Mark Overmeer via RT wrote​:

* Dominic Hargreaves (dom@​earth.li) [120128 23​:09]​:

On Sat, Jan 28, 2012 at 02​:42​:17PM -0800, Mark Overmeer via RT wrote​:

* Dominic Hargreaves (perlbug-followup@​perl.org) [120128 14​:22]​:

No, on GNU/Hurd 'POSIX​::pathconf( "/etc", &POSIX​::_PC_PATH_MAX )'
returns undef.

<http​://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html>

Not only PATH_MAX, but also many other global constants do not really
make sense. Especially the ancient ones. Pathconf() would stand a better
chance, because the length is obviously file-system dependent. If I
read the Hurd idea about it, I would say that it supports
pathconf($fn, _PC_PATH_MAX)
returning "undef" as "unlimited".

You like to enlarge the default for MAXPATHLEN to an other artificial
value, where also a known PATH_MAX is probably too small. So, why not
remove the attempt to get a realistic value from the OS here?

#define ALWAYS_LARGE_ENOUGH 32768
Char *bufnext, *bufend, patbuf[ALWAYS_LARGE_ENOUGH];
bufend = bufnext + ALWAYS_LARGE_ENOUGH - 1;

As I understand it, you're suggesting picking a different arbitrary value
as a maximum, and using it even when the underlying system wouldn't
support paths that long. I'm not convinced that this is an improvement
on my suggestion. Have I missed something?

Cheers,
Dominic.

--
Dominic Hargreaves | http​://www.larted.org.uk/~dom/
PGP key 5178E2A5 from the.earth.li (keyserver,web,email)

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2012

From Mark@Overmeer.net

* Dominic Hargreaves (dom@​earth.li) [120129 15​:20]​:

* Dominic Hargreaves (dom@​earth.li) [120128 23​:09]​:
<http​://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html>

On Sat, Jan 28, 2012 at 03​:34​:49PM -0800, Mark Overmeer via RT wrote​:

You like to enlarge the default for MAXPATHLEN to an other artificial
value, where also a known PATH_MAX is probably too small. So, why not
remove the attempt to get a realistic value from the OS here?
#define ALWAYS_LARGE_ENOUGH 32768
Char *bufnext, *bufend, patbuf[ALWAYS_LARGE_ENOUGH];
bufend = bufnext + ALWAYS_LARGE_ENOUGH - 1;

As I understand it, you're suggesting picking a different arbitrary value
as a maximum, and using it even when the underlying system wouldn't
support paths that long. I'm not convinced that this is an improvement
on my suggestion. Have I missed something?

The document you pointed us to, and documents referred to in that doc,
suggest that PATH_MAX/MAXPATHLEN are constants much shorter than
the actual maximum size of the path. So, why should the buffer used
in bsd_glob.c be smaller than may be needed? In lack of any useful
constant with a realistic value, we can just pick something huge (in
this context)
--
Regards,
  MarkOv


  Mark Overmeer MSc MARKOV Solutions
  Mark@​Overmeer.net solutions@​overmeer.net
http​://Mark.Overmeer.net http​://solutions.overmeer.net

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2012

From @jmdh

On Sun, Jan 29, 2012 at 09​:01​:35AM -0800, Mark Overmeer via RT wrote​:

* Dominic Hargreaves (dom@​earth.li) [120129 15​:20]​:

* Dominic Hargreaves (dom@​earth.li) [120128 23​:09]​:
<http​://www.gnu.org/software/hurd/community/gsoc/project_ideas/maxpath.html>

On Sat, Jan 28, 2012 at 03​:34​:49PM -0800, Mark Overmeer via RT wrote​:

You like to enlarge the default for MAXPATHLEN to an other artificial
value, where also a known PATH_MAX is probably too small. So, why not
remove the attempt to get a realistic value from the OS here?
#define ALWAYS_LARGE_ENOUGH 32768
Char *bufnext, *bufend, patbuf[ALWAYS_LARGE_ENOUGH];
bufend = bufnext + ALWAYS_LARGE_ENOUGH - 1;

As I understand it, you're suggesting picking a different arbitrary value
as a maximum, and using it even when the underlying system wouldn't
support paths that long. I'm not convinced that this is an improvement
on my suggestion. Have I missed something?

The document you pointed us to, and documents referred to in that doc,
suggest that PATH_MAX/MAXPATHLEN are constants much shorter than
the actual maximum size of the path.

So, why should the buffer used
in bsd_glob.c be smaller than may be needed? In lack of any useful
constant with a realistic value, we can just pick something huge (in
this context)

Right, I see that you're referring to
<http​://insanecoding.blogspot.com/2007/11/pathmax-simply-isnt.html>
which mentions how it's possible to create paths longer than PATH_MAX,
on systems which define PATH_MAX but which aren't Windows, but that doing
so risks not being able to manipulate the path with anything which uses
PATH_MAX. I'm not sure I want to be the one who enables that in perl,
even if it theoretically correct.

My choice of 4096 was a trade off based on what people seem to expect
(because of other systems' limits) without inflating the size of the
buffer unnecessarily.

In an ideal world, there wouldn't be a fixed size buffer here at all,
but in the absence, the minimal change I suggested still seems like the
right thing to do.

--
Dominic Hargreaves | http​://www.larted.org.uk/~dom/
PGP key 5178E2A5 from the.earth.li (keyserver,web,email)

@p5pRT
Copy link
Author

p5pRT commented Jan 29, 2012

From solutions@overmeer.net

* Dominic Hargreaves (dom@​earth.li) [120129 18​:55]​:

PATH_MAX. I'm not sure I want to be the one who enables that in perl,
even if it theoretically correct.

It's not "in Perl", but only "in glob" (at least the bug report)
Whether (for instance) chown can handle long path or not is an
other thing.
--
Regards,
  MarkOv


  Mark Overmeer MSc MARKOV Solutions
  Mark@​Overmeer.net solutions@​overmeer.net
http​://Mark.Overmeer.net http​://solutions.overmeer.net

@p5pRT
Copy link
Author

p5pRT commented Jan 30, 2012

From @jmdh

On Sun, Jan 29, 2012 at 12​:39​:39PM -0800, Mark Overmeer via RT wrote​:

* Dominic Hargreaves (dom@​earth.li) [120129 18​:55]​:

PATH_MAX. I'm not sure I want to be the one who enables that in perl,
even if it theoretically correct.

It's not "in Perl", but only "in glob" (at least the bug report)
Whether (for instance) chown can handle long path or not is an
other thing.

That's pretty much my point. Increasing the fallback value is a quick
win for GNU/Hurd users, without risking breaking other parts of the
system.

--
Dominic Hargreaves | http​://www.larted.org.uk/~dom/
PGP key 5178E2A5 from the.earth.li (keyserver,web,email)

@p5pRT
Copy link
Author

p5pRT commented Jan 31, 2012

From @cpansprout

On Mon Jan 30 14​:44​:05 2012, dom wrote​:

On Sun, Jan 29, 2012 at 12​:39​:39PM -0800, Mark Overmeer via RT wrote​:

* Dominic Hargreaves (dom@​earth.li) [120129 18​:55]​:

PATH_MAX. I'm not sure I want to be the one who enables that in perl,
even if it theoretically correct.

It's not "in Perl", but only "in glob" (at least the bug report)
Whether (for instance) chown can handle long path or not is an
other thing.

That's pretty much my point. Increasing the fallback value is a quick
win for GNU/Hurd users, without risking breaking other parts of the
system.

I think that’s a good enough argument for this small change. I’ve
applied it as ffa23ac. Thank you.

--

Father Chrysostomos

@p5pRT
Copy link
Author

p5pRT commented Jan 31, 2012

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

@p5pRT
Copy link
Author

p5pRT commented Jan 31, 2012

From @jmdh

On Mon, Jan 30, 2012 at 04​:01​:57PM -0800, Father Chrysostomos via RT wrote​:

On Mon Jan 30 14​:44​:05 2012, dom wrote​:

On Sun, Jan 29, 2012 at 12​:39​:39PM -0800, Mark Overmeer via RT wrote​:

* Dominic Hargreaves (dom@​earth.li) [120129 18​:55]​:

PATH_MAX. I'm not sure I want to be the one who enables that in perl,
even if it theoretically correct.

It's not "in Perl", but only "in glob" (at least the bug report)
Whether (for instance) chown can handle long path or not is an
other thing.

That's pretty much my point. Increasing the fallback value is a quick
win for GNU/Hurd users, without risking breaking other parts of the
system.

I think that’s a good enough argument for this small change. I’ve
applied it as ffa23ac. Thank you.

Unfortunately, and rather embarrassingly, I only just[1] received the
mail[2] sent on Sunday pointing out that that fix isn't sufficient.
Mea culpa for not verifying the fix properly :(

It's likely, therefore, that another commit will be needed, once we've
figured out what makes sense.

Dominic.

[1] Debian infrastructure problems
[2] <http​://bugs.debian.org/cgi-bin/bugreport.cgi?bug=656869#16>

--
Dominic Hargreaves | http​://www.larted.org.uk/~dom/
PGP key 5178E2A5 from the.earth.li (keyserver,web,email)

@p5pRT
Copy link
Author

p5pRT commented Feb 3, 2012

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

@p5pRT
Copy link
Author

p5pRT commented Feb 3, 2012

From @jmdh

On Tue, Jan 31, 2012 at 09​:55​:38AM +0000, Dominic Hargreaves wrote​:

On Mon, Jan 30, 2012 at 04​:01​:57PM -0800, Father Chrysostomos via RT wrote​:

On Mon Jan 30 14​:44​:05 2012, dom wrote​:

On Sun, Jan 29, 2012 at 12​:39​:39PM -0800, Mark Overmeer via RT wrote​:

* Dominic Hargreaves (dom@​earth.li) [120129 18​:55]​:

PATH_MAX. I'm not sure I want to be the one who enables that in perl,
even if it theoretically correct.

It's not "in Perl", but only "in glob" (at least the bug report)
Whether (for instance) chown can handle long path or not is an
other thing.

That's pretty much my point. Increasing the fallback value is a quick
win for GNU/Hurd users, without risking breaking other parts of the
system.

I think that’s a good enough argument for this small change. I’ve
applied it as ffa23ac. Thank you.

Unfortunately, and rather embarrassingly, I only just[1] received the
mail[2] sent on Sunday pointing out that that fix isn't sufficient.
Mea culpa for not verifying the fix properly :(

It's likely, therefore, that another commit will be needed, once we've
figured out what makes sense.

Okay, so perl.h has its own fallback definition of MAXPATHLEN, which,
if PATH_MAX is not defined, uses _POSIX_PATH_MAX. Samuel suggests at
[1] that this is in error; _POSIX_PATH_MAX is defined[2] to be 256,
whilst PATH_MAX is not required to be defined.

Therefore, my new proposed fix is attached, along with a reversion
of the previous patch which you kindly committed.

I actually have a commit bit now, so if I can get positive feedback
on the attached, I'll push that out to blead.

I've now actually verified this against a test case, also attached; any
thoughts about whether this would make sense to add to the test suite?

Thanks,
Dominic.

[1] <http​://bugs.debian.org/cgi-bin/bugreport.cgi?bug=656869>
[2] <http​://pubs.opengroup.org/onlinepubs/9699919799/basedefs/limits.h.html>

--
Dominic Hargreaves | http​://www.larted.org.uk/~dom/
PGP key 5178E2A5 from the.earth.li (keyserver,web,email)

@p5pRT
Copy link
Author

p5pRT commented Feb 3, 2012

From @jmdh

0001-Don-t-use-_POSIX_PATH_MAX-as-a-fallback-PATH_MAX.patch
From 27f1ced91c044ec133b3def3bef964bbd52706f6 Mon Sep 17 00:00:00 2001
From: Dominic Hargreaves <dom@earth.li>
Date: Fri, 3 Feb 2012 19:35:36 +0000
Subject: [PATCH 1/2] Don't use _POSIX_PATH_MAX as a fallback PATH_MAX

_POSIX_PATH_MAX is required to be defined for POSIX systems as
256, which is too small for a fallback PATH_MAX (some systems, such as
GNU/Hurd, do not have an inherent limit on path length).
---
 perl.h |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/perl.h b/perl.h
index fe1eaec..3448a2b 100644
--- a/perl.h
+++ b/perl.h
@@ -2764,11 +2764,7 @@ freeing any remaining Perl interpreters.
 #      define MAXPATHLEN (PATH_MAX+1)
 #    endif
 #  else
-#    ifdef _POSIX_PATH_MAX
-#       define MAXPATHLEN _POSIX_PATH_MAX
-#    else
-#       define MAXPATHLEN 1024	/* Err on the large side. */
-#    endif
+#    define MAXPATHLEN 1024	/* Err on the large side. */
 #  endif
 #endif
 
-- 
1.7.8.3

@p5pRT
Copy link
Author

p5pRT commented Feb 3, 2012

From @jmdh

0002-Revert-Increase-the-fallback-value-of-MAXPATHLEN.patch
From 9973f328375f257eb400846f7bd00ed5e687b7ad Mon Sep 17 00:00:00 2001
From: Dominic Hargreaves <dom@earth.li>
Date: Fri, 3 Feb 2012 19:40:30 +0000
Subject: [PATCH 2/2] Revert "Increase the fallback value of MAXPATHLEN"

This reverts commit ffa23acf6bf9670bd1d5fdc9a958c918b6cf3d06.

This change was made without realisation that the fallback value for
MAXPATHLEN on POSIX systems comes (in perl.h) from _POSIX_MAX_PATH,
so the fallback value here was not used.
---
 ext/File-Glob/bsd_glob.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/ext/File-Glob/bsd_glob.c b/ext/File-Glob/bsd_glob.c
index 1e9ead3..5019af1 100644
--- a/ext/File-Glob/bsd_glob.c
+++ b/ext/File-Glob/bsd_glob.c
@@ -83,7 +83,7 @@ static char sscsid[]=  "$OpenBSD: glob.c,v 1.8.10.1 2001/04/10 jason Exp $";
 #  ifdef PATH_MAX
 #    define	MAXPATHLEN	PATH_MAX
 #  else
-#    define	MAXPATHLEN	4096
+#    define	MAXPATHLEN	1024
 #  endif
 #endif
 
-- 
1.7.8.3

@p5pRT
Copy link
Author

p5pRT commented Feb 3, 2012

From @jmdh

test.sh

@p5pRT
Copy link
Author

p5pRT commented Feb 16, 2012

From @rjbs

Thanks Dominic, please go ahead.

@p5pRT
Copy link
Author

p5pRT commented Feb 18, 2012

From @jmdh

Committed to blead.

@p5pRT
Copy link
Author

p5pRT commented Feb 18, 2012

From [Unknown Contact. See original ticket]

Committed to blead.

@p5pRT
Copy link
Author

p5pRT commented Feb 18, 2012

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

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