Skip to content

Commit

Permalink
Fixed an issue where a missing user/group on restore could cause an "…
Browse files Browse the repository at this point in the history
…uninitialized value" error in File->owner().

Reported by Leonardo Avellar.
  • Loading branch information
dwsteele committed Jan 5, 2017
1 parent 4ff2714 commit 39744a4
Show file tree
Hide file tree
Showing 7 changed files with 176 additions and 19 deletions.
13 changes: 13 additions & 0 deletions doc/xml/release.xml
Expand Up @@ -17,6 +17,11 @@
</contributor>

<!-- The order of other contributors is alpha by name -->
<contributor id="avellar.leonardo">
<contributor-name-display>Leonardo Avellar</contributor-name-display>
<contributor-id type="github">L30Bola</contributor-id>
</contributor>

<contributor id="barber.chris">
<contributor-name-display>Chris Barber</contributor-name-display>
<contributor-id type="github">gamerscomplete</contributor-id>
Expand Down Expand Up @@ -154,6 +159,14 @@
<release-item>
<p>Fixed a few directory syncs that were missed for the <br-option>--repo-sync</br-option> option.</p>
</release-item>

<release-item>
<release-item-contributor-list>
<release-item-ideator id="avellar.leonardo"/>
</release-item-contributor-list>

<p>Fixed an issue where a missing user/group on restore could cause an <quote>uninitialized value</quote> error in <code>File->owner()</code>.</p>
</release-item>
</release-bug-list>

<release-feature-list>
Expand Down
6 changes: 6 additions & 0 deletions lib/pgBackRest/Common/Exception.pm
Expand Up @@ -144,8 +144,14 @@ use constant ERROR_LINK_OPEN => ERROR_MIN
push @EXPORT, qw(ERROR_LINK_OPEN);
use constant ERROR_ARCHIVE_DISABLED => ERROR_MINIMUM + 62;
push @EXPORT, qw(ERROR_ARCHIVE_DISABLED);
use constant ERROR_FILE_OWNER => ERROR_MINIMUM + 63;
push @EXPORT, qw(ERROR_FILE_OWNER);
use constant ERROR_USER_MISSING => ERROR_MINIMUM + 64;
push @EXPORT, qw(ERROR_USER_MISSING);
use constant ERROR_OPTION_COMMAND => ERROR_MINIMUM + 65;
push @EXPORT, qw(ERROR_OPTION_COMMAND);
use constant ERROR_GROUP_MISSING => ERROR_MINIMUM + 66;
push @EXPORT, qw(ERROR_GROUP_MISSING);

use constant ERROR_INVALID_VALUE => ERROR_MAXIMUM - 2;
push @EXPORT, qw(ERROR_INVALID_VALUE);
Expand Down
55 changes: 39 additions & 16 deletions lib/pgBackRest/File.pm
Expand Up @@ -866,53 +866,76 @@ sub owner
__PACKAGE__ . '->owner', \@_,
{name => 'strPathType'},
{name => 'strFile'},
{name => 'strUser'},
{name => 'strGroup'}
{name => 'strUser', required => false},
{name => 'strGroup', required => false}
);

# Set operation variables
my $strFileOp = $self->pathGet($strPathType, $strFile);

# Run remotely
if ($self->isRemote($strPathType))
{
confess &log(ASSERT, "${strOperation}: remote operation not supported");
}
# Run locally
else
{
my $iUserId;
my $iGroupId;
my $oStat;

if (!defined($strUser) || !defined($strGroup))
# 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)))
{
$oStat = stat($strFileOp);
my $oStat = fileStat($strFileOp);

if (!defined($strUser))
{
$iUserId = $oStat->uid;
}

if (!defined($oStat))
if (!defined($strGroup))
{
confess &log(ERROR, 'unable to stat ${strFileOp}');
$iGroupId = $oStat->gid;
}
}

# Lookup user if specified
if (defined($strUser))
{
$iUserId = getpwnam($strUser);
}
else
{
$iUserId = $oStat->uid;

if (!defined($iUserId))
{
confess &log(ERROR, "user '${strUser}' does not exist", ERROR_USER_MISSING);
}
}

# Lookup group if specified
if (defined($strGroup))
{
$iGroupId = getgrnam($strGroup);

if (!defined($iGroupId))
{
confess &log(ERROR, "group '${strGroup}' does not exist", ERROR_GROUP_MISSING);
}
}
else

# Set ownership on the file
if (!chown($iUserId, $iGroupId, $strFileOp))
{
$iGroupId = $oStat->gid;
}
my $strError = $!;

chown($iUserId, $iGroupId, $strFileOp)
or confess &log(ERROR, "unable to set ownership for ${strFileOp}");
if (fileExists($strFileOp))
{
confess &log(ERROR,
"unable to set ownership for '${strFileOp}'" . (defined($strError) ? ": $strError" : ''), ERROR_FILE_OWNER);
}

confess &log(ERROR, "${strFile} does not exist", ERROR_FILE_MISSING);
}
}

# Return from function and log return values if any
Expand Down
8 changes: 6 additions & 2 deletions test/lib/pgBackRestTest/Common/ContainerTest.pm
Expand Up @@ -247,7 +247,8 @@ sub sudoSetup
elsif ($$oVm{$strOS}{&VM_OS_BASE} eq VM_OS_BASE_DEBIAN)
{
$strScript .=
"\nRUN sed -i 's/^\\\%admin.*\$/\\\%${strGroup} ALL\\=\\(ALL\\) NOPASSWD\\: ALL/' /etc/sudoers";
"\nRUN apt-get -y install sudo\n" .
"\nRUN echo '%${strGroup} ALL=(ALL) NOPASSWD: ALL' >> /etc/sudoers";
}
else
{
Expand Down Expand Up @@ -568,7 +569,7 @@ sub containerBuild
containerWrite($strTempPath, $strOS, "${strTitle} Test", $strImageParent, $strImage, $strScript, $bVmForce, true, true);
}

