From 0be1fde9626c58beffa02a036e00b784ca2b1b13 Mon Sep 17 00:00:00 2001
From: Rishabh Sharma <35334635+rishabh1704@users.noreply.github.com>
Date: Thu, 26 May 2022 11:09:49 +0530
Subject: [PATCH] HDFS-16561. Handle error returned by strtol

* strtol is used in hdfs-chmod.cc. The call
  to strtol could error out when an invalid
  input is provided.
* This PR handles the error given out by
   strtol.
---
 .../libhdfspp/tests/tools/hdfs-chmod-mock.cc      |  9 +++++++++
 .../libhdfspp/tests/tools/hdfs-tool-tests.h       | 13 +++++++++++++
 .../libhdfspp/tools/hdfs-chmod/hdfs-chmod.cc      | 15 ++++++++++++++-
 3 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/tools/hdfs-chmod-mock.cc b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/tools/hdfs-chmod-mock.cc
index e0df32cc2a..3c4ea24e0d 100644
--- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/tools/hdfs-chmod-mock.cc
+++ b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/tools/hdfs-chmod-mock.cc
@@ -55,6 +55,15 @@ void ChmodMock::SetExpectations(
         .WillOnce(testing::Return(true));
   }
 
+  if (*test_case_func == &PassInvalidPermissionsAndAPath<ChmodMock>) {
+    const auto arg1 = args[0];
+    const auto arg2 = args[1];
+
+    EXPECT_CALL(*this, HandlePath(arg1, false, arg2))
+        .Times(1)
+        .WillOnce(testing::Return(false));
+  }
+
   if (*test_case_func == &PassRecursivePermissionsAndAPath<ChmodMock>) {
     const auto arg1 = args[1];
     const auto arg2 = args[2];
diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/tools/hdfs-tool-tests.h b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/tools/hdfs-tool-tests.h
index 4fef261d0e..8a1ae8cf0b 100644
--- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/tools/hdfs-tool-tests.h
+++ b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tests/tools/hdfs-tool-tests.h
@@ -175,6 +175,19 @@ template <class T> std::unique_ptr<T> PassPermissionsAndAPath() {
   return hdfs_tool;
 }
 
+template <class T> std::unique_ptr<T> PassInvalidPermissionsAndAPath() {
+  constexpr auto argc = 3;
+  static std::string exe("hdfs_tool_name");
+  static std::string arg1("123456789123456789123456789");
+  static std::string arg2("g/h/i");
+
+  static char *argv[] = {exe.data(), arg1.data(), arg2.data()};
+
+  auto hdfs_tool = std::make_unique<T>(argc, argv);
+  hdfs_tool->SetExpectations(PassInvalidPermissionsAndAPath<T>, {arg1, arg2});
+  return hdfs_tool;
+}
+
 template <class T> std::unique_ptr<T> PassRecursivePermissionsAndAPath() {
   constexpr auto argc = 4;
   static std::string exe("hdfs_tool_name");
diff --git a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tools/hdfs-chmod/hdfs-chmod.cc b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tools/hdfs-chmod/hdfs-chmod.cc
index 4775860fb4..cd5aefabfc 100644
--- a/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tools/hdfs-chmod/hdfs-chmod.cc
+++ b/hadoop-hdfs-project/hadoop-hdfs-native-client/src/main/native/libhdfspp/tools/hdfs-chmod/hdfs-chmod.cc
@@ -140,8 +140,21 @@ bool Chmod::HandlePath(const std::string &permissions, const bool recursive,
   /*
    * strtol is reading the value with base 8, NULL because we are reading in
    * just one value.
+   *
+   * The strtol function may result in errors so check for that before
+   * typecasting.
    */
-  auto perm = static_cast<uint16_t>(strtol(permissions.c_str(), nullptr, 8));
+  errno = 0;
+  long result = strtol(permissions.c_str(), nullptr, 8);
+  bool all_0_in_permission = std::all_of(permissions.begin(), permissions.end(),
+                                         [](char c) { return c == '0'; });
+  /*
+   * The errno is set to ERANGE incase the string doesn't fit in long
+   * Also, the result is set to 0, in case conversion is not possible
+   */
+  if ((errno == ERANGE) || (!all_0_in_permission && result == 0))
+    return false;
+  auto perm = static_cast<uint16_t>(result);
   if (!recursive) {
     fs->SetPermission(uri.get_path(), perm, handler);
   } else {