This is an archive of the discontinued LLVM Phabricator instance.

[hurd] Fix unconditional use of PATH_MAX
ClosedPublic

Authored by sthibaul on Nov 18 2018, 7:37 AM.

Details

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.

Diff Detail

Repository
rCXX libc++

Event Timeline

sthibaul created this revision.Nov 18 2018, 7:37 AM
ldionne added inline comments.Nov 19 2018, 8:05 AM
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 added inline comments.Nov 19 2018, 1:31 PM
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 added inline comments.Nov 19 2018, 2:09 PM
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 updated this revision to Diff 174680.Nov 19 2018, 2:11 PM

Here is the test for realpath extension fixed.

sthibaul marked 6 inline comments as done.Nov 19 2018, 2:12 PM
ldionne added inline comments.Nov 19 2018, 3:29 PM
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 updated this revision to Diff 174705.Nov 19 2018, 4:30 PM
sthibaul updated this revision to Diff 174707.
sthibaul marked 2 inline comments as done.Nov 19 2018, 4:32 PM
sthibaul added inline comments.
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.

sthibaul marked 4 inline comments as done.Nov 19 2018, 4:32 PM
ldionne accepted this revision.Nov 19 2018, 4:40 PM
ldionne added 1 blocking reviewer(s): EricWF.

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'.

sthibaul updated this revision to Diff 174711.Nov 19 2018, 4:53 PM
sthibaul marked an inline comment as done.
EricWF requested changes to this revision.Nov 26 2018, 10:24 AM
EricWF added inline comments.
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.

This revision now requires changes to proceed.Nov 26 2018, 10:24 AM
sthibaul updated this revision to Diff 175551.Nov 27 2018, 11:58 AM
sthibaul marked an inline comment as done.

I have updated the patch according to the comment.

EricWF added inline comments.Dec 10 2018, 9:49 AM
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 updated this revision to Diff 178808.Dec 18 2018, 3:40 PM

Right, that looks better indeed :)

Here is an updated patch.

sthibaul marked 4 inline comments as done.Dec 18 2018, 3:40 PM

Any news on this?

EricWF accepted this revision.Jan 16 2019, 7:03 PM

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

This revision is now accepted and ready to land.Jan 16 2019, 7:03 PM
EricWF closed this revision.Jan 16 2019, 7:03 PM