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

[Hubert Tong via Phabricator via libcxx-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: libcxx/src/filesystem/operations.cpp:545
+  #if defined(__MVS__)
+    char buff[1024 +1];
+  #else
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92110