20
\$\begingroup\$

I have developed some free standing functions to provide hex dumps for buffer (std::string) contents.

You can see it working in action here.

The purpose is for debugging diagnostics to print buffer contents to a (logging) stream, in a format similar as wireshark does.

Any proposals for improvements are welcome.

Here's the code:

#include <iostream>
#include <iomanip>
#include <cctype>

std::ostream& render_printable_chars(std::ostream& os, const char* buffer, size_t bufsize) {
    os << " | ";
    for (size_t i = 0; i < bufsize; ++i)
    {
        if (std::isprint(buffer[i]))
        {
            os << buffer[i];
        }
        else
        {
            os << ".";
        }
    }
    return os;
}

std::ostream& hex_dump(std::ostream& os, const uint8_t* buffer, size_t bufsize, bool showPrintableChars = true)
{
    auto oldFormat = os.flags();
    auto oldFillChar = os.fill();

    os << std::hex;
    os.fill('0');
    bool printBlank = false;
    size_t i = 0;
    for (; i < bufsize; ++i)
    {
        if (i % 8 == 0)
        {
            if (i != 0 && showPrintableChars)
            {
                render_printable_chars(os, reinterpret_cast<const char*>(&buffer[i] - 8), 8);
            }
            os << std::endl;
            printBlank = false;
        }
        if (printBlank)
        {
            os << ' ';
        }
        os << std::setw(2) << std::right << unsigned(buffer[i]);
        if (!printBlank)
        {
            printBlank = true;
        }
    }
    if (i % 8 != 0 && showPrintableChars)
    {
        for (size_t j = 0; j < 8 - (i % 8); ++j)
        {
            os << "   ";
        }
        render_printable_chars(os, reinterpret_cast<const char*>(&buffer[i] - (i % 8)), (i % 8));
    }

    os << std::endl;

    os.fill(oldFillChar);
    os.flags(oldFormat);

    return os;
}

std::ostream& hex_dump(std::ostream& os, const std::string& buffer, bool showPrintableChars = true)
{
    return hex_dump(os,reinterpret_cast<const uint8_t*>(buffer.data()), buffer.length(),showPrintableChars);
}

Here's the test case:

int main()
{
    const char test[] = "abcdef123456\0zyxwvu987654";
    std::string s(test,sizeof(test));

    hex_dump(std::cout, s);

    return 0;
}

The output looks like expected:

61 62 63 64 65 66 31 32 | abcdef12
33 34 35 36 00 7a 79 78 | 3456.zyx
77 76 75 39 38 37 36 35 | wvu98765
34 00                   | 4.
\$\endgroup\$
14
\$\begingroup\$
  1. Did you ever test it with buffers which are multiples of 8 long?
    Because the last block won't get the nice printable display.

  2. Are you sure the newline at the beginning of the output should be there?

  3. Should the test-output really contain the implicit 0-terminator?

  4. return 0; is implicit for main().

Modified test-function:

int main()
{
    const char test[] = "abcdef123456\0zyxwvu987654";
    std::string s(test, sizeof(test) - 1);

    std::cout << "---\n";
    hex_dump(std::cout, s);
    std::cout << "---\n";
    hex_dump(std::cout, "");
    std::cout << "---\n";
    hex_dump(std::cout, "12345678");
    std::cout << "---\n";
    hex_dump(std::cout, "1234567812345678");
    std::cout << "---\n";
}
  1. Do you have the opening brace for functions on its own line or not? Seems 2+1 for, 1 against...

  2. If you only want to output a single character, using a character is potentially more efficient than a string.

  3. The conditional operator is excellent for choosing between two values.

  4. Beware of the implementation-defined signedness of plain char. The character-classification-functions inherited from C expect the value of an unsigned char or EOF.
    See std::isprint.

  5. Consider marking internal functions static to avoid externally visible symbols and promote inlining.

  6. If you don't actually ever use a return-value, why provide one?

static void render_printable_chars(std::ostream& os, const char* buffer, size_t bufsize)
{
    os << " | ";
    for (size_t i = 0; i < bufsize; ++i)
        os << (std::isprint((unsigned char)buffer[i]) ? buffer[i] : '.');
}
  1. Don't play around with a useless flag just because. Tracing data-flow is quite a lot more involved than following the program-flow.

  2. Testing whether a variable already has a value before setting it to the same is rarely anything but obfuscation and pessimisation.

  3. Don't use std::endl unless you really want to flush. And if that's your intention, consider being more explicit by using std::flush.

  4. If you use C++17, you might want to accept a std::string_view by value instead of a std::string by constant reference. It's generally more efficient.

