4
\$\begingroup\$

I have written a cryptographic hashing class named Ccksum::DigestHasher that allows the caller to select a hashing algorithm by name via OpenSSL's EVP interface. This class is the bread and butter of my Ccksum programming project. Here is the code so far (I combined the header and source files for the readability of this post. I also removed all documentation comments, because I believe the code speaks for itself):

#include <iomanip>
#include <memory>
#include <openssl/evp.h>
#include <stdexcept>
#include <sstream>
#include <string>
#include <vector>

namespace Ccksum {
class Hasher {
public:
    explicit Hasher() = default;
    virtual ~Hasher() noexcept = default;
    virtual std::string process(std::istream& input) const = 0;
    virtual std::string name() const = 0;
};

class DigestHasher final : public Hasher {
public:
    explicit DigestHasher(const std::string& name)
        : m_context(EVP_MD_CTX_new(), EVP_MD_CTX_free)
        , m_algorithm(EVP_get_digestbyname(name.c_str()))
        , m_name(name)
    {
        if (m_context == nullptr) {
            throw std::runtime_error("Cannot create hash algorithm context");
        }

        if (m_algorithm == nullptr) {
            std::ostringstream errorMsg;
            errorMsg << name << " is not a valid hash algorithm";
            throw std::runtime_error(errorMsg.str());
        }
    }

    ~DigestHasher() noexcept override = default;

    std::string process(std::istream& input) const override
    {
        if (!input.good()) {
            throw std::runtime_error("Input stream is bad");
        }

        constexpr size_t bufferSize = 65536; // 64K seems good...
        std::vector<char> buffer(bufferSize);
        std::vector<unsigned char> hash(EVP_MD_size(m_algorithm));

        if (EVP_DigestInit_ex(m_context.get(), m_algorithm, nullptr) == 0) {
            throw std::runtime_error("Cannot initialize hash algorithm");
        }

        while (input.good()) {
            input.read(buffer.data(), bufferSize);
            if (EVP_DigestUpdate(m_context.get(), buffer.data(), input.gcount()) == 0) {
                throw std::runtime_error("Cannot update hash context");
            }
        }

       if (EVP_DigestFinal_ex(m_context.get(), hash.data(), nullptr) == 0) {
           throw std::runtime_error("Cannot finalize hash context");
       }

       std::ostringstream formatHash;
       formatHash << std::hex << std::setfill('0');

       for (const auto& iterate : hash) {
           formatHash << std::setw(2) << static_cast<unsigned int>(iterate);
       }

        return formatHash.str();
    }

    std::string name() const override
    {
        return m_name;
    }

private:
    std::unique_ptr<EVP_MD_CTX, decltype(&EVP_MD_CTX_free)> m_context;
    const EVP_MD* m_algorithm;
    const std::string m_name;
};
} // Ccksum namespace

NOTE: The Ccksum::Hasher abstract class exists for mocking purposes. My plan is to mock Ccksum::DigestHasher using Ccksum::Hasher to make it easier to unit test the Ccksum::GenerateMode class I am currently working on.

Question 1: Should I wrap m_algorithm in a std::unique_ptr or leave it the way it is?

Current rationale regarding question 1: I left m_algorithm as a raw const pointer of type EVP_MD, because the example code offered by OpenSSL's docs does not show freeing its EVP_MD variable, nor did I find any kind of EVP_MD_free() function. A Stack Overflow answer states that there is no need to free EVP_MD at all. As a sanity check, I ran my code through valgrind and no memory leaks were found.

EDIT 1: I changed the bufferSize constant to 64KB. This article states that a buffer size of 64KB is a reasonable default for modern desktops.

\$\endgroup\$
3
\$\begingroup\$

This looks quite good already. However, some improvements can be made:

Avoid unnecessary use of stringstreams

String streams are quite heavy, so using it to build small strings is perhaps not the way to go. Instead of:

std::ostringstream errorMsg;
errorMsg << name << " is not a valid hash algorithm";
throw std::runtime_error(errorMsg.str());

You can just write:

throw std::runtime_error(name + " is not a valid hash algorithm");

With C++20 (or using the {fmt} library for older versions of the C++ standard), you can format strings the C way, but in a type-safe way:

throw std::runtime_error(std::format("{} is not a valid hash algorithm", name));

Incomplete error checking

process() is checking the state of the input string right at the start of the function, but then ignores any errors that might happen later on. What you should do is check that input.eof() is true after the while-loop; you can even omit the check at the start.

Return the raw hash value

process() calculates the hash and converts it to a hexadecimal string. But what if I just want to convert two hash values, which can be done more efficiently if they were just in the original binary encoding? Or what if I need the hash in base64 format? I would apply the separation of concerns design principle, and just let process() return a std::vector<unsigned char> (even better, use std::byte instead of unsigned char). Let the caller worry about converting it to other representations if so desired.

\$\endgroup\$
4
  • \$\begingroup\$ Thank you for your input! I will update my code base and the code of my post to reflect your recommendations. \$\endgroup\$
    – user245257
    Jul 23 at 19:33
  • \$\begingroup\$ @g-sliepen I forgot to ask, when you state to check input.eof() after the while loop, would it also be correct to replace while (input.good()) with while (!input.eof()) instead of checking for input.eof() after while? \$\endgroup\$
    – user245257
    Jul 23 at 19:54
  • 1
    \$\begingroup\$ @JasonPena No, because you might encounter an error before reaching the end of the file. The while-loop should just check if it makes sense to try to read() some more, after the loop you check whether you have really read all of the input. Also: don't edit your post, if you want updated code to be reviewed, just create a new question! \$\endgroup\$
    – G. Sliepen
    Jul 23 at 20:07
  • 1
    \$\begingroup\$ @g-sliepen Okay, so I check for input.eof() after the while loop and throw if it is not true as a replacement for the original sanity check at the start of process(). Thanks! \$\endgroup\$
    – user245257
    Jul 23 at 20:11

Your Answer

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