Page MenuHomePhabricator

[SystemZ][ZOS] Provide PATH_MAX macro for libcxx
ClosedPublic

Authored by zibi on Nov 25 2020, 9:45 AM.

Details

Summary

Defining PATH_MAX to _XOPEN_PATH_MAX which is the closest macro available on z/OS.
Note that this value is 1024 which is 4 times smaller from same macro on Linux.

Diff Detail

Event Timeline

zibi created this revision.Nov 25 2020, 9:45 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptNov 25 2020, 9:45 AM
zibi requested review of this revision.Nov 25 2020, 9:45 AM
ldionne requested changes to this revision.Nov 26 2020, 9:20 AM
ldionne added inline comments.
libcxx/src/filesystem/operations.cpp
1193

We already try to workaround the lack of PATH_MAX. Where are you seeing errors?

This revision now requires changes to proceed.Nov 26 2020, 9:20 AM
zibi added a comment.Nov 26 2020, 11:06 AM

libcxx/src/filesystem/operations.cpp:549:13: error: use of undeclared identifier 'PATH_MAX'

char buff[PATH_MAX + 1];
          ^

1 error generated.

zibi added a comment.Nov 26 2020, 11:39 AM

Alternatively we can do this:

-#if defined(_POSIX_VERSION) && _POSIX_VERSION >= 200112
+#if defined(MVS) || (defined(_POSIX_VERSION) && _POSIX_VERSION >= 200112)

zibi added a comment.Nov 26 2020, 12:37 PM

Alternatively we can do this:

-#if defined(_POSIX_VERSION) && _POSIX_VERSION >= 200112
+#if defined(MVS) || (defined(_POSIX_VERSION) && _POSIX_VERSION >= 200112)

Actually we can not do this because out implementation of realpath does not do malloc.
We are forced to defined PATH_MAX as I originally proposed.

hubert.reinterpretcast added inline comments.
libcxx/src/filesystem/operations.cpp
52

I've been told that PATH_MAX is indeed limited to _XOPEN_PATH_MAX and that seems to be the case (based on experimentation) with realpath, but I have not seen documentation that supports such an assertion. The scope of this macro definition may be too broad as a solution of dealing with realpath.

zibi marked an inline comment as done.Dec 4 2020, 12:09 PM
zibi added inline comments.
libcxx/src/filesystem/operations.cpp
52

Will avoid defining the macro altogether.

zibi updated this revision to Diff 309610.Dec 4 2020, 12:15 PM
zibi marked an inline comment as done.

Avoid using PATH_MAX on z/OS.

Thanks; the targeted fix for handling realpath (where we have experimentally verified that the operation does fail with paths longer than 1024) seems reasonable to me. Looks good from my end.

libcxx/src/filesystem/operations.cpp
638

Minor nit: style.

ldionne requested changes to this revision.Dec 4 2020, 1:03 PM
ldionne added inline comments.
libcxx/src/filesystem/operations.cpp
638

Hardcoding buffer sizes like that is a great way to trigger a buffer overflow when assumptions are not respected. I would much rather we don't introduce this sort of code in libc++, to be honest. Is there a reason why zOS can't just provide PATH_MAX?

Or, even much better, why doesn't zOS satisfy defined(_POSIX_VERSION) && _POSIX_VERSION >= 200112? Then, we could use the safe and non-buggy version of realpath. The buffer-taking version is documented in various places, including realpath's manpage itself, as being buggy.

This revision now requires changes to proceed.Dec 4 2020, 1:03 PM
zibi updated this revision to Diff 309629.Dec 4 2020, 1:05 PM
zibi marked an inline comment as done.

Handing nit.

libcxx/src/filesystem/operations.cpp
638

Hardcoding buffer sizes like that is a great way to trigger a buffer overflow when assumptions are not respected.

Precisely because violating the assumptions mean that buffer overflow would be introduced into compiled programs, changes that would cause the assumption to not be respected cannot realistically be made.

I would much rather we don't introduce this sort of code in libc++, to be honest. Is there a reason why zOS can't just provide PATH_MAX?

The POSIX specification provides rationale for not defining PATH_MAX in <limits.h>.

Or, even much better, why doesn't zOS satisfy defined(_POSIX_VERSION) && _POSIX_VERSION >= 200112? Then, we could use the safe and non-buggy version of realpath. The buffer-taking version is documented in various places, including realpath's manpage itself, as being buggy.

Deployed systems have stability considerations that may be incompatible with such a change. The IBM documentation for released versions clearly state that passing NULL to the second parameter produces an error. Indeed the bugginess of the buffer-taking version is why the hardcoded 1024 that the implementation of realpath on z/OS uses is preferable to an implementation that does not implement a fixed upper bound on the buffer-taking form of realpath.

ldionne added inline comments.Dec 8 2020, 8:49 AM
libcxx/src/filesystem/operations.cpp
638