\$\endgroup\$
5
\$\begingroup\$

Your code will invoke undefined behavior when you feed it some character outside the basic execution character set, e.g. UTF-8 text. When your compiler defines char to have the same range as signed char, these characters are represented as negative numbers. Passing a negative number to the functions from <cctype> is only allowed for the special value EOF.

You should fix that bug, but don't worry that you didn't know about it before. I think less than 5% of the C programmers know this, and it is too easy to get wrong.

To fix this bug, call std::isprint(uint8_t(buffer[i])).

\$\endgroup\$
5
\$\begingroup\$

A proposal for improvement and an associated possible enhancement:

The constant 8 appears in many places as the number of characters to be output on each line. It could do with a meaningful name, so its intent is clear and it can be changed with a single edit.

The enhancement opportunity should now be obvious - allow chars_per_line to be specified by the caller somehow.

\$\endgroup\$
5
\$\begingroup\$

I see some things that may help you improve your code.

Use a void * for generic arguments

As I'm sure you know, it's somewhat unusual in modern C++ to use a void *, but this is one of those situations in which it's helpful because it removes the need for awkward casts on the part of the caller. I'd change the signature of the function to this:

std::ostream& hex_dump(std::ostream& os, const void *buffer, std::size_t bufsize, bool showPrintableChars = true)

Make sure not to invoke undefined behavior

As noted in the comments already, undefined behavior can arise if the the value passed to std::isprint cannot be represented as an unsigned char and does not have the value of EOF. We can avoid this by making a convenience cast within the function if, as per the previous point, we've passed in a const void *:

const unsigned char *buf{reinterpret_cast<const unsigned char *>(buffer)};

Check for nullptr when dealing with raw pointers

I'd suggest that for safety, any pointers passed into a function should be tested for nullptr before they're dereferenced. In this case, I'd suggest the first few lines of the function could be this:

if (buffer == nullptr) {
    return os;
}

Eliminate "magic numbers"

If I wanted the line length to be 16 instead of 8, I'd have to work hard to change each instance of 8 in the code and make sure only the relevant points were changed. Instead, I'd advocate using a named constant like this:

constexpr std::size_t maxline{8};

Avoid passing over data twice

There's not really a need to pass over the data twice. As each character is read, it can be processed in both hex and printable form. To assist with this, I'd suggest creating a small local buffer for the printable version since we know it's only as long as maxline plus one for a terminating NUL char:

char renderString[maxline+1];

Use existing variables where appropriate

The bufsize variable already contains the size of the array, so there's really not much need to introduce another variable i to track it. Because it's passed by value, we essentially have a local copy that we can use directly in the loop:

