Skip to content
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.

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

Add a homeDir field in LocalFileSystem for removefile to check #4543

Open
wants to merge 35 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
67146ef
add a field homeDir in LocalFileSystem for removefile to check
SterlingT3485 Nov 18, 2024
00c5853
add explicit to resolve clang
SterlingT3485 Nov 18, 2024
a7ccf4a
Run clang-format
SterlingT3485 Nov 18, 2024
80397d4
use better subdirectory check method
SterlingT3485 Nov 18, 2024
09d805e
try weakly_con
SterlingT3485 Nov 18, 2024
8a42558
try fix reust
SterlingT3485 Nov 18, 2024
e40d223
fix bug
SterlingT3485 Nov 19, 2024
681381f
handle windows
SterlingT3485 Nov 19, 2024
8a31a89
revert
SterlingT3485 Nov 19, 2024
d8f669d
Run clang-format
SterlingT3485 Nov 19, 2024
1c1bb32
change to not resolve symlink
SterlingT3485 Nov 19, 2024
eca1bcf
moved homedir, improve expandpath
SterlingT3485 Nov 19, 2024
e6e853e
add in memory check
SterlingT3485 Nov 19, 2024
6c089de
test msvc
SterlingT3485 Nov 19, 2024
ec6fd02
test msvc
SterlingT3485 Nov 19, 2024
f96a5ae
test msvc
SterlingT3485 Nov 19, 2024
b5cbf03
fix msvc
SterlingT3485 Nov 19, 2024
8d9f4cf
add test
SterlingT3485 Nov 20, 2024
cfa6d1d
add test
SterlingT3485 Nov 20, 2024
5b8c54b
add tets
SterlingT3485 Nov 20, 2024
9d96c51
add tets
SterlingT3485 Nov 20, 2024
66f3def
safer way to check subdir
SterlingT3485 Nov 20, 2024
8179c90
safer way to check subdir
SterlingT3485 Nov 20, 2024
9f6d3ad
solve race condition
SterlingT3485 Nov 20, 2024
3019481
fix windows
SterlingT3485 Nov 20, 2024
dab9e8c
fix windows
SterlingT3485 Nov 20, 2024
e488e21
fix windows
SterlingT3485 Nov 20, 2024
3572b8a
test windows
SterlingT3485 Nov 21, 2024
a54fa52
test windows
SterlingT3485 Nov 21, 2024
f6e7168
test windows
SterlingT3485 Nov 21, 2024
50b7e91
move expandPath to storageUtil
SterlingT3485 Nov 21, 2024
7955894
fix clang
SterlingT3485 Nov 21, 2024
f78dd3c
remove local file system default constructor
SterlingT3485 Nov 21, 2024
6ff8697
fix tools
SterlingT3485 Nov 21, 2024
1f1c245
Run clang-format
SterlingT3485 Nov 21, 2024
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
2 changes: 1 addition & 1 deletion src/binder/bind/bind_extension.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ static void bindLoadExtension(const ExtensionStatement* extensionStatement) {
if (ExtensionUtils::isOfficialExtension(extensionStatement->getPath())) {
return;
}
auto localFileSystem = common::LocalFileSystem();
auto localFileSystem = common::LocalFileSystem("");
if (!localFileSystem.fileOrPathExists(extensionStatement->getPath(),
nullptr /* clientContext */)) {
throw common::BinderException(
Expand Down
35 changes: 32 additions & 3 deletions src/common/file_system/local_file_system.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -220,10 +220,14 @@ void LocalFileSystem::createDir(const std::string& dir) const {
// LCOV_EXCL_STOP
}
auto directoryToCreate = dir;
if (directoryToCreate.ends_with('/')) {
if (directoryToCreate.ends_with('/')
#if defined(_WIN32)
|| directoryToCreate.ends_with('\\')
#endif
) {
// This is a known issue with std::filesystem::create_directories. (link:
// https://github.com/llvm/llvm-project/issues/60634). We have to manually remove the
// last '/' if the path ends with '/'.
// last '/' if the path ends with '/'. (Added the second one for windows)
directoryToCreate = directoryToCreate.substr(0, directoryToCreate.size() - 1);
}
std::error_code errCode;
Expand All @@ -247,9 +251,34 @@ void LocalFileSystem::createDir(const std::string& dir) const {
}
}

bool isSubdirectory(const std::filesystem::path& base, const std::filesystem::path& sub) {
try {
// Resolve paths to their canonical form
auto canonicalBase = std::filesystem::canonical(base);
auto canonicalSub = std::filesystem::canonical(sub);

std::string relative = std::filesystem::relative(canonicalSub, canonicalBase).string();
// Size check for a "." result.
// If the path starts with "..", it's not a subdirectory.
return !relative.empty() && !(relative.starts_with(".."));

} catch (const std::filesystem::filesystem_error& e) {
// Handle errors, e.g., if paths don't exist
std::cerr << "Filesystem error: " << e.what() << std::endl;
return false;
}

return false;
}

void LocalFileSystem::removeFileIfExists(const std::string& path) {
if (!fileOrPathExists(path))
if (!fileOrPathExists(path)) {
return;
}
if (!isSubdirectory(homeDir, path)) {
throw IOException(stringFormat("Error: Path {} is not within the allowed home directory {}",
path, homeDir));
}
std::error_code errCode;
bool success = false;
if (std::filesystem::is_directory(path)) {
Expand Down
6 changes: 5 additions & 1 deletion src/common/file_system/virtual_file_system.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ namespace kuzu {
namespace common {

VirtualFileSystem::VirtualFileSystem() {
defaultFS = std::make_unique<LocalFileSystem>();
defaultFS = std::make_unique<LocalFileSystem>("");
}

VirtualFileSystem::VirtualFileSystem(std::string homeDir) {
defaultFS = std::make_unique<LocalFileSystem>(homeDir);
}

VirtualFileSystem::~VirtualFileSystem() = default;
Expand Down
6 changes: 6 additions & 0 deletions src/include/common/file_system/file_system.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ class KUZU_API FileSystem {
friend struct FileInfo;

public:
FileSystem() = default;

explicit FileSystem(std::string homeDir) : homeDir(std::move(homeDir)) {}

virtual ~FileSystem() = default;

virtual std::unique_ptr<FileInfo> openFile(const std::string& path, int flags,
Expand Down Expand Up @@ -92,6 +96,8 @@ class KUZU_API FileSystem {
virtual void truncate(FileInfo& fileInfo, uint64_t size) const;

virtual uint64_t getFileSize(const FileInfo& fileInfo) const = 0;

std::string homeDir;
};

} // namespace common
Expand Down
2 changes: 2 additions & 0 deletions src/include/common/file_system/local_file_system.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ struct LocalFileInfo : public FileInfo {

class KUZU_API LocalFileSystem final : public FileSystem {
public:
explicit LocalFileSystem(std::string homeDir) : FileSystem(std::move(homeDir)) {}
SterlingT3485 marked this conversation as resolved.
Show resolved Hide resolved

std::unique_ptr<FileInfo> openFile(const std::string& path, int flags,
main::ClientContext* context = nullptr,
FileLockType lock_type = FileLockType::NO_LOCK) override;
Expand Down
1 change: 1 addition & 0 deletions src/include/common/file_system/virtual_file_system.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ class KUZU_API VirtualFileSystem final : public FileSystem {

public:
VirtualFileSystem();
explicit VirtualFileSystem(std::string homeDir);

~VirtualFileSystem() override;

Expand Down
20 changes: 20 additions & 0 deletions src/include/storage/storage_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@
#include "common/file_system/virtual_file_system.h"
#include "common/null_mask.h"
#include "common/types/types.h"
#include "main/client_context.h"
#include "main/db_config.h"
#include "main/settings.h"
#include "storage/db_file_id.h"

namespace kuzu {
Expand Down Expand Up @@ -135,6 +138,23 @@ class StorageUtils {
return vfs->joinPath(directory, common::StorageConstants::LOCK_FILE_NAME);
}

static std::string expandPath(main::ClientContext* context, const std::string& path) {
if (main::DBConfig::isDBPathInMemory(path)) {
return path;
}
auto fullPath = path;
// Handle '~' for home directory expansion
if (path.starts_with('~')) {
fullPath = context->getCurrentSetting(main::HomeDirectorySetting::name)
.getValue<std::string>() +
fullPath.substr(1);
}
// Normalize the path to resolve '.' and '..'
std::filesystem::path normalizedPath =
std::filesystem::absolute(fullPath).lexically_normal();
return normalizedPath.string();
}

// Note: This is a relatively slow function because of division and mod and making std::pair.
// It is not meant to be used in performance critical code path.
static std::pair<uint64_t, uint64_t> getQuotientRemainder(uint64_t i, uint64_t divisor) {
Expand Down
9 changes: 6 additions & 3 deletions src/main/database.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "main/database.h"

#include "main/client_context.h"
#include "main/database_manager.h"
#include "storage/buffer_manager/buffer_manager.h"

Expand Down Expand Up @@ -86,12 +87,14 @@ std::unique_ptr<storage::BufferManager> Database::initBufferManager(const Databa
}

void Database::initMembers(std::string_view dbPath, construct_bm_func_t initBmFunc) {
vfs = std::make_unique<VirtualFileSystem>();
// To expand a path with home directory(~), we have to pass in a dummy clientContext which
// handles the home directory expansion.
auto clientContext = ClientContext(this);
const auto dbPathStr = std::string(dbPath);
databasePath = vfs->expandPath(&clientContext, dbPathStr);
auto clientContext = ClientContext(this);
databasePath = StorageUtils::expandPath(&clientContext, dbPathStr);

SterlingT3485 marked this conversation as resolved.
Show resolved Hide resolved
vfs = std::make_unique<VirtualFileSystem>(databasePath);
SterlingT3485 marked this conversation as resolved.
Show resolved Hide resolved

initAndLockDBDir();
bufferManager = initBmFunc(*this);
memoryManager = std::make_unique<MemoryManager>(bufferManager.get(), vfs.get());
Expand Down
198 changes: 198 additions & 0 deletions test/c_api/database_test.cpp
Original file line number Diff line number Diff line change
@@ -1,4 +1,8 @@
#include <fstream>

#include "c_api/kuzu.h"
#include "common/exception/io.h"
#include "common/file_system/virtual_file_system.h"
#include "graph_test/api_graph_test.h"
#include "gtest/gtest.h"

Expand Down Expand Up @@ -100,3 +104,197 @@ TEST_F(CApiDatabaseTest, CreationHomeDir) {
std::filesystem::remove_all(homePath + "/ku_test.db");
}
#endif

TEST_F(CApiDatabaseTest, VirtualFileSystemDeleteFiles) {
std::string homeDir = "/tmp/dbHome/";
kuzu::common::VirtualFileSystem vfs(homeDir);
std::filesystem::create_directories("/tmp/test1");
std::filesystem::create_directories("/tmp/dbHome/test1");

// Attempt to delete files outside the home directory (should error)
try {
vfs.removeFileIfExists("/tmp/test1");
} catch (const kuzu::common::IOException& e) {
// Expected behavior
EXPECT_STREQ(e.what(), "IO exception: Error: Path /tmp/test1 is not within the allowed "
"home directory /tmp/dbHome/");
}

vfs.removeFileIfExists("/tmp/dbHome/test1");

ASSERT_TRUE(std::filesystem::exists("/tmp/test1"));
ASSERT_FALSE(std::filesystem::exists("/tmp/dbHome/test1"));

// Cleanup: Remove directories after the test
std::filesystem::remove_all("/tmp/test1");
std::filesystem::remove_all("/tmp/dbHome/test1");
}

#ifndef __WASM__ // home directory is not available in WASM
TEST_F(CApiDatabaseTest, VirtualFileSystemDeleteFilesWithHome) {
std::string homeDir = "~/tmp/dbHome/";
kuzu::common::VirtualFileSystem vfs(homeDir);
std::filesystem::create_directories("~/tmp/test1");
std::filesystem::create_directories("~/tmp/dbHome/test1");

// Attempt to delete files outside the home directory (should error)
try {
vfs.removeFileIfExists("~/tmp/test1");
} catch (const kuzu::common::IOException& e) {
// Expected behavior
EXPECT_STREQ(e.what(), "IO exception: Error: Path ~/tmp/test1 is not within the allowed "
"home directory ~/tmp/dbHome/");
}

// Attempt to delete files outside the home directory (should error)
try {
vfs.removeFileIfExists("~");
} catch (const kuzu::common::IOException& e) {
// Expected behavior
EXPECT_STREQ(e.what(),
"IO exception: Error: Path ~ is not within the allowed home directory ~/tmp/dbHome/");
}

vfs.removeFileIfExists("~/tmp/dbHome/test1");

ASSERT_TRUE(std::filesystem::exists("~/tmp/test1"));
ASSERT_FALSE(std::filesystem::exists("~/tmp/dbHome/test1"));

// Cleanup: Remove directories after the test
std::filesystem::remove_all("~/tmp/test1");
std::filesystem::remove_all("~/tmp/dbHome/test1");
}
#endif

TEST_F(CApiDatabaseTest, VirtualFileSystemDeleteFilesEdgeCases) {
std::string homeDir = "/tmp/dbHome/";
kuzu::common::VirtualFileSystem vfs(homeDir);
std::filesystem::create_directories("/tmp/dbHome/../test2");
std::filesystem::create_directories("/tmp");
std::filesystem::create_directories("/tmp/dbHome/test2");

// Attempt to delete files outside the home directory (should error)
try {
vfs.removeFileIfExists("/tmp/dbHome/../test2");
} catch (const kuzu::common::IOException& e) {
// Expected behavior
EXPECT_STREQ(e.what(), "IO exception: Error: Path /tmp/dbHome/../test2 is not within the "
"allowed home directory /tmp/dbHome/");
}

try {
vfs.removeFileIfExists("/tmp");
} catch (const kuzu::common::IOException& e) {
// Expected behavior
EXPECT_STREQ(e.what(),
"IO exception: Error: Path /tmp is not within the allowed home directory /tmp/dbHome/");
}

try {
vfs.removeFileIfExists("/tmp/");
} catch (const kuzu::common::IOException& e) {
// Expected behavior
EXPECT_STREQ(e.what(), "IO exception: Error: Path /tmp/ is not within the allowed home "
"directory /tmp/dbHome/");
}

try {
vfs.removeFileIfExists("/tmp//////////////////");
} catch (const kuzu::common::IOException& e) {
// Expected behavior
EXPECT_STREQ(e.what(), "IO exception: Error: Path /tmp////////////////// is not within the "
"allowed home directory /tmp/dbHome/");
}

try {
vfs.removeFileIfExists("/tmp/./.././");
} catch (const kuzu::common::IOException& e) {
// Expected behavior
EXPECT_STREQ(e.what(), "IO exception: Error: Path /tmp/./.././ is not within the allowed "
"home directory /tmp/dbHome/");
}

try {
vfs.removeFileIfExists("/");
} catch (const kuzu::common::IOException& e) {
// Expected behavior
EXPECT_STREQ(e.what(),
"IO exception: Error: Path / is not within the allowed home directory /tmp/dbHome/");
}

vfs.removeFileIfExists("/tmp/dbHome/test2");

ASSERT_TRUE(std::filesystem::exists("/tmp/test2"));
ASSERT_FALSE(std::filesystem::exists("/tmp/dbHome/test2"));

// Cleanup: Remove directories after the test
std::filesystem::remove_all("/tmp/test2");
std::filesystem::remove_all("/tmp/dbHome/test2");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you also add those test cases:

  1. Delete directories with edge cases.
  2. Test whether it works as expected on windows since windows paths can have a mixture of '\' and '/'.
    For example: homedir: "C:\Desktop\dir"
    Path to delete: "C:\Desktop/dir/test1"
  3. Test whether we error in wild card pattern paths. (e.g. path to delete: '/tmp/kuzu*.test')
  4. Test when we delete a path with homedirectory symbol (e.g. path to delete "~/test")

Copy link
Collaborator Author

@SterlingT3485 SterlingT3485 Nov 21, 2024

Choose a reason for hiding this comment

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

added more tests, we do not support wild card patter for safety.

}

#if defined(_WIN32)
TEST_F(CApiDatabaseTest, VirtualFileSystemDeleteFilesWindowsPaths) {
// Test Home Directory
std::string homeDir = "C:\\Desktop\\dir";
kuzu::common::VirtualFileSystem vfs(homeDir);

// Setup directories for testing
std::filesystem::create_directories("C:\\test1");
std::filesystem::create_directories("C:\\Desktop\\dir\\test1");

// Mixed separators: HomeDir uses '\' while path uses '/'
std::string mixedSeparatorPath = "C:\\Desktop/dir/test1";

// Attempt to delete files outside the home directory (should error)
try {
vfs.removeFileIfExists("C:\\test1");
FAIL() << "Expected exception for path outside home directory.";
} catch (const kuzu::common::IOException& e) {
EXPECT_STREQ(e.what(), "IO exception: Error: Path C:\\test1 is not within the allowed home "
"directory C:\\Desktop\\dir");
}

// Attempt to delete file inside the home directory with mixed separators (should succeed)
try {
vfs.removeFileIfExists(mixedSeparatorPath);
} catch (const kuzu::common::IOException& e) {
FAIL() << "Unexpected exception when deleting files: " << e.what();
}

ASSERT_FALSE(std::filesystem::exists("C:\\Desktop\\dir\\test1")); // Should be deleted

// Cleanup
std::filesystem::remove_all("C:\\test1");
std::filesystem::remove_all("C:\\Desktop\\dir\\test1");
}
#endif

TEST_F(CApiDatabaseTest, VirtualFileSystemDeleteFilesWildcardNoRemoval) {
// Test Home Directory
std::string homeDir = "/tmp/dbHome_wildcard/";
kuzu::common::VirtualFileSystem vfs(homeDir);

// Setup files and directories
std::filesystem::create_directories("/tmp/dbHome_wildcard/test1_wildcard");
std::filesystem::create_directories("/tmp/dbHome_wildcard/test2_wildcard");
std::filesystem::create_directories("/tmp/dbHome_wildcard/nested_wildcard");
std::ofstream("/tmp/dbHome_wildcard/nested_wildcard/file1.txt").close();
std::ofstream("/tmp/dbHome_wildcard/nested_wildcard/file2.test").close();

// Attempt to remove files with wildcard pattern
try {
vfs.removeFileIfExists("/tmp/dbHome_wildcard/test*");
} catch (const kuzu::common::IOException& e) {
// Verify the exception is thrown for unsupported wildcard
EXPECT_STREQ(e.what(), "Error: Wildcard patterns are not supported in paths.");
}

// Verify files and directories still exist
ASSERT_TRUE(std::filesystem::exists("/tmp/dbHome_wildcard/test1_wildcard"));
ASSERT_TRUE(std::filesystem::exists("/tmp/dbHome_wildcard/test2_wildcard"));
ASSERT_TRUE(std::filesystem::exists("/tmp/dbHome_wildcard/nested_wildcard/file1.txt"));
ASSERT_TRUE(std::filesystem::exists("/tmp/dbHome_wildcard/nested_wildcard/file2.test"));

// Cleanup
std::filesystem::remove_all("/tmp/dbHome_wildcard");
}
4 changes: 2 additions & 2 deletions test/test_runner/csv_converter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ namespace kuzu {
namespace testing {

void CSVConverter::copySchemaFile() {
LocalFileSystem localFileSystem;
LocalFileSystem localFileSystem("");
auto csvSchemaFile =
localFileSystem.joinPath(csvDatasetPath, std::string(TestHelper::SCHEMA_FILE_NAME));
auto outputSchemaFile =
Expand Down Expand Up @@ -173,7 +173,7 @@ void CSVConverter::convertCSVFiles() {
}

void CSVConverter::convertCSVDataset() {
LocalFileSystem localFileSystem;
LocalFileSystem localFileSystem("");
if (!localFileSystem.fileOrPathExists(outputDatasetPath)) {
localFileSystem.createDir(outputDatasetPath);
}
Expand Down
Loading