Format raw bytes in hex and ascii in manner like Wireshark does

Question

This function formats raw buffer output in manner as Wireshark and many others does:

0x00: 68 65 6C 6C 6F 20 77 6F 72 6C 64 02 6B 68 67 61   |hello world.khga|
0x10: 76 73 64 20 0B 20 0A 05 58 61 73 6A 68 6C 61 73   |vsd . ..Xasjhlas|
0x20: 62 64 61 73 20 6A 61 6C 73 6A 64 6E 13 20 20 30   |bdas jalsjdn.  0|
0x30: 31 32 33 34 35 36 37 38 39                        |123456789       |

Please review and assess code quality.

std::string format_hex_payload(const char* payload, size_t payload_len)
{
    const char line_placeholder[] = "0x00:                                                   |                |";
    size_t number_of_lines = payload_len / 16 + (payload_len%16 > 0);
    size_t sizeof_output = sizeof line_placeholder * number_of_lines;
    char *output = (char*)alloca(sizeof_output);  //POSSIBLE REIMPL USE C++RVO std::string out; out.reserve(sizeof_output);

    char *pout = output;  //POSSIBLE REIMPL char *pout = out.data();
    const char *p = payload;
    const char *const end = payload + payload_len;
    size_t ascii_offset = strchr(line_placeholder,'|') - line_placeholder + 1;  //could be calculated at compile time

    unsigned short offset = 0;
    for(unsigned l=0; l < number_of_lines; l++, offset+=16)
    {
        char* pline_begin = pout;
        char* pline = pout;
        strcpy(pline,line_placeholder);
        pline += sprintf(pline, "0x%02X: ", offset);
        for(unsigned i=0; i<16 && p < end; ++i, ++p){
            pline += sprintf(pline, "%02X ", *p);
            *(pline_begin+ascii_offset+i) = isprint(*p) ? *p : '.';
        }
        *pline=' ';
        pout += sizeof line_placeholder;  // move pointer to next line
        pout[-1] = '\n';
    }
    pout[-1] = '\0';
    assert(pout == output + sizeof_output);  // sanity check

    return output;  //POSSIBLE REIMPL return out;
}

Answer 1

1. I think probably the first thing to do is decide whether you're really using C, or really using C++.

   • If you're really using C, then you need to get rid of the std::string, and allocate space for your return differently (e.g., using malloc).

   • If you're really using C++, then I'd at least consider using iostreams and manipulators to do most of the work. They are fairly verbose, but most people using C++ expect to deal with iostreams rather than raw buffers and C-style string manipulation.

2. I think I'd break the code up into a few more functions. For example, I'd probably have format_hex, which would repeatedly call format_line, which might in turn call some format_hex_value (or something on that order) to write out each individual value.

3. I'd probably avoid using the line placeholder, for a couple of reasons. The first (and probably most important) is that as you've done things right now, the last line is (as I see things) somewhat malformed. You've surrounded the ASCII part of the display with | as delimiters, but for that to make sense, you want the delimiter right next to the data, so that part of the last line should look like: |123456789|, so you immediately know there's no more data after the 9 (where your current display looks like there are space characters after the 9).

4. I'd definitely expand the addresses on the left out to at least 4 characters, and maybe more. In theory, you should probably be thinking in terms of 16 characters (allowing 64-bit addresses), but in all honesty I've rarely seen much point in hex dump of multiple gigabytes of data (but I certainly have seen good uses for more than 256 bytes).

Putting those together, one possibility (going the C++ route) might come out something on this order:

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

struct hex {
    unsigned value;
    int digits;
public:
    hex(unsigned value, int digits) : value(value), digits(digits) { }

    friend std::ostream &operator<<(std::ostream &os, hex const &h) {
        auto oldfill = os.fill('0');

        // Yeah, iostreams get really verbose. Sorry.
        os << std::hex 
           << std::setw(h.digits) 
           << std::uppercase 
           << std::setprecision(h.digits)
           << h.value;
        os.fill(oldfill);
        return os;
    }
};

void fmt_line(std::ostream &os, char const *data, size_t offset, size_t len) { 
    os << hex(offset, 8) << ":";

    for (size_t i=0; i<len; i++) {
        os << " " << hex(data[offset+i], 2);
    }

    os << std::setw((16-len)*3 +2) << "|";
    for (size_t i=0; i<len; i++) {
        char ch = data[offset+i];
        os << (std::isprint((unsigned char)ch) ? ch : '.');
    }
    os << "|\n";
}

std::string format_hex(char const *data, size_t len) { 
    unsigned lines = len/16;
    std::stringstream out;

    for (size_t line=0; line<lines; line++)
        fmt_line(out, data, line*16, 16);
    fmt_line(out, data, lines*16, len%16);
    return out.str();
}

int main() { 
    char input[] = "hello world\02khgavsd \xb \xa\x5Xasjhlasbdas jalsjdn\xd  0123456789";
    std::cout << format_hex(input, sizeof(input));
}

This could use some further improvement. For one thing, it's probably not immediately obvious what some things like: os << std::setw((16-len)*3 +2) << "|"; are really doing, so they could undoubtedly use at least a little more work.