From e73416e9e39118a3eb7a10d4d3f434ef7cc1c4ba Mon Sep 17 00:00:00 2001
From: David Steele <david@pgmasters.net>
Date: Wed, 5 Dec 2018 17:56:47 -0500
Subject: [PATCH] Change file ownership only when required.

Previously chown() would be called even when no ownership changes were required.

In most cases changes are not required and it seems better to perform an extra stat() rather than an extra chown().

Also add unit tests for owner() since there weren't any.
---
 doc/xml/release.xml                           |  4 ++
 lib/pgBackRest/Storage/Posix/Driver.pm        | 29 ++++++---------
 src/perl/embed.auto.c                         | 11 ++----
 test/define.yaml                              |  2 +-
 .../Module/Storage/StoragePosixPerlTest.pm    | 37 +++++++++++++++++++
 5 files changed, 57 insertions(+), 26 deletions(-)

diff --git a/doc/xml/release.xml b/doc/xml/release.xml
index 4b7b3b698..13efe1ba1 100644
--- a/doc/xml/release.xml
+++ b/doc/xml/release.xml
@@ -37,6 +37,10 @@
                 </release-bug-list>
 
                 <release-improvement-list>
+                    <release-item>
+                        <p>Change file ownership only when required.</p>
+                    </release-item>
+
                     <release-item>
                         <release-item-contributor-list>
                             <release-item-ideator id="brad.nicholson"/>
diff --git a/lib/pgBackRest/Storage/Posix/Driver.pm b/lib/pgBackRest/Storage/Posix/Driver.pm
index 4b189d998..e85e4b42b 100644
--- a/lib/pgBackRest/Storage/Posix/Driver.pm
+++ b/lib/pgBackRest/Storage/Posix/Driver.pm
@@ -698,19 +698,16 @@ sub owner
 
         # If the user or group is not defined then get it by stat'ing the file.  This is because the chown function requires that
         # both user and group be set.
-        if (!(defined($strUser) && defined($strGroup)))
+        my $oStat = $self->info($strFilePath);
+
+        if (!defined($strUser))
         {
-            my $oStat = $self->info($strFilePath);
+            $iUserId = $oStat->uid;
+        }
 
-            if (!defined($strUser))
-            {
-                $iUserId = $oStat->uid;
-            }
-
-            if (!defined($strGroup))
-            {
-                $iGroupId = $oStat->gid;
-            }
+        if (!defined($strGroup))
+        {
+            $iGroupId = $oStat->gid;
         }
 
         # Lookup user if specified
@@ -735,15 +732,13 @@ sub owner
             }
         }
 
-        # Set ownership on the file
-        if (!chown($iUserId, $iGroupId, $strFilePath))
+        # Set ownership on the file if the user or group would be changed
+        if ($iUserId != $oStat->uid || $iGroupId != $oStat->gid)
         {
-            if ($OS_ERROR{ENOENT})
+            if (!chown($iUserId, $iGroupId, $strFilePath))
             {
-                logErrorResult(ERROR_FILE_OWNER, "${strMessage} because it is missing");
+                logErrorResult(ERROR_FILE_OWNER, "${strMessage}", $OS_ERROR);
             }
-
-            logErrorResult(ERROR_FILE_OWNER, "${strMessage}", $OS_ERROR);
         }
     }
 
diff --git a/src/perl/embed.auto.c b/src/perl/embed.auto.c
index 1eb85f30c..c66650e7c 100644
--- a/src/perl/embed.auto.c
+++ b/src/perl/embed.auto.c
@@ -19996,8 +19996,6 @@ static const EmbeddedModule embeddedModule[] =
             "my $iUserId;\n"
             "my $iGroupId;\n"
             "\n\n\n"
-            "if (!(defined($strUser) && defined($strGroup)))\n"
-            "{\n"
             "my $oStat = $self->info($strFilePath);\n"
             "\n"
             "if (!defined($strUser))\n"
@@ -20009,7 +20007,6 @@ static const EmbeddedModule embeddedModule[] =
             "{\n"
             "$iGroupId = $oStat->gid;\n"
             "}\n"
