From 3bdc76025b9b1a8416f00b5f1b57a51c6c9c604d Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 28 Jul 2016 18:30:46 -0700 Subject: [PATCH 1/2] adb: extract Windows bits out of directory_exists test. Bug: http://b/30481559 Bug: https://code.google.com/p/android/issues/detail?id=214633 Change-Id: I8f20b3cd5aef6a77c2b4f194b914b4295397d73f --- adb/adb_utils_test.cpp | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/adb/adb_utils_test.cpp b/adb/adb_utils_test.cpp index aabc5d733..91826a9ec 100644 --- a/adb/adb_utils_test.cpp +++ b/adb/adb_utils_test.cpp @@ -52,18 +52,6 @@ TEST(adb_utils, directory_exists) { ASSERT_TRUE(directory_exists(profiles_dir)); - // On modern (English?) Windows, this is a directory symbolic link to - // C:\ProgramData. Symbolic links are rare on Windows and the user requires - // a special permission (by default granted to Administrative users) to - // create symbolic links. - ASSERT_FALSE(directory_exists(subdir(profiles_dir, "All Users"))); - - // On modern (English?) Windows, this is a directory junction to - // C:\Users\Default. Junctions are used throughout user profile directories - // for backwards compatibility and they don't require any special permissions - // to create. - ASSERT_FALSE(directory_exists(subdir(profiles_dir, "Default User"))); - ASSERT_FALSE(directory_exists(subdir(profiles_dir, "does-not-exist"))); #else ASSERT_TRUE(directory_exists("/proc")); @@ -72,6 +60,28 @@ TEST(adb_utils, directory_exists) { #endif } +#if defined(_WIN32) +TEST(adb_utils, directory_exists_win32_symlink_junction) { + char profiles_dir[MAX_PATH]; + DWORD cch = arraysize(profiles_dir); + + // On typical Windows 7, returns C:\Users + ASSERT_TRUE(GetProfilesDirectoryA(profiles_dir, &cch)); + + // On modern (English?) Windows, this is a directory symbolic link to + // C:\ProgramData. Symbolic links are rare on Windows and the user requires + // a special permission (by default granted to Administrative users) to + // create symbolic links. + EXPECT_FALSE(directory_exists(subdir(profiles_dir, "All Users"))); + + // On modern (English?) Windows, this is a directory junction to + // C:\Users\Default. Junctions are used throughout user profile directories + // for backwards compatibility and they don't require any special permissions + // to create. + EXPECT_FALSE(directory_exists(subdir(profiles_dir, "Default User"))); +} +#endif + TEST(adb_utils, escape_arg) { ASSERT_EQ(R"('')", escape_arg("")); From f551ea0f6309465eeab70404076bd881320f4883 Mon Sep 17 00:00:00 2001 From: Josh Gao Date: Thu, 28 Jul 2016 18:09:48 -0700 Subject: [PATCH 2/2] adb: fix stat on Windows. stat on Windows fails with ENOENT when passed a path with a trailing slash or backslash, regardless of whether the target is actually a directory. Emulate the correct POSIX behavior by stripping trailing path separators and then checking if the target is a directory if successful. Bug: http://b/30481559 Bug: https://code.google.com/p/android/issues/detail?id=214633 Change-Id: I1d398d19a9bce1ecb3fdc4aabc31aa98c82c3f93 Test: Relevant adb_tests pass on Linux and Windows 10. --- adb/Android.mk | 2 ++ adb/adb_utils_test.cpp | 14 +++++--- adb/sysdeps.h | 25 ++------------- adb/sysdeps/stat.h | 46 ++++++++++++++++++++++++++ adb/sysdeps/stat_test.cpp | 65 +++++++++++++++++++++++++++++++++++++ adb/sysdeps/win32/stat.cpp | 66 ++++++++++++++++++++++++++++++++++++++ adb/sysdeps_win32.cpp | 24 -------------- 7 files changed, 191 insertions(+), 51 deletions(-) create mode 100644 adb/sysdeps/stat.h create mode 100644 adb/sysdeps/stat_test.cpp create mode 100644 adb/sysdeps/win32/stat.cpp diff --git a/adb/Android.mk b/adb/Android.mk index 0babf1d4a..7c029be41 100644 --- a/adb/Android.mk +++ b/adb/Android.mk @@ -63,6 +63,7 @@ LIBADB_TEST_SRCS := \ fdevent_test.cpp \ socket_test.cpp \ sysdeps_test.cpp \ + sysdeps/stat_test.cpp \ transport_test.cpp \ LIBADB_CFLAGS := \ @@ -90,6 +91,7 @@ LIBADB_linux_SRC_FILES := \ LIBADB_windows_SRC_FILES := \ sysdeps_win32.cpp \ + sysdeps/win32/stat.cpp \ usb_windows.cpp \ LIBADB_TEST_windows_SRCS := \ diff --git a/adb/adb_utils_test.cpp b/adb/adb_utils_test.cpp index 91826a9ec..14863cd5e 100644 --- a/adb/adb_utils_test.cpp +++ b/adb/adb_utils_test.cpp @@ -123,14 +123,20 @@ TEST(adb_utils, adb_dirname) { void test_mkdirs(const std::string basepath) { // Test creating a directory hierarchy. - EXPECT_TRUE(mkdirs(basepath)); + ASSERT_TRUE(mkdirs(basepath)); // Test finding an existing directory hierarchy. - EXPECT_TRUE(mkdirs(basepath)); + ASSERT_TRUE(mkdirs(basepath)); + // Test mkdirs on an existing hierarchy with a trailing slash. + ASSERT_TRUE(mkdirs(basepath + '/')); +#if defined(_WIN32) + ASSERT_TRUE(mkdirs(basepath + '\\')); +#endif + const std::string filepath = basepath + "/file"; // Verify that the hierarchy was created by trying to create a file in it. - EXPECT_NE(-1, adb_creat(filepath.c_str(), 0600)); + ASSERT_NE(-1, adb_creat(filepath.c_str(), 0600)); // If a file exists where we want a directory, the operation should fail. - EXPECT_FALSE(mkdirs(filepath)); + ASSERT_FALSE(mkdirs(filepath)); } TEST(adb_utils, mkdirs) { diff --git a/adb/sysdeps.h b/adb/sysdeps.h index 212c1c365..04df16f80 100644 --- a/adb/sysdeps.h +++ b/adb/sysdeps.h @@ -34,6 +34,8 @@ #include #include +#include "sysdeps/stat.h" + /* * TEMP_FAILURE_RETRY is defined by some, but not all, versions of * . (Alas, it is not as standard as we'd hoped!) So, if it's @@ -199,8 +201,6 @@ static __inline__ void close_on_exec(int fd) /* nothing really */ } -#define lstat stat /* no symlinks on Win32 */ - #define S_ISLNK(m) 0 /* no symlinks on Win32 */ extern int adb_unlink(const char* path); @@ -307,27 +307,6 @@ static __inline__ int adb_is_absolute_host_path(const char* path) { return isalpha(path[0]) && path[1] == ':' && path[2] == '\\'; } -// We later define a macro mapping 'stat' to 'adb_stat'. This causes: -// struct stat s; -// stat(filename, &s); -// To turn into the following: -// struct adb_stat s; -// adb_stat(filename, &s); -// To get this to work, we need to make 'struct adb_stat' the same as -// 'struct stat'. Note that this definition of 'struct adb_stat' uses the -// *current* macro definition of stat, so it may actually be inheriting from -// struct _stat32i64 (or some other remapping). -struct adb_stat : public stat {}; - -static_assert(sizeof(struct adb_stat) == sizeof(struct stat), - "structures should be the same"); - -extern int adb_stat(const char* f, struct adb_stat* s); - -// stat is already a macro, undefine it so we can redefine it. -#undef stat -#define stat adb_stat - // UTF-8 versions of POSIX APIs. extern DIR* adb_opendir(const char* dirname); extern struct dirent* adb_readdir(DIR* dir); diff --git a/adb/sysdeps/stat.h b/adb/sysdeps/stat.h new file mode 100644 index 000000000..595359529 --- /dev/null +++ b/adb/sysdeps/stat.h @@ -0,0 +1,46 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include +#include + +#if defined(_WIN32) +// stat is broken on Win32: stat on a path with a trailing slash or backslash will always fail with +// ENOENT. +int adb_stat(const char* path, struct adb_stat* buf); + +// We later define a macro mapping 'stat' to 'adb_stat'. This causes: +// struct stat s; +// stat(filename, &s); +// To turn into the following: +// struct adb_stat s; +// adb_stat(filename, &s); +// To get this to work, we need to make 'struct adb_stat' the same as +// 'struct stat'. Note that this definition of 'struct adb_stat' uses the +// *current* macro definition of stat, so it may actually be inheriting from +// struct _stat32i64 (or some other remapping). +struct adb_stat : public stat {}; + +#undef stat +#define stat adb_stat + +// Windows doesn't have lstat. +#define lstat adb_stat + +#endif diff --git a/adb/sysdeps/stat_test.cpp b/adb/sysdeps/stat_test.cpp new file mode 100644 index 000000000..2c2e0eeeb --- /dev/null +++ b/adb/sysdeps/stat_test.cpp @@ -0,0 +1,65 @@ +/* + * Copyright (C) 2015 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include + +#include +#include + +#include "adb_utils.h" +#include "sysdeps.h" + +TEST(sysdeps, stat) { + TemporaryDir td; + TemporaryFile tf; + + struct stat st; + ASSERT_EQ(0, stat(td.path, &st)); + ASSERT_FALSE(S_ISREG(st.st_mode)); + ASSERT_TRUE(S_ISDIR(st.st_mode)); + + ASSERT_EQ(0, stat((std::string(td.path) + '/').c_str(), &st)); + ASSERT_TRUE(S_ISDIR(st.st_mode)); + +#if defined(_WIN32) + ASSERT_EQ(0, stat((std::string(td.path) + '\\').c_str(), &st)); + ASSERT_TRUE(S_ISDIR(st.st_mode)); +#endif + + std::string nonexistent_path = std::string(td.path) + "/nonexistent"; + ASSERT_EQ(-1, stat(nonexistent_path.c_str(), &st)); + ASSERT_EQ(ENOENT, errno); + + ASSERT_EQ(-1, stat((nonexistent_path + "/").c_str(), &st)); + ASSERT_EQ(ENOENT, errno); + +#if defined(_WIN32) + ASSERT_EQ(-1, stat((nonexistent_path + "\\").c_str(), &st)); + ASSERT_EQ(ENOENT, errno); +#endif + + ASSERT_EQ(0, stat(tf.path, &st)); + ASSERT_TRUE(S_ISREG(st.st_mode)); + ASSERT_FALSE(S_ISDIR(st.st_mode)); + + ASSERT_EQ(-1, stat((std::string(tf.path) + '/').c_str(), &st)); + ASSERT_EQ(ENOTDIR, errno); + +#if defined(_WIN32) + ASSERT_EQ(-1, stat((std::string(tf.path) + '\\').c_str(), &st)); + ASSERT_EQ(ENOTDIR, errno); +#endif +} diff --git a/adb/sysdeps/win32/stat.cpp b/adb/sysdeps/win32/stat.cpp new file mode 100644 index 000000000..844c1ce9a --- /dev/null +++ b/adb/sysdeps/win32/stat.cpp @@ -0,0 +1,66 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "sysdeps/stat.h" + +#include +#include +#include +#include + +#include + +#include + +// Version of stat() that takes a UTF-8 path. +int adb_stat(const char* path, struct adb_stat* s) { +// This definition of wstat seems to be missing from . +#if defined(_FILE_OFFSET_BITS) && (_FILE_OFFSET_BITS == 64) +#ifdef _USE_32BIT_TIME_T +#define wstat _wstat32i64 +#else +#define wstat _wstat64 +#endif +#else +// has a function prototype for wstat() that should be available. +#endif + + std::wstring path_wide; + if (!android::base::UTF8ToWide(path, &path_wide)) { + errno = ENOENT; + return -1; + } + + // If the path has a trailing slash, stat will fail with ENOENT regardless of whether the path + // is a directory or not. + bool expected_directory = false; + while (*path_wide.rbegin() == u'/' || *path_wide.rbegin() == u'\\') { + path_wide.pop_back(); + expected_directory = true; + } + + struct adb_stat st; + int result = wstat(path_wide.c_str(), &st); + if (result == 0 && expected_directory) { + if (!S_ISDIR(st.st_mode)) { + errno = ENOTDIR; + return -1; + } + } + + memcpy(s, &st, sizeof(st)); + return result; +} diff --git a/adb/sysdeps_win32.cpp b/adb/sysdeps_win32.cpp index f94d6fcbc..ee2740690 100644 --- a/adb/sysdeps_win32.cpp +++ b/adb/sysdeps_win32.cpp @@ -2252,30 +2252,6 @@ int unix_open(const char* path, int options, ...) { } } -// Version of stat() that takes a UTF-8 path. -int adb_stat(const char* path, struct adb_stat* s) { -#pragma push_macro("wstat") -// This definition of wstat seems to be missing from . -#if defined(_FILE_OFFSET_BITS) && (_FILE_OFFSET_BITS == 64) -#ifdef _USE_32BIT_TIME_T -#define wstat _wstat32i64 -#else -#define wstat _wstat64 -#endif -#else -// has a function prototype for wstat() that should be available. -#endif - - std::wstring path_wide; - if (!android::base::UTF8ToWide(path, &path_wide)) { - return -1; - } - - return wstat(path_wide.c_str(), s); - -#pragma pop_macro("wstat") -} - // Version of opendir() that takes a UTF-8 path. DIR* adb_opendir(const char* path) { std::wstring path_wide;