What are your deployment goals? What versions of z/OS do you want libc++ to work on? Also, where's the documentation you're referring to?

libcxx/src/filesystem/operations.cpp
638

re: EINVAL when using NULL for the second argument, see:
https://www.ibm.com/support/knowledgecenter/en/SSLTBW_2.4.0/com.ibm.zos.v2r4.bpxbd00/rpath.htm

re: ENAMETOOLONG, the fixed limits is documented for what could be thought of as the "system call interface":
https://www.ibm.com/support/knowledgecenter/en/SSLTBW_2.4.0/com.ibm.zos.v2r4.bpxb100/rph.htm

re: deployment goals, I am not in a position to provide that information (@zibi, can you say more?)

zibi marked 3 inline comments as done.Dec 8 2020, 11:45 AM
zibi added inline comments.
libcxx/src/filesystem/operations.cpp
638

Unfortunately we do not have the safe version of realpath and the value of _POSIZ_VERSOIN is much lower. The deployment goals are still in flux.

Here is another proposal:

#if defines(__MVS__) && !defined(PATH_MAX) 
  char buff[ _XOPEN_PATH_MAX + 1 ];
zibi added a comment.Dec 9 2020, 7:08 AM

On z/OS we have multiple file systems with different limits which does not allow to have just one constant for PATH_MAX. Same is true for other related limits like NAME_MAX. See the following references which justify the z/OS choice.

POSIX.1 document:
The values in the following list may be constants within an implementation or may vary from one pathname to another. For example, file systems or directories may have different characteristics.

A definition of one of the symbolic constants in the following list shall be omitted from the <limits.h> header on specific implementations where the corresponding value is equal to or greater than the stated minimum, but where the value can vary depending on the file to which it is applied. The actual value supported for a specific pathname shall be provided by the pathconf() function.

GNU document
If the system allows different file systems or files to have different limits, then the macro is undefined; use pathconf or fpathconf to find out the limit that applies to a particular file. See Pathconf.

zibi updated this revision to Diff 314415.Jan 4 2021, 11:36 AM

We have to use _XOPEN_PATH_MAX instead of PATH_MAX on z/OS.

zibi updated this revision to Diff 314417.Jan 4 2021, 11:41 AM

fixing typo

libcxx/src/filesystem/operations.cpp
638

I don't see why _XOPEN_PATH_MAX is better. Yes, it is defined to have 1024 as its value, but it's not defined (in purpose) to be directly related for this usage. The realpath callable service describes the maximum length returned without \0 termination as literally 1023 with no indirection of referring to some named constant involved.

zibi marked an inline comment as done.Jan 6 2021, 7:27 AM
zibi added inline comments.
libcxx/src/filesystem/operations.cpp
638

The only advantage is that _XOPEN_PATH_MAX is defined by system header outside of LLVM with the value we want to use anyway and avoid hardcoding the value here. However, I'm fine with any proposal as long we approve and move on.

zibi marked an inline comment as done.Jan 13 2021, 8:42 AM

@ldionne which suggestion do you prefer?

ldionne requested changes to this revision.Jan 13 2021, 10:38 AM

@ldionne which suggestion do you prefer?

Neither.

The documentation Hubert pointed to clearly mentions that PATH_MAX can be used, so I don't understand the issue. If the issue is that you want to be able to compile libc++ on systems so old that they don't have PATH_MAX defined even though the documentation says it is provided, that doesn't sound compelling to me.

This revision now requires changes to proceed.Jan 13 2021, 10:38 AM
zibi added a comment.Jan 13 2021, 11:26 AM

Neither.

The documentation Hubert pointed to clearly mentions that PATH_MAX can be used, so I don't understand the issue. If the issue is that you want to be able to compile libc++ on systems so old that they don't have PATH_MAX defined even though the documentation says it is provided, that doesn't sound compelling to me.

I'm sorry if the comments mislead you but the fact is that PATH_MAX is NOT provided on z/OS for the reasons I provided on Dec 9, 2020.
We have to use hard-coded value 1024 or _XOPEN_PATH_MAX macro. I discussed this issue internally the rational was provided before on Dec. 9.

zibi updated this revision to Diff 316822.Jan 14 2021, 6:15 PM

rebase and resubmit to fix the build

ldionne accepted this revision.Jan 18 2021, 9:37 AM

This is why i think that's okay (from the documentation you linked on Dec 9):

{PATH_MAX}
Maximum number of bytes the implementation will store as a pathname in a user-supplied buffer of unspecified size, including the terminating null character. Minimum number the implementation will accept as the maximum number of bytes in a pathname.
Minimum Acceptable Value: {_POSIX_PATH_MAX}

I still think it would be better to use realpath instead and bump your OS requirements (since we might want to drop the PATH_MAX codepath in libcxx eventually). But enough time has been wasted for such a small change.

This revision is now accepted and ready to land.Jan 18 2021, 9:37 AM
This revision was automatically updated to reflect the committed changes.