-            "}\n"
             "\n\n"
             "if (defined($strUser))\n"
             "{\n"
@@ -20031,16 +20028,14 @@ static const EmbeddedModule embeddedModule[] =
             "}\n"
             "}\n"
             "\n\n"
+            "if ($iUserId != $oStat->uid || $iGroupId != $oStat->gid)\n"
+            "{\n"
             "if (!chown($iUserId, $iGroupId, $strFilePath))\n"
             "{\n"
-            "if ($OS_ERROR{ENOENT})\n"
-            "{\n"
-            "logErrorResult(ERROR_FILE_OWNER, \"${strMessage} because it is missing\");\n"
-            "}\n"
-            "\n"
             "logErrorResult(ERROR_FILE_OWNER, \"${strMessage}\", $OS_ERROR);\n"
             "}\n"
             "}\n"
+            "}\n"
             "\n\n"
             "return logDebugReturn($strOperation);\n"
             "}\n"
diff --git a/test/define.yaml b/test/define.yaml
index 8bc2af125..0037c3562 100644
--- a/test/define.yaml
+++ b/test/define.yaml
@@ -458,7 +458,7 @@ unit:
 
       # ----------------------------------------------------------------------------------------------------------------------------
       - name: posix-perl
-        total: 9
+        total: 10
 
         coverage:
           Storage/Posix/Driver: partial
diff --git a/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm b/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm
index 6007ab4a4..a216f1e10 100644
--- a/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm
+++ b/test/lib/pgBackRestTest/Module/Storage/StoragePosixPerlTest.pm
@@ -19,6 +19,7 @@ use pgBackRest::Common::Exception;
 use pgBackRest::Common::Log;
 use pgBackRest::Storage::Posix::Driver;
 
+use pgBackRestTest::Common::ContainerTest;
 use pgBackRestTest::Common::ExecuteTest;
 use pgBackRestTest::Common::RunTest;
 
@@ -313,6 +314,42 @@ sub run
             sub {$oPosixIo->close()}, ERROR_FILE_WRITE, "unable to set time for '${strFile}': No such file or directory");
     }
 
+    ################################################################################################################################
+    if ($self->begin('owner()'))
+    {
+        my $strFile = $self->testPath() . "/test.txt";
+
+        $self->testException(
+            sub {$oPosix->owner($strFile, {strUser => 'root'})}, ERROR_FILE_MISSING,
+            "unable to stat '${strFile}': No such file or directory");
+
+        executeTest("touch ${strFile}");
+
+        $self->testException(
+            sub {$oPosix->owner($strFile, {strUser => BOGUS})}, ERROR_FILE_OWNER,
+            "unable to set ownership for '${strFile}' because user 'bogus' does not exist");
+        $self->testException(
+            sub {$oPosix->owner($strFile, {strGroup => BOGUS})}, ERROR_FILE_OWNER,
+            "unable to set ownership for '${strFile}' because group 'bogus' does not exist");
+
+        $self->testResult(sub {$oPosix->owner($strFile)}, undef, "no ownership changes");
+        $self->testResult(sub {$oPosix->owner($strFile, {strUser => TEST_USER})}, undef, "same user");
+        $self->testResult(sub {$oPosix->owner($strFile, {strGroup => TEST_GROUP})}, undef, "same group");
+        $self->testResult(
+            sub {$oPosix->owner($strFile, {strUser => TEST_USER, strGroup => TEST_GROUP})}, undef, "same user, group");
+
+        $self->testException(
+            sub {$oPosix->owner($strFile, {strUser => 'root'})}, ERROR_FILE_OWNER,
+            "unable to set ownership for '${strFile}': Operation not permitted");
+        $self->testException(
+            sub {$oPosix->owner($strFile, {strGroup => 'root'})}, ERROR_FILE_OWNER,
+            "unable to set ownership for '${strFile}': Operation not permitted");
+
+        executeTest("sudo chown :root ${strFile}");
+        $self->testResult(
+            sub {$oPosix->owner($strFile, {strGroup => TEST_GROUP})}, undef, "change group back from root");
+    }
+
     ################################################################################################################################
     if ($self->begin('pathCreate()'))
     {