# Db test image (for sythetic tests)
# Db test image (for synthetic tests)
########################################################################################################################
$strImageParent = containerNamespace() . "/${strOS}-base";
$strImage = "${strOS}-db-test";
Expand Down Expand Up @@ -600,6 +601,9 @@ sub containerBuild
"\n\n# Make " . TEST_USER . " home dir readable\n" .
'RUN chmod g+r,g+x /home/' . TEST_USER;

# Setup sudo
$strScript .= "\n\n" . sudoSetup($strOS, TEST_GROUP);

# Write the image
containerWrite($strTempPath, $strOS, 'Loop Test', $strImageParent, $strImage, $strScript, $bVmForce, true, true);

Expand Down
5 changes: 5 additions & 0 deletions test/lib/pgBackRestTest/Common/DefineTest.pm
Expand Up @@ -93,6 +93,11 @@ my $oTestDef =
&TESTDEF_TEST_TOTAL => 1,
&TESTDEF_TEST_INDIVIDUAL => false,
},
{
&TESTDEF_TEST_NAME => 'owner',
&TESTDEF_TEST_TOTAL => 8,
&TESTDEF_TEST_INDIVIDUAL => false,
},
{
&TESTDEF_TEST_NAME => 'path-create',
&TESTDEF_TEST_TOTAL => 8,
Expand Down
3 changes: 2 additions & 1 deletion test/lib/pgBackRestTest/Common/RunTest.pm
Expand Up @@ -245,7 +245,8 @@ sub testResult

my $strActual = $fnSub->();

if ($strActual ne $strExpected)
if (!defined($strExpected) && defined($strActual) || defined($strExpected) && !defined($strActual) ||
$strActual ne $strExpected)
{
confess
'expected ' . (defined($strExpected) ? "\"${strExpected}\"" : '[undef]') .
Expand Down
105 changes: 105 additions & 0 deletions test/lib/pgBackRestTest/File/FileOwnerTest.pm
@@ -0,0 +1,105 @@
####################################################################################################################################
# FileOwnerTest.pm - Tests for File->owner()
####################################################################################################################################
package pgBackRestTest::File::FileOwnerTest;
use parent 'pgBackRestTest::File::FileCommonTest';

####################################################################################################################################
# Perl includes
####################################################################################################################################
use strict;
use warnings FATAL => qw(all);
use Carp qw(confess);
use English '-no_match_vars';

use pgBackRest::Common::Exception;
use pgBackRest::Common::Log;
use pgBackRest::File;

use pgBackRestTest::Common::ExecuteTest;
use pgBackRestTest::Common::RunTest;

####################################################################################################################################
# run
####################################################################################################################################
sub run
{
my $self = shift;

# Loop through local/remote
foreach my $bRemote (false, true)
{
# Loop through exists
foreach my $bExists (!$bRemote ? (false, true) : (false))
{
# Loop through exists
foreach my $bError ($bExists && !$bRemote ? (false, true) : (false))
{
# Loop through users
foreach my $strUser ($bExists && !$bError && !$bRemote ? ($self->pgUser(), BOGUS, undef) : ($self->pgUser()))
{
# Loop through group
foreach my $strGroup (
$bExists && !$bError && !$bRemote && defined($strUser) && $strUser ne BOGUS ? ($self->group(), BOGUS, undef) : ($self->group()))
{
if (!$self->begin(
"rmt ${bRemote}, err ${bError}, exists ${bExists}, user " . (defined($strUser) ? $strUser : '[undef]') .
", group " . (defined($strGroup) ? $strGroup : '[undef]'))) {next}

# Setup test directory and get file object
my $oFile = $self->setup($bRemote, $bError);

# Create the test file
my $strFile = $self->testPath() . '/test.txt';

if ($bExists)
{
executeTest("echo 'TESTDATA' > ${strFile}");

if ($bError)
{
executeTest("sudo chown root:root ${strFile}");
}
}

my $strFunction = sub {$oFile->owner(PATH_BACKUP_ABSOLUTE, $strFile, $strUser, $strGroup)};

# Remote operation not allowed
if ($bRemote)
{
$self->testException($strFunction, ERROR_ASSERT, "pgBackRest::File->owner: remote operation not supported");
}
# Error on not exists
elsif (!$bExists)
{
$self->testException($strFunction, ERROR_FILE_MISSING, "${strFile} does not exist");
}
# Else permission error
elsif ($bError)
{
$self->testException(
$strFunction, ERROR_FILE_OWNER, "unable to set ownership for '${strFile}': Operation not permitted");
}
# Else bogus user
elsif (defined($strUser) && $strUser eq BOGUS)
{
$self->testException($strFunction, ERROR_USER_MISSING, "user '" . BOGUS . "' does not exist");
}
# Else bogus group
elsif (defined($strGroup) && $strGroup eq BOGUS)
{
$self->testException($strFunction, ERROR_GROUP_MISSING, "group '" . BOGUS . "' does not exist");
}
# Else success
else
{
$self->testResult($strFunction, "0");
}
}
}
}
}
}
}

1;

0 comments on commit 39744a4

Please sign in to comment.