A WinAPI C++ program for printing the master boot record of the hard drive

Question

I have rolled this program for reading the MBR:

#define WIN32_LEAN_AND_MEAN

#include <Windows.h>
#include <cctype>
#include <iostream>
#include <string>
#include <utility>

using std::isprint;
using std::cout;
using std::cin;
using std::move;
using std::size_t;
using std::string;

constexpr size_t MBR_SIZE = 512;
constexpr size_t BYTES_PER_LINE = 4;
constexpr size_t NUMBER_OF_LINES = MBR_SIZE / BYTES_PER_LINE;

// Converts 4 least significant bits of 'c' to corresponding 
// hexadecimal string.
static string fourBitsToString(char c)
{
    string s = " ";

    if (c >= 0 && c <= 9) {
        s[0] = '0' + c;
    } else {
        c -= 10;
        s[0] = 'A' + c;
    }

    return move(s);
}

// Converts a character to its hexadecimal representation.
static string charToHex(char c)
{
    char lo = c & 0xf;
    char hi = (c >> 4) & 0xf;
    string s;
    char chars[] = {hi, lo};

    for (char ch : chars) {
        s += fourBitsToString(ch);
    }

    return std::move(s);
}

// Prints the MBR to console.
static void PrintMBR(char buffer[MBR_SIZE])
{
    size_t byteIndex = 0;
    string lineSeparator;
    string columnSeparator;

    for (size_t i = 0; i < NUMBER_OF_LINES; i++) {
        cout << lineSeparator;
        lineSeparator = '\n';
        columnSeparator = "";

        for (size_t j = 0; j < BYTES_PER_LINE; j++) {
            string ch = charToHex(buffer[byteIndex++]);
            cout << columnSeparator << ch;
            columnSeparator = " ";
        }

        cout << ' ';

        for (size_t j = 0; j < BYTES_PER_LINE; j++) {
            char c = buffer[byteIndex - BYTES_PER_LINE + j];
            cout << (isprint((unsigned int) c) ? c : '.');
        }
    }
}

// Reads the entire master boot record (MBR) into lpBuffer. 
// Expects lpBuffer to point to a memory point that may     
// hold at leasst 512 bytes.                                
static DWORD ReadMBR(PCHAR lpBuffer)
{

    HANDLE diskHandle = CreateFile(
        TEXT("\\\\.\\PhysicalDrive0"),
        GENERIC_READ,
        FILE_SHARE_READ | FILE_SHARE_WRITE,
        NULL,
        OPEN_EXISTING,
        0,
        NULL);

    char* buffer = new char[MBR_SIZE];

    ReadFile(
        diskHandle,
        lpBuffer,
        MBR_SIZE,
        NULL,
        NULL);

    return GetLastError();
}

int main() {
    char buffer[MBR_SIZE];
    ReadMBR(buffer);
    PrintMBR(buffer);
    cin.get();
    return 3;
}

(Compile as Release instead of Debug if it throws a nasty runtime breakpoint.)

I would love to hear any comments possible, but my main question is whether it is idiomatic C/C++/WinAPI.

Note: To see the actual MBR, run this as administrator.

Answer 1

fourBitsToString and charToHex

Both of these functions are basically reimplementations of existing features of the <iostream> and <iomanip> headers.

The whole call to charToHex could simply be replaced by std::cout << std::uppercase << std::hex << std::setfill('0') << std::setw(2) << static_cast<int>(c);.

Note the static_cast<int>: We want to print the numerical value, not the ASCII character glyph!

However, setting std::uppercase, std::hex and std::setfill('0') for every byte to be printed would be wasteful, plus it would overwrite any previously set flags on std::cout.

A better approach would be using a intermediary std::ostringstream instead, setting the flags once and retrieving (and resetting) its contents when sufficiently filled.

More on that in the section below.

Also, both functions end with return move(s);. This seems like misguided premature optimization ("Hey, we can move here! No need for a copy!"), but it likely is actually a pessimization instead!

Compilers actually know that the returned value is a temporary and can optimize for this case (usually called NRVO - "Named return value optimization"). But: They can only do this if the variable is returned directly, and the call to std::move prevents that.

