Skip to content

bootstrap: allow /dev/fd/<fd> paths for config files #7279

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
htuch merged 1 commit into from
Jul 12, 2019
Merged

bootstrap: allow /dev/fd/<fd> paths for config files #7279

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions source/common/filesystem/posix/filesystem_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,17 @@ std::string InstanceImplPosix::fileReadToEnd(const std::string& path) {
}

bool InstanceImplPosix::illegalPath(const std::string& path) {
// Special case, allow /dev/fd/* access here so that config can be passed in a
// file descriptor from an execing bootstrap script. The reason we do this
// _before_ canonicalizing the path is that different unix flavors implement
// /dev/fd/* differently, for example on linux they are symlinks to /dev/pts/*
// which are symlinks to /proc/self/fds/. On BSD (and darwin) they are not
// symlinks at all. To avoid lots of platform, specifics, we whitelist
// /dev/fd/* _before_ resolving the canonical path.
if (absl::StartsWith(path, "/dev/fd/")) {
return false;
}
Copy link
Contributor Author

@banks banks Jun 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be the simplest approach here rather than trying to research all the different OS implementations of this path and white list every possible ending point.

I'm not sure if there are any specific downsides of bypassing realpath here?

Copy link
Member

@htuch htuch Jun 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay, been catching up on backlog after OOO. I think this is fine for now, but can you leave a TODO to switch to https://en.cppreference.com/w/cpp/filesystem/absolute when we have C++17? The main downside of what you have is that there is no relative path normalization, which we want independent of symlink following.
/wait

Copy link
Contributor Author

@banks banks Jun 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the TODO. I hope this is appropriate - I added it under your username as it seems to be more likely you will have context for this later than I will!


const Api::SysCallStringResult canonical_path = canonicalPath(path);
if (canonical_path.rc_.empty()) {
ENVOY_LOG_MISC(debug, "Unable to determine canonical path for {}: {}", path,
Expand Down
2 changes: 2 additions & 0 deletions test/common/filesystem/filesystem_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ TEST_F(FileSystemImplTest, IllegalPath) {
#else
EXPECT_TRUE(file_system_.illegalPath("/dev"));
EXPECT_TRUE(file_system_.illegalPath("/dev/"));
// Exception to allow opening from file descriptors. See #7258.
EXPECT_FALSE(file_system_.illegalPath("/dev/fd/0"));
EXPECT_TRUE(file_system_.illegalPath("/proc"));
EXPECT_TRUE(file_system_.illegalPath("/proc/"));
EXPECT_TRUE(file_system_.illegalPath("/sys"));
Expand Down
3 changes: 3 additions & 0 deletions tools/spelling_dictionary.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ cstring
ctor
ctrl
customizations
darwin
dbg
de
dechunking
Expand Down Expand Up @@ -631,6 +632,7 @@ pthreads
pton
ptr
ptrs
pts
pwd
py
qps
Expand Down Expand Up @@ -718,6 +720,7 @@ structs
subexpr
subdirs
symlink
symlinks
symlinked
sync
sys
Expand Down