From c63cb070633864fe5424d8a2b6e44f0fd19ef08f Mon Sep 17 00:00:00 2001
From: liwugang <liwugang@xiaomi.com>
Date: Wed, 11 Jul 2018 13:24:49 +0800
Subject: [PATCH] libbase: return different result depend on the errno

In the RemoveFileIfExists it always return true even if error appeared
when using stat function.

It should distinguish different error. Such as ENOENT and ENOTDIR
we exactly know the file does not exist. But EACCES(current user has not
all search permission in the file path) and other errors appeared
we can't know whether file exits. So we should return false indicate
there are some error appeared.

Test: ran unit tests
Change-Id: I75788bf0621040812413d52596b5effb628fd0b1
Signed-off-by: liwugang <liwugang@xiaomi.com>
---
 base/file.cpp      | 10 ++++++++--
 base/file_test.cpp | 42 ++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/base/file.cpp b/base/file.cpp
index 2f697a1cc..d6fe753d1 100644
--- a/base/file.cpp
+++ b/base/file.cpp
@@ -199,17 +199,23 @@ bool WriteFully(int fd, const void* data, size_t byte_count) {
 bool RemoveFileIfExists(const std::string& path, std::string* err) {
   struct stat st;
 #if defined(_WIN32)
-  //TODO: Windows version can't handle symbol link correctly.
+  // TODO: Windows version can't handle symbolic links correctly.
   int result = stat(path.c_str(), &st);
   bool file_type_removable = (result == 0 && S_ISREG(st.st_mode));
 #else
   int result = lstat(path.c_str(), &st);
   bool file_type_removable = (result == 0 && (S_ISREG(st.st_mode) || S_ISLNK(st.st_mode)));
 #endif
+  if (result == -1) {
+    if (errno == ENOENT || errno == ENOTDIR) return true;
+    if (err != nullptr) *err = strerror(errno);
+    return false;
+  }
+
   if (result == 0) {
     if (!file_type_removable) {
       if (err != nullptr) {
-        *err = "is not a regular or symbol link file";
+        *err = "is not a regular file or symbolic link";
       }
       return false;
     }
diff --git a/base/file_test.cpp b/base/file_test.cpp
index 02b431de7..67946523c 100644
--- a/base/file_test.cpp
+++ b/base/file_test.cpp
@@ -26,6 +26,10 @@
 
 #include "android-base/test_utils.h"
 
+#if !defined(_WIN32)
+#include <pwd.h>
+#endif
+
 TEST(file, ReadFileToString_ENOENT) {
   std::string s("hello");
   errno = 0;
@@ -115,7 +119,7 @@ TEST(file, WriteFully) {
   ASSERT_FALSE(android::base::ReadFully(tf.fd, &s[0], s.size()));
 }
 
-TEST(file, RemoveFileIfExist) {
+TEST(file, RemoveFileIfExists) {
   TemporaryFile tf;
   ASSERT_TRUE(tf.fd != -1);
   close(tf.fd);
@@ -126,9 +130,43 @@ TEST(file, RemoveFileIfExist) {
   TemporaryDir td;
   ASSERT_FALSE(android::base::RemoveFileIfExists(td.path));
   ASSERT_FALSE(android::base::RemoveFileIfExists(td.path, &err));
-  ASSERT_EQ("is not a regular or symbol link file", err);
+  ASSERT_EQ("is not a regular file or symbolic link", err);
 }
 
+TEST(file, RemoveFileIfExists_ENOTDIR) {
+  TemporaryFile tf;
+  close(tf.fd);
+  tf.fd = -1;
+  std::string err{"xxx"};
+  ASSERT_TRUE(android::base::RemoveFileIfExists(std::string{tf.path} + "/abc", &err));
+  ASSERT_EQ("xxx", err);
+}
+
+#if !defined(_WIN32)
+TEST(file, RemoveFileIfExists_EACCES) {
+  // EACCES -- one of the directories in the path has no search permission
+  // root can bypass permission restrictions, so drop root.
+  if (getuid() == 0) {
+    passwd* shell = getpwnam("shell");
+    setgid(shell->pw_gid);
+    setuid(shell->pw_uid);
+  }
+
+  TemporaryDir td;
+  TemporaryFile tf(td.path);
+  close(tf.fd);
+  tf.fd = -1;
+  std::string err{"xxx"};
+  // Remove dir's search permission.
+  ASSERT_TRUE(chmod(td.path, S_IRUSR | S_IWUSR) == 0);
+  ASSERT_FALSE(android::base::RemoveFileIfExists(tf.path, &err));
+  ASSERT_EQ("Permission denied", err);
+  // Set dir's search permission again.
+  ASSERT_TRUE(chmod(td.path, S_IRWXU) == 0);
+  ASSERT_TRUE(android::base::RemoveFileIfExists(tf.path, &err));
+}
+#endif
+
 TEST(file, Readlink) {
 #if !defined(_WIN32)
   // Linux doesn't allow empty symbolic links.