Storing data in char array causing corruption around variable

Question

I am working on a C++ project and I am having an issue.

Below is my code

tempfingerprint = libssh2_hostkey_hash(session, LIBSSH2_HOSTKEY_TYPE_RSA);
    char temp[48];
    memset(temp, 0, sizeof(temp));
    for (i = 0; i < 16; i++)
    {
        //fingerprintstream << (unsigned char)tempfingerprint[i] << ":";
        if (temp[0] == 0)
        {
            sprintf(temp, "%02X:", (unsigned char)tempfingerprint[i]);
        }
        else
        {
            //sprintf(temp, "%s:%02X", temp, (unsigned char)tempfingerprint[i]);
            char characters[3];
            memset(characters, 0, sizeof(characters));
            //If less than 16, then add the colon (:) to the end otherwise don't bother as we're at the end of the fingerprint
            sprintf(characters, "%02X:", (unsigned char)tempfingerprint[i]);
            strcat(temp, characters);
        }
    }
    //Remove the end colon as its not needed. 48 Will already be null terminated, so the previous will contain the last colon
    temp[47] = 0;
    return string(temp);

When I run my app, I get the following error from visual studio

Run-Time-Check Failure #2 - Stack around the variable 'temp' was corrupted.

I've ran the same code on Linux through Valgrind and no errors were shown so I'm not sure what the problem is with Windows.

