libc++: Add option to disable access to the global filesystem namespace

[ed]

Summary

Systems like FreeBSD's Capsicum and Nuxi CloudABI apply the concept of capability-based security on the way processes can interact with the filesystem API. It is no longer to interact with the VFS through calls like open(), unlink(), rename(), etc. Instead, processes are only allowed to interact with files and directories to which they have been granted access. The *at() functions can be used for this purpose.

This change adds a new config switch called _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE. If set, all functionality that requires the global filesystem namespace will be disabled. More concretely:

• fstream's open() function will be removed.
• cstdio will no longer pull in fopen(), rename(), etc.
• The test suite's get_temp_file_name() will be removed. This will cause all tests that use the global filesystem namespace to break, but will at least make all the other tests run (as get_temp_file_name will not build anyway).

It is important to mention that this change will make fstream rather useless on those systems for now. Still, I'd rather not have fstream disabled entirely, as it is of course possible to come up with an extension for fstream that would allow access to local filesystem namespaces (e.g., by adding an openat() member function).

[jroelofs]

Seems reasonable to me. I think you ought to fix a few of the tests that this "breaks", and add a few *.fail.cpp ones to verify that the functions you expect to be removed have actually been removed. For example ./test/std/input.output/file.streams/c.files/cstdio.pass.cpp should have:

static_assert((std::is_same<decltype(std::fflush(fp)), int>::value), "");
#if defined(_LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE)
static_assert((std::is_same<decltype(std::fopen("", "")), std::FILE*>::value), "");
static_assert((std::is_same<decltype(std::freopen("", "", fp)), std::FILE*>::value), "");
#endif
static_assert((std::is_same<decltype(std::setbuf(fp,cp)), void>::value), "");

And then I'd add a ./test/std/input.output/file.streams/c.files/no.global.filesystem.namespace/fopen.fail.cpp with:

// REQUIRES: CloudABI

#include <cstdio>

int main() {
    // fopen is not allowed on systems without the global filesystem namespace
    std::fopen("", "");
}

[ed]

In D8194#137640, @jroelofs wrote:

Seems reasonable to me. I think you ought to fix a few of the tests that this "breaks", and add a few *.fail.cpp ones to verify that the functions you expect to be removed have actually been removed. For example ./test/std/input.output/file.streams/c.files/cstdio.pass.cpp should have:

static_assert((std::is_same<decltype(std::fflush(fp)), int>::value), "");
#if defined(_LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE)
static_assert((std::is_same<decltype(std::fopen("", "")), std::FILE*>::value), "");
static_assert((std::is_same<decltype(std::freopen("", "", fp)), std::FILE*>::value), "");
#endif
static_assert((std::is_same<decltype(std::setbuf(fp,cp)), void>::value), "");

And then I'd add a ./test/std/input.output/file.streams/c.files/no.global.filesystem.namespace/fopen.fail.cpp with:

// REQUIRES: CloudABI

#include <cstdio>

int main() {
    // fopen is not allowed on systems without the global filesystem namespace
    std::fopen("", "");
}

Yes, exactly! I was planning on doing something similar to what you've described. Good to see that I'm starting to get the hang of it.

So my current roadmap is to first get libc++ to build on CloudABI before I attempt to tackle the test suite. In my opinion it doesn't make sense to patch up the test suite if the tests cannot be run decently. Being able to build libc++ would be a prerequisite. Getting all the tests to pass followed by setting up a buildbot will be my next milestones.

Does that approach sound reasonable to you?

Thanks,
Ed

[ed]

Hi Jonathan,

As you proposed, I have inverted the logic. We now have a flag _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE. I temporarily added -D_LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE to test/libcxx/test/config.py, so I've been able to mark all the tests that require the global filesystem namespace.

I won't add anything permanent to config.py yet. There may still be a number of other flags I'll need to add. My plan is to tailor config.py as soon as libc++ builds entirely.

[jroelofs]

I'd still like to see some test cases added (like the ones I mentioned before), as well as the changes needed to config.py, lit.site.in, CMakeLists.txt, etc. so that this can be tested on platforms that do have the global filesystem namespace. Then it should be simple to add another buildbot that checks these, without having to get a CloudABI buildbot running (I understand that's part of your plan, this keeps them from bit-rotting in the meantime).

Also, since you're basically marking everything in test/std/input.output/file.streams/fstreams/ as unsupported, it would be better to add:

test/std/input.output/file.streams/fstreams/lit.local.cfg
test/std/localization/locales/locale.convenience/conversion/conversions.buffer/lit.local.cfg

with this in them:

if 'libcpp-has-no-global-filesystem-namespace' in config.available_features:
    config.unsupported = True

