Skip to content
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
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
8 changes: 6 additions & 2 deletions envoy/filesystem/filesystem.h
Original file line number Diff line number Diff line change
Expand Up @@ -169,15 +169,19 @@ struct DirectoryEntry {
// target. For example, if name_ is a symlink to a directory, its file type will be Directory.
FileType type_;

// The file size in bytes for regular files. Should not be relied on for directories,
// symlinks, or FileType::Other.
uint64_t size_bytes_;
Comment thread
jmarantz marked this conversation as resolved.
Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any way we can make this less of a footgun? One thing that comes to mind is wrapping it in a call that ASSERTs that type_ is Regular? Or perhaps making it a absl::optional?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Asserting that type_ is Regular doesn't actually solve the problem, because on Windows with a symlink the type is regular and the size is unknown (or maybe it's the size of the linked path, I forget - either way, not the size of the file).

Using absl::optional is a good idea though, to make it a hard failure instead of a quiet failure if size is used when it shouldn't be.


bool operator==(const DirectoryEntry& rhs) const {
return name_ == rhs.name_ && type_ == rhs.type_;
return name_ == rhs.name_ && type_ == rhs.type_ && size_bytes_ == rhs.size_bytes_;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

sorry if I'm missing but did we test this new equivalence?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We did now.

}
};

class DirectoryIteratorImpl;
class DirectoryIterator {
public:
DirectoryIterator() : entry_({"", FileType::Other}) {}
DirectoryIterator() : entry_({"", FileType::Other, 0}) {}
virtual ~DirectoryIterator() = default;

const DirectoryEntry& operator*() const { return entry_; }
Expand Down
35 changes: 17 additions & 18 deletions source/common/filesystem/posix/directory_iterator_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,39 +42,38 @@ void DirectoryIteratorImpl::nextEntry() {
}

if (entry == nullptr) {
entry_ = {"", FileType::Other};
entry_ = {"", FileType::Other, 0};
} else {
const std::string current_path(entry->d_name);
const std::string full_path(directory_path_ + "/" + current_path);
entry_ = {current_path, fileType(full_path, os_sys_calls_)};
entry_ = makeEntry(entry->d_name);
Comment thread
jmarantz marked this conversation as resolved.
}
}

FileType DirectoryIteratorImpl::fileType(const std::string& full_path,
Api::OsSysCallsImpl& os_sys_calls) {
DirectoryEntry DirectoryIteratorImpl::makeEntry(absl::string_view filename) const {
const std::string full_path = absl::StrCat(directory_path_, "/", filename);
struct stat stat_buf;

const Api::SysCallIntResult result = os_sys_calls.stat(full_path.c_str(), &stat_buf);
const Api::SysCallIntResult result = os_sys_calls_.stat(full_path.c_str(), &stat_buf);
if (result.return_value_ != 0) {
if (errno == ENOENT) {
if (result.errno_ == ENOENT) {
// Special case. This directory entity is likely to be a symlink,
// but the reference is broken as the target could not be stat()'ed.
// If we confirm this with an lstat, treat this file entity as
// a regular file, which may be unlink()'ed.
// a regular file, which may be unlink()'ed, with size 0.
if (::lstat(full_path.c_str(), &stat_buf) == 0 && S_ISLNK(stat_buf.st_mode)) {
return FileType::Regular;
return DirectoryEntry{std::string{filename}, FileType::Regular, 0};
}
}
// TODO: throwing an exception here makes this dangerous to use in worker threads,
// and in general since it's not clear to the user of Directory that an exception
// may be thrown. Perhaps make this return StatusOr and handle failures gracefully.
throw EnvoyException(fmt::format("unable to stat file: '{}' ({})", full_path, errno));
}

if (S_ISDIR(stat_buf.st_mode)) {
return FileType::Directory;
} else if (S_ISDIR(stat_buf.st_mode)) {
return DirectoryEntry{std::string{filename}, FileType::Directory, 0};
} else if (S_ISREG(stat_buf.st_mode)) {
return FileType::Regular;
return DirectoryEntry{std::string{filename}, FileType::Regular,
static_cast<uint64_t>(stat_buf.st_size)};
} else {
return DirectoryEntry{std::string{filename}, FileType::Other, 0};
}

return FileType::Other;
}

} // namespace Filesystem
Expand Down
6 changes: 4 additions & 2 deletions source/common/filesystem/posix/directory_iterator_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,17 @@ class DirectoryIteratorImpl : public DirectoryIterator {
DirectoryIteratorImpl(const DirectoryIteratorImpl&) = delete;
DirectoryIteratorImpl(DirectoryIteratorImpl&&) = default;

static FileType fileType(const std::string& name, Api::OsSysCallsImpl& os_sys_calls);

private:
void nextEntry();
void openDirectory();

DirectoryEntry makeEntry(absl::string_view filename) const;

std::string directory_path_;
DIR* dir_{nullptr};
Api::OsSysCallsImpl& os_sys_calls_;

friend class DirectoryTest_MakeEntryThrowsOnStatFailure_Test;
};

} // namespace Filesystem
Expand Down
24 changes: 13 additions & 11 deletions source/common/filesystem/win32/directory_iterator_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ DirectoryIteratorImpl::DirectoryIteratorImpl(const std::string& directory_path)
fmt::format("unable to open directory {}: {}", directory_path, ::GetLastError()));
}

entry_ = {std::string(find_data.cFileName), fileType(find_data)};
entry_ = makeEntry(find_data);
}

DirectoryIteratorImpl::~DirectoryIteratorImpl() {
Expand All @@ -34,27 +34,29 @@ DirectoryIteratorImpl& DirectoryIteratorImpl::operator++() {
}

if (ret == 0) {
entry_ = {"", FileType::Other};
entry_ = {"", FileType::Other, 0};
} else {
entry_ = {std::string(find_data.cFileName), fileType(find_data)};
entry_ = makeEntry(find_data);
}

return *this;
}

FileType DirectoryIteratorImpl::fileType(const WIN32_FIND_DATA& find_data) const {
DirectoryEntry DirectoryIteratorImpl::makeEntry(const WIN32_FIND_DATA& find_data) {
ULARGE_INTEGER file_size;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should this all be in the final else?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call, now that it's unset everywhere else!

file_size.LowPart = find_data.nFileSizeLow;
file_size.HighPart = find_data.nFileSizeHigh;
uint64_t size = static_cast<uint64_t>(file_size.QuadPart);
if ((find_data.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT) &&
!(find_data.dwReserved0 & IO_REPARSE_TAG_SYMLINK)) {
// The file is reparse point and not a symlink, so it can't be
// a regular file or a directory
return FileType::Other;
}

if (find_data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
return FileType::Directory;
return {std::string(find_data.cFileName), FileType::Other, 0};
} else if (find_data.dwFileAttributes & FILE_ATTRIBUTE_DIRECTORY) {
return {std::string(find_data.cFileName), FileType::Directory, 0};
} else {
return {std::string(find_data.cFileName), FileType::Regular, size};
}

return FileType::Regular;
}

} // namespace Filesystem
Expand Down
2 changes: 1 addition & 1 deletion source/common/filesystem/win32/directory_iterator_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ class DirectoryIteratorImpl : public DirectoryIterator {
DirectoryIteratorImpl& operator=(DirectoryIteratorImpl&&) = default;

private:
FileType fileType(const WIN32_FIND_DATA& find_data) const;
static DirectoryEntry makeEntry(const WIN32_FIND_DATA& find_data);

HANDLE find_handle_;
};
Expand Down
Loading