3
\$\begingroup\$

Over at Stack Overflow I asked how to scan a local filesystem ignoring all remote mount points. I only really wanted a push in the right direction and received it. After clacking away on my keyboard I think I might have what I what I need. I ask for an evaluation to determine if it fulfills the requirements:

  1. Scan every local directory for world-writable files ignoring "special" directories (/sys, /proc, /dev)
  2. Ignore directories
  3. Ignore files on any remote filesystems which may be mounted
  4. Ignore any files in an "exclude" list

I'm quite certain it could be drastically optimized and any suggestions are welcome as are any suggestions for a better algorithm. However, rewriting the script is secondary to performing the task properly.

I've tested it and I'm fairly confident that I've created what I want. I just need some extra eyes to confirm.

#!/usr/bin/perl

# Directives which establish our execution environment
use warnings;
use strict;
use Fcntl ':mode';
use File::Find;
no warnings 'File::Find';
no warnings 'uninitialized';

# Variables used throughout the script
my $DIR = "/var/log/tivoli/";
my $MTAB = "/etc/mtab";
my $PERMFILE = "world_writable_w_files.txt";
my $TMPFILE = "world_writable_files.tmp";
my $EXCLUDE = "/usr/local/etc/world_writable_excludes.txt";
my $ROOT = "/";
my @devNum;

# Create an array of the file stats for "/"
my @rootStats = stat("${ROOT}");

# Compile a list of mountpoints that need to be scanned
my @mounts;

open MT, "<${MTAB}" or die "Cannot open ${MTAB}, $!";

# We only want the local mountpoints
while (<MT>) {
  if ($_ =~ /ext[34]/) {
    my @line = split;
    push(@mounts, $line[1]);

  }

}

close MT;

# Read in the list of excluded files and create a regex from them
my $regExcld = do {
  open XCLD, "<${EXCLUDE}" or die "Cannot open ${EXCLUDE}, $!\n";
  my @ignore = <XCLD>;
  chomp @ignore;
  local $" = '|';
  qr/@ignore/;

};

# Create a regex containing mountpoint dev numbers to compare file device numbers to.
my $devRegex = do {
  chomp @devNum;
  local $" = '|';
  qr/@devNum/;

};

# Create the output file path if it doesn't already exist.
mkdir("${DIR}" or die "Cannot execute mkdir on ${DIR}, $!") unless (-d "${DIR}");

# Create our filehandle for writing our findings
open WWFILE, ">${DIR}${TMPFILE}" or die "Cannot open ${DIR}${TMPFILE}, $!";

foreach (@mounts) {
  # The anonymous subroutine which is executed by File::Find
  find sub {
    # Is it in a basic (non-special) directory, ...
    return if $File::Find::dir =~ /sys|proc|dev/;

    # ...a regular file, ...
    return unless -f;

    # ...local, ...
    my @dirStats = stat($File::Find::name);
    return unless $dirStats[0] =~ $devRegex;

    # ...and world writable?
    return unless (((stat)[2] & S_IWUSR) && ((stat)[2] & S_IWGRP) && ((stat)[2] & S_IWOTH));

    # Add the file to the list of found world writable files unless it is
    # in the list if exclusions
    print(WWFILE, "$File::Find::name\n") unless ($File::Find::name =~ $regExcld);

  }, $_;

}

close WWFILE;

