5
\$\begingroup\$

I'm back for more community punishment review of my own scripts and code! This time, I'm looking for general code review of my approach to a Python way of getting one or more hash sums for a provided file path (unfortunately, it's one-file-at-a-time, and only accepts one file path at a time). Hash sum selection is done via argument flags, and if none are present, the default is to run MD5 and SHA1 hash sum functions and return only those.

Note that this script is designed to generate hashes, and not compare them with other hashes. It does not include the corresponding "compare" function that many hash summing tools provide, and it is not meant to - I'm writing something else for this comparison functionality.

Also note that this uses a third party module, but I am considering removing it in the future (I needed a 'failure' code status to process easily...)

I have confirmed this code works in Windows, Mac, and *nix variants, and generates valid hash sums, the same as any file hashing program that is distributed for operating systems. I have verified this, using built-ins on Linux/Unix, built-ins on Mac, and Windows-based programs such as WinMD5Sum or Rapid CRC Unicode to generate the hashes for a file, and comparing them to the outputs from my script. This is a good thing, so I know it works and correctly processes files and returns accurate hashes. I also permit specification of what hashes to run, by argparse and optional 'argument flag' arguments, and all that works - this is useful for people that need more than one hash function, and can get the file hash all in one program rather than

A few notes before we go into this about some fears I have, and what issues I think may / may not exist. While this can be used for some specific review focuses, please feel free to consider this as guideposts, and not an all-inclusive list of review points I need:

  1. This script is designed to process files for hashing in chunks. If the provided file size is greater than one million bytes (~1MB), it will process the file in one-million-byte chunks. If the provided file is less than one-million-bytes in size, it just processes the file directly and takes the whole file into memory. I have a concern though, about files still taking a huge amount of memory, for larger files - if this is not actually a concern given how I'm chunking files on read, then please tell me, so I'm not worried about writing memory-inefficient things.
  2. I have a horrible approach to handling output and formatting, in which I use a lot of conditional checks. I know this, and would love to have any types of improvement to processing (I currently have a lot of if statements and such, which I know is inefficient). Any suggestions to minimizing the amount of conditional functions here are appreciated.
  3. I tend to prepare for the worst, and very likely overdo the amount of error catching, and probably need to reduce how much of this I do. Any suggestions are welcome here, because I know i need to stop trying to wrap everything in try/excepts, and reduce the amount of these I'm executing.
  4. I love the argparse library's ArgumentParser for handling arguments, but I know that I abuse some protected properties here for one of my needs. Any method to bypass the need to alter a protected property of the argparse ArgumentParser functions for this is appreciated to get it to have Title Case for "optional arguments" header in help output, because altering protected properties instinctively makes me hate myself for possibly introducing library changes that will break in the future down the road in undefined and unforseen ways.

A note regarding PEP8: I know my lines are over 80 characters in length for code and comments. I should probably apply PEP8 restrictions on comments, but I have accepted a line length maximum of 100 characters, as I am the only person who's been working on this.

I thank you ahead of time for reviewing my code, and thank you for your thoughts and suggestions.


Prerequisite: Third-party module, exitstatus


compute-file-hashes.py

#!/usr/bin/env python
import os
import hashlib
import argparse
from exitstatus import ExitStatus


# Customized exit handler, though we can't let it be called anywhere else.
def _exit(error_occurred=False, err=None):
    if error_occurred:
        print "\r\nProcess ended in error, details:"
        print _get_exception_string(err)
        exit(ExitStatus.failure)
    else:
        exit(ExitStatus.success)


# Custom built-in for formatting Exception output in a friendly-ish way.
def _get_exception_string(exception=None):
    # errtype=type(e).__name__, errmsg=str(e)
    # print "[%s] %s" % (errtype, errmsg)
    return "[%s] %s" % (type(exception).__name__, str(e))


# Return the size of the file in bytes, necessary for chunking determination
def _get_filesize(path):
    return os.path.getsize(path)


# Handler for hash computations, so we don't have to repeat the same 'hash it' code
# four times over
#
# noinspection PyUnusedLocal,PyShadowingNames
def _compute_hash(filepath, digest):
    try:
        # Init the hashlib object
        if digest == 'md5':
            filehash = hashlib.md5()
        elif digest == 'sha1':
            filehash = hashlib.sha1()
        elif digest == 'sha256':
            filehash = hashlib.sha256()
        elif digest == 'sha512':
            filehash = hashlib.sha512()
        else:
            raise TypeError(
                "Invalid digest type specified, accepted are: md5 , sha1, sha256, sha512")

        # Open the file specified in the filepath.
        with open(filepath, "rb") as filename:
            # Check file size, and chunk it if it's greater than 1MB
            if _get_filesize(filepath) > 1000000:
                for chunk in iter(lambda: filename.read(4096), b""):
                    # For each chunk, incorporate the chunk into the hash sum.
                    filehash.update(chunk)
            else:
                # If we don't need to chunk, run the hash sum directly on the file (<= 1MB only)
                filehash.update(filename.read())

        # Return the MD5 hash sum (hexadecimal) to the end user.
        return filehash.hexdigest()
    except Exception as e:
        raise RuntimeError(str(e))


# noinspection PyShadowingNames,PyProtectedMember
def _get_arguments():
    # Argument Parser for the overall function
    parser = argparse.ArgumentParser(
        description="Run hash sum functions on a file and return them (default: "
                    "run both MD5 and SHA1, if no hash sum flags provided).",
        add_help=True)

    # Reset the title for "Optional Arguments" (help builtin)
    # NOTE: May introduce undefined behavior in future, be careful!
    parser._optionals.title = "Optional Arguments"

    # Argument Group for Hash Sum Flags, which define what hashes to run when at least one
    # of these args is specified.
    sum_flags = parser.add_argument_group('Hash Sum Flags')
    sum_flags.add_argument('-md5', required=False, action="store_true",
                           default=False,
                           help="Generate an MD5 sum for the file.")
    sum_flags.add_argument('-sha1', required=False, action="store_true",
                           default=False,
                           help="Generate a SHA1 sum for the file.")
    sum_flags.add_argument('-sha256', required=False, action="store_true",
                           default=False,
                           help="Generate a SHA256 sum for the file.")
    sum_flags.add_argument('-sha512', required=False, action="store_true",
                           default=False,
                           help="Generate a SHA512 sum for the file.")

    # Argument Group for Required Arguments, i.e. the path to the file.
    required_args = parser.add_argument_group('Required Arguments')
    required_args.add_argument('filepath', help="Path to file to run sums on.",
                               metavar="filepath")

    # Actually parse the args, and return this to the system.
    return parser.parse_args()


def main():
    # Init the vars for sums, used for checks later.
    md5 = None
    sha1 = None
    sha256 = None
    sha512 = None

    try:
        # Provide some useful data to user about progress, and what file we're actually checking.
        print "FilePath: %s\r\n" % filepath
        if "md5" in SUMS_TO_RUN:
            print "Generating MD5 sum, this could take some time..."
            # Actually compute MD5
            md5 = _compute_hash(filepath, 'md5')
            # More useful progress data.
            print "MD5 hash calculated.\r\n"

        if "sha1" in SUMS_TO_RUN:
            print "Generating SHA1 sum, this could take some time..."
            # Actually compute SHA1
            sha1 = _compute_hash(filepath, 'sha1')
            # More useful progress data.
            print "SHA1 hash calculated.\r\n"

        if "sha256" in SUMS_TO_RUN:
            print "Generating sha256 sum, this could take some time..."
            # Actually compute sha256
            sha256 = _compute_hash(filepath, 'sha256')
            # More useful progress data.
            print "SHA256 hash calculated.\r\n"

        if "sha512" in SUMS_TO_RUN:
            print "Generating SHA512 sum, this could take some time..."
            # Actually compute sha512
            sha512 = _compute_hash(filepath, 'sha512')
            # More useful progress data.
            print "SHA512 hash calculated.\r\n"

        print "Done, output below.\r\n\r\n"
        print "FilePath: %s\r\n" % filepath
        print "Requested Hash Sums:"
        if md5:
            print "    MD5 sum: %s" % str(md5)
        if sha1:
            print "   SHA1 sum: %s" % str(sha1)
        if sha256:
            print " SHA256 sum: %s" % str(sha256)
        if sha512:
            print " SHA512 sum: %s" % str(sha512)

    except Exception as err:
        # If the process dies off for any reason, we need to catch the exception and state it back
        # to the user, then die off gracefully.
        print "Process died with error: %s" % str(err)
        _exit(error_occurred=True, err=err)

    _exit()


# We have a few things we need to run *before* we get to main().
# Namely, argument parsing, cleaning the file paths slightly, and
# also checking if the file path exists for checking.
if __name__ == "__main__":
    # noinspection PyBroadException
    try:
        # Init "SUMS_TO_RUN", which will contain what hash sums we should run.
        SUMS_TO_RUN = []

        # First, we need to get the arguments.
        args = _get_arguments()

        # Store the filepath argument.
        filepath = args.filepath

        # First check if the filepath contains any slashes of any kind, and if not, rewrite path
        # to cwd.  Also triggers if path is `./blah`.
        if ('/' not in filepath and '\\' not in filepath) or ('./' in filepath):
            if '/' in os.getcwd():
                filepath = os.getcwd() + '/' + filepath
            elif '\\' in os.getcwd():
                filepath = os.getcwd() + '\\' + filepath

        # Now we check if the path provided exists
        if not os.path.exists(filepath):
            raise IOError(
                "The specified file path does not exist. Provided path: %s" % filepath)

        # PreProcessing: Determine from args which functions to run.
        if not (args.md5 or args.sha1 or args.sha256 or args.sha512):
            SUMS_TO_RUN = ['md5', 'sha1']
        else:
            if args.md5:
                SUMS_TO_RUN.append('md5')
            if args.sha1:
                SUMS_TO_RUN.append('sha1')
            if args.sha256:
                SUMS_TO_RUN.append('sha256')
            if args.sha512:
                SUMS_TO_RUN.append('sha512')

        # Now, call main() which will execute the actual hash functions.
        main()

    except Exception as e:
        _exit(error_occurred=True, err=e)

    else:
        _exit()
\$\endgroup\$
  • \$\begingroup\$ If for some reason you prefer to read the code elsewhere, a copy of this exists on a GitHub Gist if you prefer to read it and work with it there. \$\endgroup\$ – Thomas Ward Aug 10 '16 at 23:27
  • 1
    \$\begingroup\$ You should probably mention that this requires a third-party module. \$\endgroup\$ – zondo Aug 11 '16 at 0:24
  • \$\begingroup\$ @zondo Added, though I'm considering removal of it in a future revision. \$\endgroup\$ – Thomas Ward Aug 11 '16 at 0:31
5
\$\begingroup\$

Prefer a dictionary to if/elseif/else cascade:

    digests = { "md5": hashlib.md5, "sha1": hashlib.sha1, ... }

then compute_hash streamlines to

    try:
        filehash = digests[digest]()
        with open(filepath, "rb") as filename:
        ....
    except KeyError:
        raise TypeError("Invalid digest type ...")

Notice that you may reuse the same dictionary in your main:

    for key in digests.keys():
        if key in SUMS_TO_RUN:
            print "Generating ", key, "sum, this could take some time"
            ....

Rewriting the file path is absolutely not necessary. More, testing for the file to exist is also not necessary: you may safely rely on open to fail if the file doesn't exist. In fact, testing beforehand is subject to TOC-TOU race.


There is no need to special-case "small" files. It is effectively the same thing as the last (incomplete) chunk of a "large" file. Consequently, there is no need to determine file size beforehand (which is yet another TOC-TOU suspect).


Parsing arguments is perfectly valid job for main. I don't see the reason to lift it out.

\$\endgroup\$
  • \$\begingroup\$ Just avoid indexing a dictionary in your with, otherwise you'd need to move it to an else block. \$\endgroup\$ – Peilonrayz Aug 11 '16 at 14:53
1
\$\begingroup\$

In addition to the comments from @vnp, which I agree with, consider simplifying your argparse to something like:

import argparse

digests = { "md5": None, "sha1": None, "sha256": None}

def _get_arguments(args=None):
    # Argument Parser for the overall function
    parser = argparse.ArgumentParser(
        description="Run hash sum functions on a file and return them.")
    parser.add_argument('filepath',
                        help="Path to file to run sums on.")
    parser.add_argument('--hashes',
                        nargs='*',
                        choices=digests,
                        default=["md5", "sha1"],
                        help="Hashes to be used. Default: md5, sha1")
    return parser.parse_args(args)

print _get_arguments()

The choices of the hashes can use the dictionary of defined hashes @vnp recommended as well (No need for digests.keys() here, argparse does it correctly even when given a dictionary instead of a list).

I added the args parameter to allow interactive testing of this function.

$ python argparse_new.py file
Namespace(filepath='file', hashes=['md5', 'sha1'])

$ python argparse_new.py file --hashes md5
Namespace(filepath='file', hashes='md5')

$ python argparse_new.py file --hashes md5 sha256
Namespace(filepath='file', hashes=['md5', 'sha256'])

$ python argparse_new.py --hashes md5 sha256
usage: argparse_new.py [-h]
                       [--hashes [{sha256,sha1,md5} [{sha256,sha1,md5} ...]]]
                       filepath
argparse_new.py: error: too few arguments
\$\endgroup\$
  • \$\begingroup\$ There's a problem somewhere here, and it's not a 'me not understanding the theory here' problem. In practical testing here, I ran a test by running this script with the altered _get_arguments. The command run was /usr/bin/python2.7 /home/teward/tmp/compute-file-hashes.py --filepath /home/teward/tmp/temp.txt md5 sha1 which should run the script, and do what you want it to with argparse. I put debug code in to spit out the args and quit, for testing. It returns these args, which are garbage and useless: Namespace(filepath='sha1', hashes=['md5']) (using -f didn't help either) \$\endgroup\$ – Thomas Ward Aug 11 '16 at 12:17
  • \$\begingroup\$ Modifying digests as shown in the latest form of the code breaks all of @nvp's suggestions, which replace it intentionally with a dict which correlates to the specific hash library functions to use. (Replacing the hashlib.sha1 with None will break his (and my) updated handling of hash algorithm detection in _compute_hash and elsewhere) \$\endgroup\$ – Thomas Ward Aug 11 '16 at 13:45
  • \$\begingroup\$ Ah, I see. I misread the code (proof I need more coffee!). That said, the latest iteration of the code works as intended, but flips the logical ordering of what I had initially intended (file must be specified first, then hash sums), but that should be fine, given that it simplifies things a little. \$\endgroup\$ – Thomas Ward Aug 11 '16 at 13:50
  • \$\begingroup\$ Yeah, it turns it around a bit. But it does get rid of having to specify a flag for every hash, making it easier to add more hashes in the future. I think it also makes sense to have the required argument first and then the optionals at the end. \$\endgroup\$ – Graipher Aug 11 '16 at 13:53
  • \$\begingroup\$ I can see merit in both ways. That said, I have not found any need for more than the basic SHA filehashes, nor implementations for other things (CRC, etc.) at this time. The original purpose here was to create file hashes used for file integrity checks, such as on Ubuntu ISOs, or files where we need to specifically verify integrity on things, and I never saw any need for more than MD5, and SHA{1,256,512}. System's yelling about extended convos. in comments, so I have no more comments (any more, and I'll create a chat room heh) \$\endgroup\$ – Thomas Ward Aug 11 '16 at 13:57

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.