Skip to content

ARROW-13092: [C++] Return an error in CreateDir if target is a file #10541

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

Closed
pitrou wants to merge 1 commit into from
Closed

ARROW-13092: [C++] Return an error in CreateDir if target is a file #10541

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
3 changes: 3 additions & 0 deletions cpp/src/arrow/filesystem/test_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,9 @@ void GenericFileSystemTest::TestCreateDir(FileSystem* fs) {
ASSERT_RAISES(IOError, fs->CreateDir("AB/def/EF/GH", true /* recursive */));
ASSERT_RAISES(IOError, fs->CreateDir("AB/def/EF", false /* recursive */));

// Cannot create a directory when there is already a file with the same name
ASSERT_RAISES(IOError, fs->CreateDir("AB/def"));

AssertAllDirs(fs, {"AB", "AB/CD", "AB/CD/EF", "AB/GH", "AB/GH/IJ", "XY"});
AssertAllFiles(fs, {"AB/def"});
}
Expand Down
18 changes: 16 additions & 2 deletions cpp/src/arrow/util/io_util.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,11 +472,18 @@ namespace {

Result<bool> DoCreateDir(const PlatformFilename& dir_path, bool create_parents) {
#ifdef _WIN32
if (CreateDirectoryW(dir_path.ToNative().c_str(), nullptr)) {
const auto s = dir_path.ToNative().c_str();
if (CreateDirectoryW(s, nullptr)) {
return true;
}
int errnum = GetLastError();
if (errnum == ERROR_ALREADY_EXISTS) {
const auto attrs = GetFileAttributesW(s);
if (attrs == INVALID_FILE_ATTRIBUTES || !(attrs & FILE_ATTRIBUTE_DIRECTORY)) {
// Note we propagate the original error, not the GetFileAttributesW() error
return IOErrorFromWinError(ERROR_ALREADY_EXISTS, "Cannot create directory '",
dir_path.ToString(), "': non-directory entry exists");
}
return false;
}
if (create_parents && errnum == ERROR_PATH_NOT_FOUND) {
Expand All @@ -489,10 +496,17 @@ Result<bool> DoCreateDir(const PlatformFilename& dir_path, bool create_parents)
return IOErrorFromWinError(GetLastError(), "Cannot create directory '",
dir_path.ToString(), "'");
#else
if (mkdir(dir_path.ToNative().c_str(), S_IRWXU | S_IRWXG | S_IRWXO) == 0) {
const auto s = dir_path.ToNative().c_str();
if (mkdir(s, S_IRWXU | S_IRWXG | S_IRWXO) == 0) {
return true;
}
if (errno == EEXIST) {
struct stat st;
if (stat(s, &st) || !S_ISDIR(st.st_mode)) {
// Note we propagate the original errno, not the stat() errno
return IOErrorFromErrno(EEXIST, "Cannot create directory '", dir_path.ToString(),
"': non-directory entry exists");
}
return false;
}
if (create_parents && errno == ENOENT) {
Expand Down
37 changes: 25 additions & 12 deletions cpp/src/arrow/util/io_util_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <pthread.h>
#endif

#include <gmock/gmock-matchers.h>
#include <gtest/gtest.h>

#include "arrow/testing/gtest_util.h"
Expand All @@ -53,6 +54,12 @@ void AssertNotExists(const PlatformFilename& path) {
ASSERT_FALSE(exists) << "Path '" << path.ToString() << "' exists";
}

void TouchFile(const PlatformFilename& path) {
int fd = -1;
ASSERT_OK_AND_ASSIGN(fd, FileOpenWritable(path));
ASSERT_OK(FileClose(fd));
}

TEST(ErrnoFromStatus, Basics) {
Status st;
st = Status::OK();
Expand Down Expand Up @@ -370,7 +377,7 @@ TEST(CreateDirDeleteDir, Basics) {
const std::string BASE =
temp_dir->path().Join("xxx-io-util-test-dir2").ValueOrDie().ToString();
bool created, deleted;
PlatformFilename parent, child;
PlatformFilename parent, child, child_file;

ASSERT_OK_AND_ASSIGN(parent, PlatformFilename::FromString(BASE));
ASSERT_EQ(parent.ToString(), BASE);
Expand All @@ -392,6 +399,11 @@ TEST(CreateDirDeleteDir, Basics) {
ASSERT_TRUE(created);
AssertExists(child);

ASSERT_OK_AND_ASSIGN(child_file, PlatformFilename::FromString(BASE + "/some-file"));
TouchFile(child_file);
EXPECT_RAISES_WITH_MESSAGE_THAT(
IOError, ::testing::HasSubstr("non-directory entry exists"), CreateDir(child_file));

ASSERT_OK_AND_ASSIGN(deleted, DeleteDirTree(parent));
ASSERT_TRUE(deleted);
AssertNotExists(parent);
Expand Down Expand Up @@ -436,9 +448,7 @@ TEST(DeleteDirContents, Basics) {
ASSERT_OK_AND_ASSIGN(child2, PlatformFilename::FromString(BASE + "/child-file"));
ASSERT_OK_AND_ASSIGN(created, CreateDir(child1));
ASSERT_TRUE(created);
int fd = -1;
ASSERT_OK_AND_ASSIGN(fd, FileOpenWritable(child2));
ASSERT_OK(FileClose(fd));
TouchFile(child2);
AssertExists(child1);
AssertExists(child2);

Expand Down Expand Up @@ -522,6 +532,14 @@ TEST(CreateDirTree, Basics) {
ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("EF"));
ASSERT_OK_AND_ASSIGN(created, CreateDirTree(fn));
ASSERT_TRUE(created);

ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("AB/file"));
Copy link
Member

@westonpace westonpace Jun 16, 2021

Choose a reason for hiding this comment

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

I checked and this code does the right thing with symlinks today (There is a S_ISLNK but you have to use lstat to get it) 👍 . I'm not sure if you want to add a test case or not.

Copy link
Member Author

@pitrou pitrou Jun 16, 2021

Choose a reason for hiding this comment

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

I'd rather not complicate the tests with that (especially as it would require some symlink creation code for Windows).

TouchFile(fn);
EXPECT_RAISES_WITH_MESSAGE_THAT(
IOError, ::testing::HasSubstr("non-directory entry exists"), CreateDirTree(fn));

ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("AB/file/sub"));
ASSERT_RAISES(IOError, CreateDirTree(fn));
}

TEST(ListDir, Basics) {
Expand All @@ -546,9 +564,7 @@ TEST(ListDir, Basics) {
ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("AB/EF/GH"));
ASSERT_OK(CreateDirTree(fn));
ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("AB/ghi.txt"));
int fd = -1;
ASSERT_OK_AND_ASSIGN(fd, FileOpenWritable(fn));
ASSERT_OK(FileClose(fd));
TouchFile(fn);

ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("AB"));
ASSERT_OK_AND_ASSIGN(entries, ListDir(fn));
Expand All @@ -568,15 +584,13 @@ TEST(ListDir, Basics) {
TEST(DeleteFile, Basics) {
std::unique_ptr<TemporaryDir> temp_dir;
PlatformFilename fn;
int fd;
bool deleted;

ASSERT_OK_AND_ASSIGN(temp_dir, TemporaryDir::Make("io-util-test-"));
ASSERT_OK_AND_ASSIGN(fn, temp_dir->path().Join("test-file"));

AssertNotExists(fn);
ASSERT_OK_AND_ASSIGN(fd, FileOpenWritable(fn));
ASSERT_OK(FileClose(fd));
TouchFile(fn);
AssertExists(fn);
ASSERT_OK_AND_ASSIGN(deleted, DeleteFile(fn));
ASSERT_TRUE(deleted);
Expand Down Expand Up @@ -638,8 +652,7 @@ TEST(FileUtils, LongPaths) {
AssertExists(long_path);
ASSERT_OK_AND_ASSIGN(long_filename,
PlatformFilename::FromString(fs.str() + "/file.txt"));
ASSERT_OK_AND_ASSIGN(fd, FileOpenWritable(long_filename));
ASSERT_OK(FileClose(fd));
TouchFile(long_filename);
AssertExists(long_filename);
fd = -1;
ASSERT_OK_AND_ASSIGN(fd, FileOpenReadable(long_filename));
Expand Down