Prefer return s;, unless you have good reasons (e.g. measurements/bad compiler) indicating otherwise.

PrintMBR

This function is a big pile of unnecessarily complicated code with poorly documented/enfored preconditions.

First off, let's take a look at the function signature:

static void PrintMBR(char buffer[MBR_SIZE])

The flaw is really subtle: buffer isn't actually restricted to arrays of size MBR_SIZE at all! It's basically just a slightly fancier markup of static void PrintMBR(char *buffer), and the compiler will actually treat it as such.

This allows PrintMBR to be called with char arrays of all possible sizes, including smaller than MBR_SIZE, or even just nullptr.

How to fix this?

Well, the first inclination might be to just limit the size of buffer by explicitly making it keep track of the size information, e.g. by using a reference to a C-style array char (&buffer)[MBR_SIZE] or a fancier std::array reference const std::array<char, MBR_SIZE> &buffer.

And that is a valid option if the only purpose was to print MBR contents.

But looking at it's actually implementation, it seems like the intention was to print the hex values in one big column next to a column containing to the ASCII characters (if printable) nicely aligned. This could be generalized for printing arbitrary char arrays by amending some of the assumptions made further down in the implementation.

Next, let's have a look at the function body:

size_t byteIndex = 0;
string lineSeparator;
string columnSeparator;

for (size_t i = 0; i < NUMBER_OF_LINES; i++) {
    cout << lineSeparator;
    lineSeparator = '\n';
    columnSeparator = "";

    for (size_t j = 0; j < BYTES_PER_LINE; j++) {
        string ch = charToHex(buffer[byteIndex++]);
        cout << columnSeparator << ch;
        columnSeparator = " ";
    }

    cout << ' ';

    for (size_t j = 0; j < BYTES_PER_LINE; j++) {
        char c = buffer[byteIndex - BYTES_PER_LINE + j];
        cout << (isprint((unsigned int) c) ? c : '.');
    }
}

If I understand correctly, the intention is to print the MBR in a XX XX XX XX xxxx line format (where X is a hex digit and x is an ASCII character).

First off, it's hard to keep track of "constants" if their values are constantly changing. With one more check and a tiny bit of reordering, we can fix this:

size_t byteIndex = 0;
static const string lineSeparator = "\n";
static const string columnSeparator = " ";

for (size_t i = 0; i < NUMBER_OF_LINES; i++) {
    if(i != 0) cout << lineSeparator;

    for (size_t j = 0; j < BYTES_PER_LINE; j++) {
        string ch = charToHex(buffer[byteIndex++]);
        cout << ch << columnSeparator;
    }

    for (size_t j = 0; j < BYTES_PER_LINE; j++) {
        char c = buffer[byteIndex - BYTES_PER_LINE + j];
        cout << (isprint((unsigned int) c) ? c : '.');
    }
}

A bit more readable, and I don't have to keep track of all the possible states (and weird reassignments).

I mentioned above that instead of using charToHex, one could use std::ostringstream and <iomanip> facilities instead:

void PrintMBR(char (&buffer)[MBR_SIZE]) {
    static constexpr auto bytes_per_line = 4;
    static const auto column_delimiter = " "s;
    static const auto line_delimiter = "\n"s;

    auto hex_part = std::ostringstream{};
    auto ascii_part = std::ostringstream{};
    auto counter = 0;

    hex_part << std::uppercase << std::hex << std::setfill('0');

    for(auto c : buffer)
    {
        hex_part << std::setw(2) << static_cast<int>(c) << column_delimiter;
        ascii_part << (isprint(static_cast<unsigned int>(c)) ? c : '.');

        ++counter;

        if(counter % bytes_per_line == 0)
        {
            std::cout << hex_part.str() << ascii_part.str() << line_delimiter;
            hex_part.str("");
            ascii_part.str("");
        }
    }
}

This could easily be made more generic to allow printing any byte buffer in this format.

ReadMBR

• Memory leak: buffer doesn't ever get used after being allocated, including being deleted.

