apacheGH-38821: [C++] Strengthen handling of duplicate slashes in S3, GCS

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.

Author: pitrou

cpp/src/arrow/filesystem/filesystem_test.cc

@@ -152,20 +152,32 @@ TEST(PathUtil, GetAbstractPathParent) {
   AssertPairEqual(pair, {"abc", "def\\ghi"});
 }
 
-TEST(PathUtil, ValidateAbstractPathParts) {
-  ASSERT_OK(ValidateAbstractPathParts({}));
-  ASSERT_OK(ValidateAbstractPathParts({"abc"}));
+TEST(PathUtil, ValidateAbstractPathPartsString) {
+  ASSERT_OK(ValidateAbstractPathParts(""));
+  ASSERT_OK(ValidateAbstractPathParts("abc"));
+  ASSERT_OK(ValidateAbstractPathParts("abc/def"));
+  ASSERT_OK(ValidateAbstractPathParts("abc/def.ghi"));
+  ASSERT_OK(ValidateAbstractPathParts("abc/def\\ghi"));
+
+  // Extraneous separators
+  ASSERT_RAISES(Invalid, ValidateAbstractPathParts("//"));
+  ASSERT_RAISES(Invalid, ValidateAbstractPathParts("abc//def"));
+}
+
+TEST(PathUtil, ValidateAbstractPathPartsVector) {
+  ASSERT_OK(ValidateAbstractPathParts(std::vector<std::string>{}));
+  ASSERT_OK(ValidateAbstractPathParts(std::vector<std::string>{"abc"}));
   ASSERT_OK(ValidateAbstractPathParts({"abc", "def"}));
   ASSERT_OK(ValidateAbstractPathParts({"abc", "def.ghi"}));
   ASSERT_OK(ValidateAbstractPathParts({"abc", "def\\ghi"}));
 
   // Empty path component
-  ASSERT_RAISES(Invalid, ValidateAbstractPathParts({""}));
+  ASSERT_RAISES(Invalid, ValidateAbstractPathParts(std::vector<std::string>{""}));
   ASSERT_RAISES(Invalid, ValidateAbstractPathParts({"abc", "", "def"}));
 
   // Separator in component
-  ASSERT_RAISES(Invalid, ValidateAbstractPathParts({"/"}));
-  ASSERT_RAISES(Invalid, ValidateAbstractPathParts({"abc/def"}));
+  ASSERT_RAISES(Invalid, ValidateAbstractPathParts(std::vector<std::string>{"/"}));
+  ASSERT_RAISES(Invalid, ValidateAbstractPathParts(std::vector<std::string>{"abc/def"}));
 }
 
 TEST(PathUtil, ConcatAbstractPath) {

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::ValidateAbstractPathParts(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, ""};

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

cpp/src/arrow/filesystem/path_util.cc

@@ -137,6 +137,18 @@ Status ValidateAbstractPathParts(const std::vector<std::string>& parts) {
   return Status::OK();
 }
 
+Status ValidateAbstractPathParts(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()) {

cpp/src/arrow/filesystem/path_util.h

@@ -67,6 +67,10 @@ std::pair<std::string, std::string> GetAbstractPathParent(const std::string& s);
 ARROW_EXPORT
 Status ValidateAbstractPathParts(const std::vector<std::string>& parts);
 
+// Validate the components of an abstract path.
+ARROW_EXPORT
+Status ValidateAbstractPathParts(std::string_view path);
+
 // Append a non-empty stem to an abstract path.
 ARROW_EXPORT
 std::string ConcatAbstractPath(std::string_view base, std::string_view stem);

cpp/src/arrow/filesystem/s3fs.cc

@@ -154,9 +154,9 @@ using internal::S3Backend;
 using internal::ToAwsString;
 using internal::ToURLEncodedAwsString;
 
-static const char kSep = '/';
-constexpr char kAwsEndpointUrlEnvVar[] = "AWS_ENDPOINT_URL";
-constexpr char kAwsEndpointUrlS3EnvVar[] = "AWS_ENDPOINT_URL_S3";
+static constexpr const char kSep = '/';
+static constexpr const char kAwsEndpointUrlEnvVar[] = "AWS_ENDPOINT_URL";
+static constexpr const char kAwsEndpointUrlS3EnvVar[] = "AWS_ENDPOINT_URL_S3";
 
 // -----------------------------------------------------------------------
 // S3ProxyOptions implementation
@@ -481,12 +481,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::ValidateAbstractPathParts(path.full_path);
+    if (!st.ok()) {
+      return Status::Invalid(st.message(), " in path ", path.full_path);
     }
+    return Status::OK();
   }
 
   Aws::String ToAwsString() const {

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