for (std::size_t linecount=std::min(maxline, bufsize) ;bufsize; --bufsize, ++buf) {

Results

Using all of these suggestions, we get a simpler, smaller, safer interface in a single function. Here is the alternative version:

std::ostream& hex_dump(std::ostream& os, const void *buffer, 
                       std::size_t bufsize, bool showPrintableChars = true)
{
    if (buffer == nullptr) {
        return os;
    }
    auto oldFormat = os.flags();
    auto oldFillChar = os.fill();
    constexpr std::size_t maxline{8};
    // create a place to store text version of string
    char renderString[maxline+1];
    char *rsptr{renderString};
    // convenience cast
    const unsigned char *buf{reinterpret_cast<const unsigned char *>(buffer)};

    for (std::size_t linecount=maxline; bufsize; --bufsize, ++buf) {
        os << std::setw(2) << std::setfill('0') << std::hex 
           << static_cast<unsigned>(*buf) << ' ';
        *rsptr++ = std::isprint(*buf) ? *buf : '.';
        if (--linecount == 0) {
            *rsptr++ = '\0';  // terminate string
            if (showPrintableChars) {
                os << " | " << renderString;
            } 
            os << '\n';
            rsptr = renderString;
            linecount = std::min(maxline, bufsize);
        }
    }
    // emit newline if we haven't already
    if (rsptr != renderString) {
        if (showPrintableChars) {
            for (*rsptr++ = '\0'; rsptr != &renderString[maxline+1]; ++rsptr) {
                 os << "   ";
            }
            os << " | " << renderString;
        }
        os << '\n';
    }

    os.fill(oldFillChar);
    os.flags(oldFormat);
    return os;
}

Example use:

int main()
{
    const char test[] = "abcdef123456\0zyxwvu987654Edward";
    const std::string s(test,sizeof(test));
    const std::wstring s2{L"A wide string."};
    const double not_really_pi{22.0/7};

    std::cout << "\nbasic string:\n";
    hex_dump(std::cout, s.data(), s.length()*sizeof(s.front()));
    std::cout << "\nwide string:\n";
    hex_dump(std::cout, s2.data(), s2.length()*sizeof(s2.front()));
    std::cout << "\na double\n";
    hex_dump(std::cout, &not_really_pi, sizeof(not_really_pi));
    std::cout << '\n';
}

Example output:

basic string:
61 62 63 64 65 66 31 32  | abcdef12
33 34 35 36 00 7a 79 78  | 3456.zyx
77 76 75 39 38 37 36 35  | wvu98765
34 45 64 77 61 72 64 00  | 4Edward.

wide string:
41 00 00 00 20 00 00 00  | A... ...
77 00 00 00 69 00 00 00  | w...i...
64 00 00 00 65 00 00 00  | d...e...
20 00 00 00 73 00 00 00  |  ...s...
74 00 00 00 72 00 00 00  | t...r...
69 00 00 00 6e 00 00 00  | i...n...
67 00 00 00 2e 00 00 00  | g.......

a double
49 92 24 49 92 24 09 40  | I.$I.$.@

Further enhancements

It would be nice to be able to use this functionality like this:

int main()
{
    const char test[] = "abcdef123456\0zyxwvu987654Edward";
    const std::string s(test,sizeof(test));
    const std::wstring s2{L"A wide stringy."};
    const double not_really_pi{22.0/7};

    std::cout << "\nbasic string:\n" << hexDump(s.data(), s.length()*sizeof(s.front())) 
              << "\nwide string:\n" << hexDump(s2.data(), s2.length()*sizeof(s2.front()))
              << "\nraw char array:\n" << hexDump(test, sizeof(test))
              << "\na double\n" << hexDump(&not_really_pi, sizeof(not_really_pi)) << '\n';

}

This can be effected by just a few additional lines of code:

struct hexDump {
    const void *buffer;
    std::size_t bufsize;
    hexDump(const void *buf, std::size_t bufsz) : buffer{buf}, bufsize{bufsz} {}
    friend std::ostream &operator<<(std::ostream &out, const hexDump &hd) {
        return hex_dump(out, hd.buffer, hd.bufsize, true);
    }
};
\$\endgroup\$
  • \$\begingroup\$ THX for the wide string example. I intended to expand that later for this purpose. \$\endgroup\$ – πάντα ῥεῖ Jun 7 '17 at 13:29
  • \$\begingroup\$ You are missing test-cases with buffers which aren't multiple of 8 long... \$\endgroup\$ – Deduplicator Jun 7 '17 at 18:49
  • \$\begingroup\$ They're not shown, but in my own version of the code, I used many more test cases, including zero-length buffer, nullptr pointers, and also different values for the line length. \$\endgroup\$ – Edward Jun 7 '17 at 18:55
  • \$\begingroup\$ Ha! I see that although I tested thoroughly, I failed to post the updated version of the code! Fixed now -- thanks! \$\endgroup\$ – Edward Jun 7 '17 at 19:03
4
\$\begingroup\$

One improvement I could think of, is to print the buffer addresses before the values dumped:

if (i % 8 == 0)
{
    if (i != 0 && showPrintableChars)
    {
        render_printable_chars(os, reinterpret_cast<const char*>(&buffer[i] - 8), 8);
    }
    os << std::endl;
    printBlank = false;
    os << (void*)&buffer[i] << ": ";
}

See here.

The output looks like this then:

0x209ec20: 61 62 63 64 65 66 31 32 | abcdef12
0x209ec28: 33 34 35 36 00 7a 79 78 | 3456.zyx
0x209ec30: 77 76 75 39 38 37 36 35 | wvu98765
0x209ec38: 34 00                   | 4.
\$\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.