rather than putting // UNSUPPORTED:s everywhere. They get yucky to maintain when other folks add new test files, and this is a mistake I made when I did the ones for the single-threaded stuff... I still need to fix that. Sorry I didn't mention this earlier.

Cheers,

Jon

[ed]

Hi Jonathan,

This is interesting. Today I spent quite some time getting lit to do what I want but it seems I'm running into the issue that lit completely ignores the lit.local.cfg files that I've created. The fstream tests are executed even if I build with -DLIBCXX_ENABLE_GLOBAL_FILESYSTEM_NAMESPACE=OFF. Conversely, the fopen.fail.cpp test is still executed even if the global filesystem namespace is available.

It looks like I'm the first person attempting to add a lit.local.cfg to libc++. Maybe we still need to flip a switch to have it enabled? I looked around, but I wasn't able to figure out what to do.

Ed

[jroelofs]

The missing piece is:

Index: test/libcxx/test/format.py
===================================================================
--- test/libcxx/test/format.py	(revision 231941)
+++ test/libcxx/test/format.py	(working copy)
@@ -60,6 +60,9 @@
         is_pass_test = name.endswith('.pass.cpp')
         is_fail_test = name.endswith('.fail.cpp')
 
+        if test.config.unsupported:
+            return lit.Test.UNSUPPORTED, "A lit.local.cfg marked this unsupported"
+
         res = lit.TestRunner.parseIntegratedTestScript(
             test, require_script=is_sh_test)
         # Check if a result for the test was returned. If so return that

With that, LGTM.

[ed]

In D8194#139766, @jroelofs wrote:

The missing piece is:

Index: test/libcxx/test/format.py
===================================================================
--- test/libcxx/test/format.py	(revision 231941)
+++ test/libcxx/test/format.py	(working copy)
@@ -60,6 +60,9 @@
         is_pass_test = name.endswith('.pass.cpp')
         is_fail_test = name.endswith('.fail.cpp')
 
+        if test.config.unsupported:
+            return lit.Test.UNSUPPORTED, "A lit.local.cfg marked this unsupported"
+
         res = lit.TestRunner.parseIntegratedTestScript(
             test, require_script=is_sh_test)
         # Check if a result for the test was returned. If so return that

With that, LGTM.

Awesome! That seems to make things work indeed. Thanks for your time (and patience)!

[EricWF]

I don't like this patch. You define _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE in three different places. If it is defined it the __config file there should be no need to define it in the CMakeLists.txt. This seems like a recipe for causing trouble.

Also these are some big changes we are introducing for CloudABI. Could you explain to me where this platform is used and who needs the changes?

libcxx/trunk/include/__config

731 ↗

