You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@arrow.apache.org by ap...@apache.org on 2024/03/08 14:11:35 UTC
(arrow) branch main updated: GH-38821: [C++] Strengthen handling of duplicate slashes in S3, GCS (#40371)
This is an automated email from the ASF dual-hosted git repository.
apitrou pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/arrow.git
The following commit(s) were added to refs/heads/main by this push:
new 0148db4f97 GH-38821: [C++] Strengthen handling of duplicate slashes in S3, GCS (#40371)
0148db4f97 is described below
commit 0148db4f97bfcf80237b265ee0d9a4b61194ffde
Author: Antoine Pitrou <an...@python.org>
AuthorDate: Fri Mar 8 15:11:30 2024 +0100
GH-38821: [C++] Strengthen handling of duplicate slashes in S3, GCS (#40371)
### Rationale for this change
Giving a path such as "bucket//key" to some S3 and GCS filesystem APIs could silently succeed or crash.
Make sure those paths instead return an error, as other instances of duplicate slashes already do.
### Are these changes tested?
Yes.
### Are there any user-facing changes?
Yes. Some paths that were accepted by some filesystems can now return an error.
* GitHub Issue: #38821
Authored-by: Antoine Pitrou <an...@python.org>
Signed-off-by: Antoine Pitrou <an...@python.org>
---
cpp/src/arrow/filesystem/filesystem_test.cc | 12 ++++++++++++
cpp/src/arrow/filesystem/gcsfs.cc | 9 +++++++++
cpp/src/arrow/filesystem/gcsfs_test.cc | 14 ++++++++++++++
cpp/src/arrow/filesystem/path_util.cc | 12 ++++++++++++
cpp/src/arrow/filesystem/path_util.h | 4 ++++
cpp/src/arrow/filesystem/s3fs.cc | 17 ++++++++---------
cpp/src/arrow/filesystem/s3fs_test.cc | 8 ++++++++
7 files changed, 67 insertions(+), 9 deletions(-)
diff --git a/cpp/src/arrow/filesystem/filesystem_test.cc b/cpp/src/arrow/filesystem/filesystem_test.cc
index c76c3d27e8..8477647b2c 100644
--- a/cpp/src/arrow/filesystem/filesystem_test.cc
+++ b/cpp/src/arrow/filesystem/filesystem_test.cc
@@ -152,6 +152,18 @@ TEST(PathUtil, GetAbstractPathParent) {
AssertPairEqual(pair, {"abc", "def\\ghi"});
}
+TEST(PathUtil, ValidateAbstractPath) {
+ ASSERT_OK(ValidateAbstractPath(""));
+ ASSERT_OK(ValidateAbstractPath("abc"));
+ ASSERT_OK(ValidateAbstractPath("abc/def"));
+ ASSERT_OK(ValidateAbstractPath("abc/def.ghi"));
+ ASSERT_OK(ValidateAbstractPath("abc/def\\ghi"));
+
+ // Extraneous separators
+ ASSERT_RAISES(Invalid, ValidateAbstractPath("//"));
+ ASSERT_RAISES(Invalid, ValidateAbstractPath("abc//def"));
+}
+
TEST(PathUtil, ValidateAbstractPathParts) {
ASSERT_OK(ValidateAbstractPathParts({}));
ASSERT_OK(ValidateAbstractPathParts({"abc"}));
diff --git a/cpp/src/arrow/filesystem/gcsfs.cc b/cpp/src/arrow/filesystem/gcsfs.cc
index d41cb49022..1f091e980b 100644
--- a/cpp/src/arrow/filesystem/gcsfs.cc
+++ b/cpp/src/arrow/filesystem/gcsfs.cc
@@ -73,9 +73,18 @@ struct GcsPath {
path.full_path = s;
path.bucket = s.substr(0, first_sep);
path.object = s.substr(first_sep + 1);
+ RETURN_NOT_OK(Validate(path));
return path;
}
+ static Status Validate(const GcsPath& path) {
+ auto st = internal::ValidateAbstractPath(path.full_path);
+ if (!st.ok()) {
+ return Status::Invalid(st.message(), " in path ", path.full_path);
+ }
+ return Status::OK();
+ }
+
GcsPath parent() const {
auto object_parent = internal::GetAbstractPathParent(object).first;
if (object_parent.empty()) return GcsPath{bucket, bucket, ""};
diff --git a/cpp/src/arrow/filesystem/gcsfs_test.cc b/cpp/src/arrow/filesystem/gcsfs_test.cc
index 7a8afc0ee4..a6022a8d21 100644
--- a/cpp/src/arrow/filesystem/gcsfs_test.cc
+++ b/cpp/src/arrow/filesystem/gcsfs_test.cc
@@ -861,6 +861,14 @@ TEST_F(GcsIntegrationTest, CreateDirUri) {
ASSERT_RAISES(Invalid, fs->CreateDir("gs://" + RandomBucketName(), true));
}
+TEST_F(GcsIntegrationTest, CreateDirExtraneousSlashes) {
+ auto fs = GcsFileSystem::Make(TestGcsOptions());
+ ASSERT_RAISES(Invalid,
+ fs->CreateDir(RandomBucketName() + "//somedir", /*recursive=*/true));
+ ASSERT_RAISES(Invalid, fs->CreateDir(RandomBucketName() + "/somedir//newdir",
+ /*recursive=*/true));
+}
+
TEST_F(GcsIntegrationTest, DeleteBucketDirSuccess) {
auto fs = GcsFileSystem::Make(TestGcsOptions());
ASSERT_OK(fs->CreateDir("pyarrow-filesystem/", /*recursive=*/true));
@@ -888,6 +896,12 @@ TEST_F(GcsIntegrationTest, DeleteDirUri) {
ASSERT_RAISES(Invalid, fs->DeleteDir("gs://" + PreexistingBucketPath()));
}
+TEST_F(GcsIntegrationTest, DeleteDirExtraneousSlashes) {
+ auto fs = GcsFileSystem::Make(TestGcsOptions());
+ ASSERT_RAISES(Invalid, fs->DeleteDir(PreexistingBucketPath() + "/somedir"));
+ ASSERT_RAISES(Invalid, fs->DeleteDir(PreexistingBucketPath() + "somedir//newdir"));
+}
+
TEST_F(GcsIntegrationTest, DeleteDirContentsSuccess) {
auto fs = GcsFileSystem::Make(TestGcsOptions());
ASSERT_OK_AND_ASSIGN(auto hierarchy, CreateHierarchy(fs));
diff --git a/cpp/src/arrow/filesystem/path_util.cc b/cpp/src/arrow/filesystem/path_util.cc
index 9c895ae76c..f38537e59f 100644
--- a/cpp/src/arrow/filesystem/path_util.cc
+++ b/cpp/src/arrow/filesystem/path_util.cc
@@ -137,6 +137,18 @@ Status ValidateAbstractPathParts(const std::vector<std::string>& parts) {
return Status::OK();
}
+Status ValidateAbstractPath(std::string_view path) {
+ auto pos = path.find_first_of(kSep);
+ while (pos != path.npos) {
+ ++pos;
+ if (path.length() > pos && path[pos] == kSep) {
+ return Status::Invalid("Empty path component");
+ }
+ pos = path.find_first_of(kSep, pos);
+ }
+ return Status::OK();
+}
+
std::string ConcatAbstractPath(std::string_view base, std::string_view stem) {
DCHECK(!stem.empty());
if (base.empty()) {
diff --git a/cpp/src/arrow/filesystem/path_util.h b/cpp/src/arrow/filesystem/path_util.h
index 1da7afd3f9..d49d9d2efa 100644
--- a/cpp/src/arrow/filesystem/path_util.h
+++ b/cpp/src/arrow/filesystem/path_util.h
@@ -63,6 +63,10 @@ ARROW_EXPORT int GetAbstractPathDepth(std::string_view path);
ARROW_EXPORT
std::pair<std::string, std::string> GetAbstractPathParent(const std::string& s);
+// Validate an abstract path.
+ARROW_EXPORT
+Status ValidateAbstractPath(std::string_view path);
+
// Validate the components of an abstract path.
ARROW_EXPORT
Status ValidateAbstractPathParts(const std::vector<std::string>& parts);
diff --git a/cpp/src/arrow/filesystem/s3fs.cc b/cpp/src/arrow/filesystem/s3fs.cc
index b14f96e4dd..77fd951ba2 100644
--- a/cpp/src/arrow/filesystem/s3fs.cc
+++ b/cpp/src/arrow/filesystem/s3fs.cc
@@ -154,10 +154,10 @@ using internal::S3Backend;
using internal::ToAwsString;
using internal::ToURLEncodedAwsString;
-constexpr const char kSep = '/';
-constexpr const char kAwsEndpointUrlEnvVar[] = "AWS_ENDPOINT_URL";
-constexpr const char kAwsEndpointUrlS3EnvVar[] = "AWS_ENDPOINT_URL_S3";
-constexpr const char kAwsDirectoryContentType[] = "application/x-directory";
+static constexpr const char kSep = '/';
+static constexpr const char kAwsEndpointUrlEnvVar[] = "AWS_ENDPOINT_URL";
+static constexpr const char kAwsEndpointUrlS3EnvVar[] = "AWS_ENDPOINT_URL_S3";
+static constexpr const char kAwsDirectoryContentType[] = "application/x-directory";
// -----------------------------------------------------------------------
// S3ProxyOptions implementation
@@ -482,12 +482,11 @@ struct S3Path {
}
static Status Validate(const S3Path& path) {
- auto result = internal::ValidateAbstractPathParts(path.key_parts);
- if (!result.ok()) {
- return Status::Invalid(result.message(), " in path ", path.full_path);
- } else {
- return result;
+ auto st = internal::ValidateAbstractPath(path.full_path);
+ if (!st.ok()) {
+ return Status::Invalid(st.message(), " in path ", path.full_path);
}
+ return Status::OK();
}
Aws::String ToAwsString() const {
diff --git a/cpp/src/arrow/filesystem/s3fs_test.cc b/cpp/src/arrow/filesystem/s3fs_test.cc
index ad7aaa1bd4..2ed97c5e97 100644
--- a/cpp/src/arrow/filesystem/s3fs_test.cc
+++ b/cpp/src/arrow/filesystem/s3fs_test.cc
@@ -935,6 +935,10 @@ TEST_F(TestS3FS, CreateDir) {
// URI
ASSERT_RAISES(Invalid, fs_->CreateDir("s3:bucket/newdir2"));
+
+ // Extraneous slashes
+ ASSERT_RAISES(Invalid, fs_->CreateDir("bucket//somedir"));
+ ASSERT_RAISES(Invalid, fs_->CreateDir("bucket/somedir//newdir"));
}
TEST_F(TestS3FS, DeleteFile) {
@@ -994,6 +998,10 @@ TEST_F(TestS3FS, DeleteDir) {
// URI
ASSERT_RAISES(Invalid, fs_->DeleteDir("s3:empty-bucket"));
+
+ // Extraneous slashes
+ ASSERT_RAISES(Invalid, fs_->DeleteDir("bucket//newdir"));
+ ASSERT_RAISES(Invalid, fs_->DeleteDir("bucket/newdir//newsub"));
}
TEST_F(TestS3FS, DeleteDirContents) {