• I don't like the return GetLastError(); bit. I guess it's fine in pure C, but in C++ there is a more common way to communicate error conditions: exceptions.

  I get this is kind of a glue layer between C WinAPI and C++, but that doesn't mean you have to port idioms from one side into the other, especially if there are more idiomatic alternatives.

  I see this as problematic in this case, as likely nobody checks return codes unless they have to. Case in point: main().

• To be more generic, I would really like for this method to accept an OutputIterator. But sadly, the C WinAPI doesn't know those, so that would require another copy of the data (using an intermediary buffer). That's one of the design trade-offs: Performance vs. Usability.

main

• No check on the return code of ReadMBR.
• return 3; - What does this 3 represent?

Answer 2

Idiomatic is relative. I guess it's a coding style you could find in older code-bases (and many winapi based applications are rather old), but you wouldn't write C++ like this today. Consider for instance:

#include <iostream>
#include <iomanip>
#include <iterator>
#include <algorithm>
#include <numeric>
#include <array>

constexpr std::size_t MBR_SIZE = 255;
constexpr std::size_t BYTES_PER_COLUMN = 4;
constexpr char LINE_DELIMITER = '\n';
constexpr const char* COL_DELIMITER = " ";

int main() {
    std::array<unsigned char, MBR_SIZE> mbr;
    std::iota(std::begin(mbr), std::end(mbr), 0); // filling it for testing purposes

    std::cout << std::hex << std::setfill('0');
    std::size_t index = 0;
    std::transform(std::begin(mbr), std::end(mbr), std::ostream_iterator<int>(std::cout, COL_DELIMITER), [index](auto c) mutable {
        if (index++ % BYTES_PER_COLUMN == 0) std::cout << LINE_DELIMITER;
        std::cout << std::setw(2); // setw must be applied for each output
        return c;
    });
    std::cout << std::endl;
}

In a dozen of lines you get the same result as printMBR and its two auxiliary functions (at least I believe so, I haven't had the chance to run your code), which are longer and more complicated.

To be more specific about your code:

• don't declare a variable before you're ready to define it, and don't define it before you're ready to use it.

• don't std::move your return value, it prevents copy elision

• don't declare free functions static, it's a java thing

• inside ReadMBR there's a char* buffer = new char[MBR_SIZE]; you don't use afterwards

• I'm not a fan of that many using directives; prefix the names with std:: and be done with it, unless you prefer to set up aliases: then using string = std::string is clearer

• use standard algorithms, such as std::copy, std::accumulate, etc (in headers <algorithm> and <numeric>) or range-base for loops (for auto item : sequence) instead of "raw" loops. For instance, charToHex could be written

like this:

std::string s;
for (auto nibble : { (c >> 4) & 0xf, c & 0xf }) 
    s += fourBitsToString(nibble);
return s;

or:

char lo = c & 0xf;
char hi = (c >> 4) & 0xf;
char chars[] = {hi, lo};

return std::accumulate(std::begin(chars), std::end(chars), std::string(), [](auto init, auto elem) {
    return init += (fourBitsToString(elem));
});

And, on a more aesthetic note, a four-bits aggregate can be called a half-byte or a nibble.

Answer 3

Here are some things that may help you improve your program.

Release resources as soon as practical

The diskHandle should be released immediately after the ReadFile call to minimize the time that the handle is open. Call CloseHandle to close the file and release the handle.

Use minimal sufficient privileges

In this case, the MBR is only read and not written, so the file should be opened only with the FILE_SHARE_READ option and not FILE_SHARE_WRTE.

Use const where practical

The PrintMBR function does not alter the passed buffer so the buffer parameter should be const.

Use an object

I'm surprised that other reviews didn't mention this, but why not treat the MBR as a C++ object? That way your main could look like this:

int main() {
    MBR mbr;
    if (mbr.read()) {
        std::cout << mbr << '\n';
    } else {
        std::cout << "Error reading MBR\n";
    }
}

Isolate platform-specific code

It would be very simple to write this code so that it also runs under Linux and doesn't rely on any particular compiler. The easiest way to do that is to isolate the Windows-specific code so that it is easy to port. This may not seem like a big deal if you're only interested in having a Windows version at the moment, but writing portable code by habit generally pays off in the long run in my experience.