(On Diff #21836)

Where do we get the definition for __CloudABI__ from?

libcxx/trunk/test/libcxx/test/format.py

63 ↗

(On Diff #21836)

I don't think we should be using this. Why not put the metadata into the tests?

libcxx/trunk/test/std/input.output/file.streams/c.files/no.global.filesystem.namespace/fopen.fail.cpp

8 ↗

(On Diff #21836)

Add // UNSUPPORTED: libcpp-has-no-global-filesystem-namespace

libcxx/trunk/test/std/input.output/file.streams/c.files/no.global.filesystem.namespace/lit.local.cfg

1 ↗

(On Diff #21836)

Remove this file. Add // UNSUPPORTED: libcpp-has-no-global-filesystem-namespace to the tests.

libcxx/trunk/test/std/input.output/file.streams/c.files/no.global.filesystem.namespace/rename.fail.cpp

9 ↗

(On Diff #21836)

Add // UNSUPPORTED: libcpp-has-no-global-filesystem-namespace

libcxx/trunk/test/std/input.output/file.streams/fstreams/lit.local.cfg

1 ↗

(On Diff #21836)

Remove this file. Add // UNSUPPORTED: libcpp-has-no-global-filesystem-namespace to the tests.

libcxx/trunk/test/std/localization/locales/locale.convenience/conversions/conversions.buffer/lit.local.cfg

1 ↗

(On Diff #21836)

Remove this file. Add // UNSUPPORTED: libcpp-has-no-global-filesystem-namespace to the tests.

[jroelofs]

libcxx/trunk/include/__config

731 ↗

(On Diff #21836)

Compiler provides it, just like __APPLE__ and all the others.

libcxx/trunk/test/libcxx/test/format.py

63 ↗

(On Diff #21836)

This is how the tests in llvm/test/CodeGen work. I think it cleans things up significantly. I view it as a mistake that I did the libcpp-has-no-threads UNSUPPORTEDs this way.... it's a bit painful to maintain them.

libcxx/trunk/test/std/input.output/file.streams/c.files/no.global.filesystem.namespace/fopen.fail.cpp

8 ↗

(On Diff #21836)

No, unless you meant // REQUIRES: libcpp-has-no-global-filesystem-namespace.

libcxx/trunk/test/std/input.output/file.streams/c.files/no.global.filesystem.namespace/rename.fail.cpp

9 ↗

(On Diff #21836)

No, unless you meant // REQUIRES: libcpp-has-no-global-filesystem-namespace.

[EricWF]

libcxx/trunk/include/__config

731 ↗

(On Diff #21836)

Where can I find more information about when Clang defines this macro?

libcxx/trunk/test/libcxx/test/format.py

63 ↗

(On Diff #21836)

My concern is that we accidentally don't run tests because they were mistakenly put in a directory with a lit.site.cfg that marks all the tests as unsupported.

I also think that lit.TestRunner.parseIntegratedTestScript may already handle this but I'll have to double check.

libcxx/trunk/test/std/input.output/file.streams/c.files/no.global.filesystem.namespace/fopen.fail.cpp

8 ↗

(On Diff #21836)

Yep. My mistake.

libcxx/trunk/test/std/input.output/file.streams/c.files/no.global.filesystem.namespace/rename.fail.cpp

9 ↗

(On Diff #21836)

I did. Thank you for the correction.

[jroelofs]

libcxx/trunk/test/libcxx/test/format.py

63 ↗

(On Diff #21836)

My concern is that we accidentally don't run tests because they were mistakenly put in a directory with a lit.site.cfg that marks all the tests as unsupported.

--show-unsupported takes care of that, no?

I also think that lit.TestRunner.parseIntegratedTestScript may already handle this but I'll have to double check.

Apparently not, or there's some flag that I missed that makes it happen... I debugged it a bit this morning before suggesting these lines to @ed

[jroelofs]

libcxx/trunk/include/__config

731 ↗

(On Diff #21836)

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150302/264362.html

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20150309/125077.html

[EricWF]

libcxx/trunk/include/__config

731 ↗

(On Diff #21836)

Thanks. Since _LIBCPP_HAS_NO_GLOBAL_FILESYSTEM_NAMESPACE is set in the __config file I don't really see why we need to manually define it in CMake and LIT unless we want to be able to test these configurations on platforms which are not __CloudABI__ (ex FreeBSD which was mentioned).

Can someone explain why we need this?

libcxx/trunk/test/libcxx/test/format.py

63 ↗

(On Diff #21836)

--show-unsupported should *help* but you still need to be watching it carefully and understand what tests are actually unsupported.

At least with requiring the metadata in the test I can check the commit that put that line in there. If a test is added to a directory that has a lit.site.cfg it's a little harder to figure out.

I don't really mind the extra maintenance cost because it make things more explicit and clear to readers of the test.

Anyway I don't strongly object to this change so I'll drop the subject.

[jroelofs]

libcxx/trunk/include/__config

731 ↗

(On Diff #21836)

Can someone explain why we need this?

Same reason we need the corresponding thing for the singlethreaded stuff...

http://reviews.llvm.org/D8293

[EricWF]

libcxx/trunk/include/__config

731 ↗

(On Diff #21836)

We need the single threaded stuff because that macro is not defined it the __config header but this one is.

[jroelofs]

libcxx/trunk/test/libcxx/test/format.py

63 ↗

(On Diff #21836)

What if they got marked in the lit.local.cfg as xfail instead of unsupported? I could see that being quite a bit less risky as the tests would still get run regardless of the feature.

include/__config

          _LIBCPP_HAS_NO_THREADS is defined.
 #endif
 
+// Systems that use capability-based security (FreeBSD with Capsicum,
+// Nuxi CloudABI) may only provide local filesystem access (using *at()).
+// Functions like open(), rename(), unlink() and stat() should not be
+// used, as they attempt to access the global filesystem namespace.
+#ifndef __CloudABI__
+#define _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
+#endif
+
 #endif  // _LIBCPP_CONFIG

include/cstdio

 using ::fpos_t;
 using ::size_t;
 
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
 using ::remove;
 using ::rename;
 using ::tmpfile;
 using ::tmpnam;
+#endif
 using ::fclose;
 using ::fflush;
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
 using ::fopen;
 using ::freopen;
+#endif
 using ::setbuf;
 using ::setvbuf;
 using ::fprintf;

include/fstream

 
     // 27.9.1.4 Members:
     bool is_open() const;
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
     basic_filebuf* open(const char* __s, ios_base::openmode __mode);
     basic_filebuf* open(const string& __s, ios_base::openmode __mode);
+#endif
     basic_filebuf* close();
 
 protected:

     return __file_ != 0;
 }
 
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
 template <class _CharT, class _Traits>
 basic_filebuf<_CharT, _Traits>*
 basic_filebuf<_CharT, _Traits>::open(const char* __s, ios_base::openmode __mode)

 {
     return open(__s.c_str(), __mode);
 }
+#endif
 
 template <class _CharT, class _Traits>
 basic_filebuf<_CharT, _Traits>*

     typedef typename traits_type::off_type off_type;
 
     basic_ifstream();
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
     explicit basic_ifstream(const char* __s, ios_base::openmode __mode = ios_base::in);
     explicit basic_ifstream(const string& __s, ios_base::openmode __mode = ios_base::in);
+#endif
 #ifndef _LIBCPP_HAS_NO_RVALUE_REFERENCES
     basic_ifstream(basic_ifstream&& __rhs);
 #endif

 
     basic_filebuf<char_type, traits_type>* rdbuf() const;
     bool is_open() const;
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
     void open(const char* __s, ios_base::openmode __mode = ios_base::in);
     void open(const string& __s, ios_base::openmode __mode = ios_base::in);
+#endif
     void close();
 
 private:

 {
 }
 
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
 template <class _CharT, class _Traits>
 inline _LIBCPP_INLINE_VISIBILITY
 basic_ifstream<_CharT, _Traits>::basic_ifstream(const char* __s, ios_base::openmode __mode)

     if (__sb_.open(__s, __mode | ios_base::in) == 0)
         this->setstate(ios_base::failbit);
 }
+#endif
 
 #ifndef _LIBCPP_HAS_NO_RVALUE_REFERENCES
 

     return __sb_.is_open();
 }
 
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
 template <class _CharT, class _Traits>
 void
 basic_ifstream<_CharT, _Traits>::open(const char* __s, ios_base::openmode __mode)

     else
         this->setstate(ios_base::failbit);
 }
+#endif
 
 template <class _CharT, class _Traits>
 inline _LIBCPP_INLINE_VISIBILITY

 
     basic_filebuf<char_type, traits_type>* rdbuf() const;
     bool is_open() const;
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
     void open(const char* __s, ios_base::openmode __mode = ios_base::out);
     void open(const string& __s, ios_base::openmode __mode = ios_base::out);
+#endif
     void close();
 
 private:

 {
 }
 
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
 template <class _CharT, class _Traits>
 inline _LIBCPP_INLINE_VISIBILITY
 basic_ofstream<_CharT, _Traits>::basic_ofstream(const char* __s, ios_base::openmode __mode)

     if (__sb_.open(__s, __mode | ios_base::out) == 0)
         this->setstate(ios_base::failbit);
 }
+#endif
 
 #ifndef _LIBCPP_HAS_NO_RVALUE_REFERENCES
 

     return __sb_.is_open();
 }
 
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
 template <class _CharT, class _Traits>
 void
 basic_ofstream<_CharT, _Traits>::open(const char* __s, ios_base::openmode __mode)

     else
         this->setstate(ios_base::failbit);
 }
+#endif
 
 template <class _CharT, class _Traits>
 inline _LIBCPP_INLINE_VISIBILITY

     typedef typename traits_type::off_type off_type;
 
     basic_fstream();
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
     explicit basic_fstream(const char* __s, ios_base::openmode __mode = ios_base::in | ios_base::out);
     explicit basic_fstream(const string& __s, ios_base::openmode __mode = ios_base::in | ios_base::out);
+#endif
 #ifndef _LIBCPP_HAS_NO_RVALUE_REFERENCES
     basic_fstream(basic_fstream&& __rhs);
 #endif

 
     basic_filebuf<char_type, traits_type>* rdbuf() const;
     bool is_open() const;
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
     void open(const char* __s, ios_base::openmode __mode = ios_base::in | ios_base::out);
     void open(const string& __s, ios_base::openmode __mode = ios_base::in | ios_base::out);
+#endif
     void close();
 
 private:

 {
 }
 
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
 template <class _CharT, class _Traits>
 inline _LIBCPP_INLINE_VISIBILITY
 basic_fstream<_CharT, _Traits>::basic_fstream(const char* __s, ios_base::openmode __mode)

     if (__sb_.open(__s, __mode) == 0)
         this->setstate(ios_base::failbit);
 }
+#endif
 
 #ifndef _LIBCPP_HAS_NO_RVALUE_REFERENCES
 

     return __sb_.is_open();
 }
 
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
 template <class _CharT, class _Traits>
 void
 basic_fstream<_CharT, _Traits>::open(const char* __s, ios_base::openmode __mode)

     else
         this->setstate(ios_base::failbit);
 }
+#endif
 
 template <class _CharT, class _Traits>
 inline _LIBCPP_INLINE_VISIBILITY

test/support/platform_support.h

 }
 #endif
 
+#ifdef _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
 inline
 std::string
 get_temp_file_name()

     return Name;
 #endif
 }
+#endif // _LIBCPP_HAS_GLOBAL_FILESYSTEM_NAMESPACE
 
 #endif // PLATFORM_SUPPORT_H