# If no world-writable files have been found ${TMPFILE} should be zero-size;
# Delete it so Tivoli won't alert
if (-z "${DIR}${TMPFILE}") {
  unlink "${DIR}${TMPFILE}";

} else {
  rename("${DIR}${TMPFILE}","${DIR}${PERMFILE}") or die "Cannot rename file ${DIR}${TMPFILE}, $!";

}
\$\endgroup\$
  • 2
    \$\begingroup\$ Tips on optimization and general code-quality are things you'll get from this site. However, wether or not this code fulfills your requirements is, IMO, part of your job (develop, debug, test, and develop some more). That's not entirely part of code-review. Anyway, welcome to the site \$\endgroup\$ – Elias Van Ootegem Apr 8 '14 at 7:38
  • \$\begingroup\$ You are correct. The writing, debugging, etc is my responsibility and I have done this. I guess I'm looking for validation :) \$\endgroup\$ – theillien Apr 8 '14 at 19:13
  • \$\begingroup\$ A few comments off the top of my head: 1) Don't turn off warnings; fix the code that emits the warnings. 2) Don't comment things that are obvious, e.g. "Directives which establish our execution environment" and "Variables used throughout the script." 3) Use lowercase for variables since built-in variables tend to use all-caps. 4) Use lexical filehandles and the 3-argument open, e.g. open my $fh, '<', $file or die "$file: $!"; 5) Instead of generating complex regexes (which you haven't anchored, by the way, so they will probably match more than you want), use hashes: (continue) \$\endgroup\$ – ThisSuitIsBlackNot Apr 16 '14 at 21:46
  • \$\begingroup\$ e.g. my %ignore = map { $_ => 1 } @array; 6) Don't call find in a loop, just call it once with @mounts as the second argument. 7) Write a separate wanted sub and pass it to find to improve readability. 8) Don't use rename as it's not portable. Instead use move from File::Copy. \$\endgroup\$ – ThisSuitIsBlackNot Apr 16 '14 at 21:53
  • \$\begingroup\$ I'm working through your suggestions. So far I've only modified variable names. I have a couple questions: When you say call find with @mounts as the second argument how do you mean? Would it be }, $_, @mounts;? For anchoring the regexes (If I stick with that rather than mapping to a hash) would this be correct: qr/^@devNum$/;? I'm not sure I know how to use a separate wanted sub. \$\endgroup\$ – theillien Apr 22 '14 at 22:33
3
\$\begingroup\$

Processing mount points

This can be improved in many ways:

# Compile a list of mountpoints that need to be scanned
my @mounts;
open MT, "<${MTAB}" or die "Cannot open ${MTAB}, $!";

# We only want the local mountpoints
while (<MT>) {
  if ($_ =~ /ext[34]/) {
    my @line = split;
    push(@mounts, $line[1]);
  }
}

close MT;

Using bare names for file handles is not recommended: replace MT with my $MT, and <MT> with <$MT>.

The comment talks about local mount points. That's too vague, when in fact you're looking specifically for ext3 and ext4 filesystems.

You could simplify if ($_ =~ /ext[34]/) as if (/ext[34]/).

You could put the entire while loop inside a map, and it will be still readable.

Putting it together, you can rewrite like this:

open my $MT, "<${MTAB}" or die "Cannot open ${MTAB}, $!";

# Compile a list of mountpoints that need to be scanned
# We only want the ext3 ext4 partitions
my @mounts = map((split)[1], grep(/ext[34]/, <$MT>));

close $MT;

Note that if you didn't find any mount points, the script has nothing to process, so it should exit:

die 'no mount points to process' unless @mounts;

Try to apply these techniques everywhere in the script, especially the bit about filehandlers.


# Read in the list of excluded files and create a regex from them
my $regExcld = do {
  open XCLD, "<${EXCLUDE}" or die "Cannot open ${EXCLUDE}, $!\n";
  my @ignore = <XCLD>;
  chomp @ignore;
  local $" = '|';
  qr/@ignore/;
};

As earlier, replace XCLD with my $XCLD or something. You also forgot to close it.

Btw, a common way to shorten my @arr = <$fh>; chomp @arr as chomp(my @arr = <$fh>).

Most importantly, should it be really a fatal error if the file with excluded patterns doesn't exist? It might make sense to make this optional. Perhaps issue a warning instead, something like this:

my $regExcld = do {
  if (open my $fh, "<${EXCLUDE}") {
      chomp(my @ignore = <$fh>);
      close($fh);
      local $" = '|';
      qr/@ignore/;
  } else {
      warn "Cannot open ${EXCLUDE}, $!\n";
      undef;
  }
};

This bit seems pointless:

# Create a regex containing mountpoint dev numbers to compare file device numbers to.
my $devRegex = do {
  chomp @devNum;
  local $" = '|';
  qr/@devNum/;
};

Because @devNum is an empty array in your script, you never actually gave it any values.


This is quite strange:

# Create the output file path if it doesn't already exist.
mkdir("${DIR}" or die "Cannot execute mkdir on ${DIR}, $!") unless (-d "${DIR}");

You misplaced the die: it will run if "${DIR}" is empty, not if mkdir fails.

Also, there's no need whatsoever to double-quote ${DIR}.

Corrected and simplified:

mkdir($DIR) or die "Cannot execute mkdir on ${DIR}, $!" unless -d $DIR;
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.