[libcxx-commits] [PATCH] D92110: [SystemZ][ZOS] Provide PATH_MAX macro for libcxx

[Zbigniew Sarbinowski via Phabricator via libcxx-commits]

zibi marked an inline comment as done.
zibi added inline comments.


================
Comment at: libcxx/src/filesystem/operations.cpp:545
+  #if defined(__MVS__)
+    char buff[1024 +1];
+  #else
----------------
hubert.reinterpretcast wrote:
> zibi wrote:
> > hubert.reinterpretcast wrote:
> > > ldionne wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > ldionne wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > Minor nit: style.
> > > > > > 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.
> > > > > > 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`.
> > > > > 
> > > > 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?
> > > 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?)
> > 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 ];
> > ```
> 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.
> 
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. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92110/new/

https://reviews.llvm.org/D92110