This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by ed on Mar 10 2015, 4:03 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

ed updated this revision to Diff 21560.Mar 10 2015, 4:03 AM
ed retitled this revision from to libc++: Add option to disable access to the global filesystem namespace.
ed updated this object.
ed edited the test plan for this revision. (Show Details)
ed added a reviewer: jroelofs.
ed set the repository for this revision to rL LLVM.
ed added a subscriber: Unknown Object (MLST).
jroelofs edited edge metadata.Mar 10 2015, 7:12 AM

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 added a comment.Mar 10 2015, 9:02 AM

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 updated this revision to Diff 21763.Mar 11 2015, 2:14 PM
ed edited edge metadata.
ed removed rL LLVM as the repository for this revision.

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.

ed updated this object.Mar 11 2015, 2:14 PM
ed added a reviewer: mclow.lists.

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 updated this revision to Diff 21830.Mar 12 2015, 7:31 AM

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

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 added a comment.Mar 12 2015, 8:42 AM

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)!

This revision was automatically updated to reflect the committed changes.
EricWF added a subscriber: EricWF.Mar 12 2015, 10:43 AM

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 added inline comments.Mar 12 2015, 10:57 AM
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 added inline comments.Mar 12 2015, 11:02 AM
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 added inline comments.Mar 12 2015, 11:07 AM
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

EricWF added inline comments.Mar 12 2015, 11:15 AM
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 added inline comments.Mar 12 2015, 11:24 AM
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 added inline comments.Mar 12 2015, 11:27 AM
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 added inline comments.Mar 12 2015, 11:38 AM
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.