[hurd] Fix unconditional use of PATH_MAX

[sthibaul]

Summary

The GNU/Hurd system does not define an arbitrary PATH_MAX limitation, the POSIX 2001 realpath extension can be used instead, and the size of symlinks can be determined.

[ldionne]

src/filesystem/operations.cpp

533

Is the extension available on old glibc's? Should that be #if _POSIX_VERSION >= 200112 && defined(__GLIBC__), or just #if _POSIX_VERSION >= 200112?

535

You could just initialize char *buff = ::realpath(...) and then make the check in the if?

[sthibaul]

src/filesystem/operations.cpp

533

The extension was added to glibc in 1996. Do we need to care about that old a version? :)

We could also just keep _POSIX_VERSION >= 200112 only, which was defined in glibc in 2003.

535

Well, sure, I was just following the coding style below, what do you prefer?

[ldionne]

src/filesystem/operations.cpp

533

The extension was added to glibc in 1996. Do we need to care about that old a version? :)

Probably not. I hope we don't.

We could also just keep _POSIX_VERSION >= 200112 only, which was defined in glibc in 2003.

That would be better IMO, and the only drawback is that we won't catch a GLIBC version released between 1996 and 2003 (which we don't care about).

535

Oh... leave as-is then.

[sthibaul]

Here is the test for realpath extension fixed.

[ldionne]

src/filesystem/operations.cpp

1089

Is lstat always available? Also, you should use ::lstat for consistency with the rest of the code.

1093

Is the reason why you're not using a std::unique_ptr<char[]> here because you don't want it to throw in case of no memory?

[sthibaul]

src/filesystem/operations.cpp

1089

lstat has been in posix etc. since first releases

1093

No, it was just that I'm not used to c++ programming so didn't realize it could be used here.

[ldionne]

LGTM at a superficial level, I'd like @EricWF to chime in since he's the Filesystem grand master.

src/filesystem/operations.cpp

1106–1108

You can just use buf[ret] = '\0'.

[EricWF]

src/filesystem/operations.cpp

1089

I don't love this implementation. It requires an additional system call, and it opens us up to a TOCTOU bug.
I would prefer to still use PATH_MAX if it's defined (and meaningful).

Otherwise, then we can call back on the lstat version, but we should do better error reporting when ret == size.

[sthibaul]

I have updated the patch according to the comment.

[sylvestre.ledru]

@EricWF is it ok to land? Thanks

[EricWF]

src/filesystem/operations.cpp

535

s/NULL/nullptr/

536

I think we can RAII this up like:

std::unique_ptr<char, decltype(&::free)> hold(::realpath(p.c_str(), nullptr), &::free);`
if (hold.get() == nullptr)
  return err.report(capture_errno());
return {hold.get()};

1087

I would like to see these two paths merged even further. How about:

path __read_symlink(const path& p, error_code* ec) {
  ErrorHandler<path> err("read_symlink", ec, &p);
#ifdef PATH_MAX
  struct NullDeleter { void operator()(void*) const {} };
  const size_t size = PATH_MAX + 1;
  char stack_buff[size];
  auto buff = std::unique_ptr<char[], NullDeleter>(stack_buff);
#else
  StatT sb;
   if (::lstat(p.c_str(), &sb) == -1) {
    return err.report(capture_errno());
  }
  const size_t size = sb.st_size + 1;
  auto buff = unique_ptr<char[]>(new char[size]);
#endif
  ::ssize_t ret;
  if ((ret = ::readlink(p.c_str(), buff.get(), size)) == -1) {
    return err.report(capture_errno());
  }
  _LIBCPP_ASSERT(ret <= size, "TODO");
  _LIBCPP_ASSERT(ret > 0, "TODO");
  buff[ret] = 0;
  return {buff.get()};
}

1108

Use the StatT typedef we already have.

[sthibaul]

Right, that looks better indeed :)

Here is an updated patch.

[sthibaul]

Any news on this?

[EricWF]

Sorry for the delay. Committed as r351414. I'll look into merging it into the 8.0 release soon.

src/filesystem/operations.cpp

path __absolute(const path& p, error_code* ec) {
   return __do_absolute(p, &cwd, ec);
 }
 
 path __canonical(path const& orig_p, error_code* ec) {
   path cwd;
   ErrorHandler<path> err("canonical", ec, &orig_p, &cwd);
 
   path p = __do_absolute(orig_p, &cwd, ec);
+#if _POSIX_VERSION >= 200112
+  char *buff;
+  if ((buff = ::realpath(p.c_str(), NULL)) == nullptr)
+    return err.report(capture_errno());
+  path ret = {buff};
+  free(buff);
+  return ret;
+#else
   char buff[PATH_MAX + 1];
   char* ret;
   if ((ret = ::realpath(p.c_str(), buff)) == nullptr)
     return err.report(capture_errno());
   return {ret};
+#endif
 }
 
 void __copy(const path& from, const path& to, copy_options options,
             error_code* ec) {
   ErrorHandler<void> err("copy", ec, &from, &to);
 
   const bool sym_status = bool(
       options & (copy_options::create_symlinks | copy_options::skip_symlinks));
#else
   if (::chmod(p.c_str(), real_perms) == -1) {
     return err.report(capture_errno());
   }
 #endif
 }
 
 path __read_symlink(const path& p, error_code* ec) {
   ErrorHandler<path> err("read_symlink", ec, &p);
 
-  char buff[PATH_MAX + 1];
-  error_code m_ec;
+  struct stat sb;
+  if (lstat(p.c_str(), &sb) == -1) {
+    return err.report(capture_errno());
+  }
+  size_t size = sb.st_size + 1;
+  char *buff = (char*) malloc(size);
+  if (buff == NULL) {
+    return err.report(capture_errno());
+  }
+
   ::ssize_t ret;
-  if ((ret = ::readlink(p.c_str(), buff, PATH_MAX)) == -1) {
+  if ((ret = ::readlink(p.c_str(), buff, size)) == -1) {
+    free(buff);
     return err.report(capture_errno());
   }
-  _LIBCPP_ASSERT(ret <= PATH_MAX, "TODO");
+  _LIBCPP_ASSERT(ret < size, "TODO");
   _LIBCPP_ASSERT(ret > 0, "TODO");
   buff[ret] = 0;
-  return {buff};
+  path res = {buff};
+  free(buff);
+  return res;
 }
 
 bool __remove(const path& p, error_code* ec) {
   ErrorHandler<bool> err("remove", ec, &p);
   if (::remove(p.c_str()) == -1) {
     if (errno != ENOENT)
       err.report(capture_errno());
     return false;