*I've ran the same code on Linux through Valgrind and no errors were shown so I'm not sure what the problem is with Windows.* -- It isn't a problem with Windows. Why are you using character arrays, and hoping that they're large enough to hold the data you're placing in them? Looks like you're writing `C` and not `C++`. — PaulMcKenzie, Jan 09 '17 at 19:00
Wouldn't `"%02X:"` produce more than 3 characters, if you count the terminating nul character. — Bo Persson, Jan 09 '17 at 19:02
Why would it be more, an int converted to unsigned char, i.e. int 99 will be converted to unsigned char in a hex value padded to 2 characters with a `:` on the end, e.g. 97: — Boardy, Jan 09 '17 at 19:05
@Boardy -- Why are you micromanaging the size of the array like that? If you're off by a single byte, you have a buffer overrun in the sprintf call. That entire routine could have been written using `std::string` without introducing any character arrays. — PaulMcKenzie, Jan 09 '17 at 19:06
`characters` will contain "97:\0", 4 characters. The '\0' on the end is the null terminator marking the end of the string. More here: [What is a null-terminated string?](http://stackoverflow.com/questions/2037209/what-is-a-null-terminated-string) — user4581301, Jan 09 '17 at 19:07
Why would it have \0 on the end. I tried adding 4 characters to the array size just in case but still get the same error — Boardy, Jan 09 '17 at 19:09
@Boardy -- *I tried adding 4 characters to the array size just in case but still get the same error* -- "Just in case"... Change to using `std::string` and `std::ostringstream`, and there isn't a need for "just in case", as the string will grow as you add characters to it. — PaulMcKenzie, Jan 09 '17 at 19:11
I did try using that originally, but didn't work, this way seemed a lot easier to use/understand. It was something about using stringstream (I assume is the same ostringstream) with hex conversion but it would end up printing rubbish — Boardy, Jan 09 '17 at 19:14
C-style character strings have an implicit NUL character at the end. So, `"97:"` is the four character, `'9'`, `'7'`, `':'` and `'\000'`. Similarly, `temp` has, at one point, 49 characters. If you aren't going to use `std::string`, try `char characters[4];` and `char temp[49];`. — Robᵩ, Jan 09 '17 at 19:35
@Boardy *I did try using that originally, but didn't work,* -- Probably you needed to post that as the original question then to start using character arrays and introduce buffer overruns. What is `tempfingerprint` declared as, and what would be a sample of what it contains? — PaulMcKenzie, Jan 09 '17 at 19:40

user4581301 · Accepted Answer · 2017-01-09T20:49:29.677

3

Here's an approach using on what Paul McKenzie's talking about (though he might implement it differently) based on it looks like you were trying to do with the stream

#include <iostream>
#include <sstream>
#include <iomanip> // output format modifiers
using namespace std;

int main()
{
    stringstream fingerprintstream;
    // set up the stream to print uppercase hex with 0 padding if required
    fingerprintstream << hex << uppercase << setfill('0');

    // print out the first value without a ':'
    fingerprintstream << setw(2) << 0;

    for (int i = 1; i < 16; i++) // starting at 1 because first has already been handled.
    {
        // print out the rest prepending the ':'
        fingerprintstream << ":" << setw(2) << i;
    }
    // print results
    std::cout << fingerprintstream.str();
    return 0;
}

Output:

00:01:02:03:04:05:06:07:08:09:0A:0B:0C:0D:0E:0F

Just realized what I think OP ran up against with the garbage output. When you output a number, << will use the appropriate conversion to get text, but if you output a character << prints the character. So fingerprintstream << (unsigned char)tempfingerprint[i]; takes the binary value at tempfingerprint[i] and, thanks to the cast, tries to render it as a character. Rather than "97", you will get (assuming ASCII) "a". A large amount of what you try to print will give nonsense characters.

Example: If I change

fingerprintstream << ":" << setw(2) << i;

to

fingerprintstream << ":" << setw(2) << (unsigned char)i;

the output becomes

0?:0?:0?:0?:0?:0?:0?:0?:0?:0?:0 :0
:0?:0?:0
:0?:0?

Note the tab and the line feeds.

I need to know the definition of tempfingerprint to be sure, but you can probably solve the garbage output problem by removing the cast.

Based on new information, tempfingerprint is const char *, so tempfingerprint[i] is a char and will be printed as a character.

We want a number, so we have to force the sucker to be an integer.

static_cast<unsigned int>(tempfingerprint[i]&0xFF)

the &0xFF masks out everything but the last byte, eliminating sign extension of negative numbers into huge positive numbers when displayed unsigned.

edited Jan 09 '17 at 20:49

answered Jan 09 '17 at 19:28

user4581301

33,082
7
33
54

Thanks, tempfingerprint is a `const char *`. I tried removing the cast so it was just `fingerprintstream << std::setw(2) << tempfingerprint[i];`but I still get gibberish – Boardy Jan 09 '17 at 19:41
@Boardy Give us a sample of `fingerprintstream` data, as "gibberish" doesn't convey any information. – PaulMcKenzie Jan 09 '17 at 19:44
The gibberish is `00e0~0 0®0x0/0┌0ý0h0ñ0` I'm expecting `11:65:7E:0A:94:55:88:77:EC:68:A4:0D:A9` as an example (I've truncated the full output I'm expecting). The output isn't quite the same, as it has some special characters as well so I can post a screenshot instead if you wish – Boardy Jan 09 '17 at 19:48
If `tempfingerprint` is `const char *` then `tempfingerprint[i]` is a `char` and we have to force the sucker to be an integer. `(unsigned int) (tempfingerprint[i]&0xFF)` the `&0xFF` masks out everything but the last byte, eliminating sign extension. – user4581301 Jan 09 '17 at 19:50
Ah that's done it, I don't really understand though what's the `&0xFF` is everything, quite new to C++ so haven't fully got to grips with everything yet. – Boardy Jan 09 '17 at 19:53
`&0xFF` is a binary AND with one byte's worth of 1s. example: 128 (0x80 as a character is a negative number. It will extend to 0xFFFFFF80 if converted to an `int`. We don't want the extra nonsense, so we mask it out 0xFFFFFF80 AND 0x000000FF = 0x80. – user4581301 Jan 09 '17 at 20:01
2

This shenanigans is necessary because `libssh2_hostkey_hash` is badly designed, it should not return pointer to signed characters when the data is unsigned – M.M Jan 09 '17 at 20:03
What do you think of something like `static_cast( static_cast(x) )`? While it's ugly, maybe can better express the intention. – Bob__ Jan 09 '17 at 20:16
Ah thanks, that makes sense, I tried converting to `unsigned` from another example on SO before your comment, and I was wondering why I was getting something `FFFFF11:FFFFFF65:` – Boardy Jan 09 '17 at 20:40
@Bob__ I gave that double cast some thought, but went back to the old school approach out of fear of `char`s of unusual size. Damn things pop up when you least expect them. That said, I should have used the C++ style casts in the answer. Will fix when I get time and remember. – user4581301 Jan 09 '17 at 20:45

score 0 · Answer 2 · answered Jan 09 '17 at 20:43

There are, as far as I see, two issues in the code which lead to exceeding array boundaries:

First, with char temp[48] you reserve exactly 48 characters for storing results; However, when calling strcat(temp, characters) with the 16th value, and characters comprises at least the characters including the colon, then temp will comprise 16*3 digits/colons + one terminating '\0'-character, i.e. 49 characters (not 48). Note that strcat automatically appends a string terminating char.

Second, you define char characters[3] such that you reserve place for two digits and the colon, but not for the terminating '\0'-character. Hence, an sprintf(characters, "%02X:",...) will exceed characterss array bounds, as sprintf also appends the string terminator.

So, if you do not want to rewrite your code in general, changing your definitions to char temp[49] and char characters[4] will solve the problem.

Storing data in char array causing corruption around variable

2 Answers2