Character pointer adding random values while attaching string characters

Question

I am using following code to print md5 and creating a string

char *hash = (char*)malloc(32 * sizeof(char));
unsigned *d = md5(msg, strlen(msg));
MD5union u;
printf("\n\n\nThe MD5 code for input string is : \n");
for (j=0;j<4; j++){
    u.w = d[j];
    for (k=0;k<4;k++) 
    {
        char *mVal = (char*)malloc(sizeof(char));
        sprintf(mVal, "%02x",u.b[k]);
        strcat(hash, mVal);
        printf("%02x",u.b[k]);
    }
}

printf("\n\n\nThe MD5 code for input string is :%s \n", hash);

My output is

The MD5 code for input string is : 
187ef4436122d1cc2f40dc2b92f0eba0


The MD5 code for input string is :p��187ef4436122d1cc2f40dc2b92f0eba0

Why is there p�� extra in my hash value

`mVal` is only one byte in size, but you are trying to sprintf several characters to it, hence you have UB. Make it at least 3 bytes if you need it to hold two hex chars plus a 0 terminator, — Paul R, Feb 23 '19 at 07:20

KamilCuk · Accepted Answer · 2019-02-23T07:39:51.473

hash is not initialized. It can have any initial value. strcat appends the data. If there is something in hash that qualifies as null terminated string, it will not be removed. Use strcpy or zero-initialize hash before use.

Except for that, there are multiple errors with your code and undefined behaviors.

malloc(32 * sizeof(char)); allocates memory for 31 (not 32) characters length string when including null termination character. If you want to store md5 there as a string, allocate 33 bytes of memory.
char *mVal = (char*)malloc(sizeof(char)); sprintf(mVal, "%02x",u.b[k]); - there is place for 0 (not 1, not 2, but zero) length string in the memory pointed to mVal. Is you wan to store the output of sprintf, "%02x" to a variable, allocate at least 3 bytes of memory - first byte for the first hex digit, second byte for the second hex digit and the third byte for null termination character.
char *mVal = (char*)malloc(sizeof(char)); is nowhere freed before it runs out of scope and just leaks memory.
Looks like you are using md5hash.c. The function returns a pointer to a static memory. This is bad, as it makes the function not reentrant.
Your program could be rewritten as just: char *hash = malloc(33); unsigned *d = md5(msg, strlen(msg)); snprintf(hash, 33, "%08x%08x%08x%08x", d[0], d[1], d[2], d[3]);. There is little need for converting unsigned int to unsigned char when printing hex, you can to that directly with %08x. And unsigned char is converted back to int when passing as printf argument. And you can just sprintf directly into hash, no need for temporary variables.
Using a union to convert between unsigned int array to bytes feels like a misuse of the tool. (On most systems) you can just cast the pointer char *d_as_char = (void*)d; and access d_as_char as 4 * sizeof(int) length char array. If you really want to not have undefined behavior and aliasing problems in your code, use the memcpy way: char d_as_array[sizeof(int) * 4]; memcpy(d_as_array, d, sizeof(d_as_array));

That's one of those "where do I start?" questions... Well done. — David C. Rankin, Feb 23 '19 at 07:39
Missed out that in C `malloc` should not be casted. Just too lazy to look up the reference. I leave that to the reader. — Ed Heal, Feb 23 '19 at 07:40
https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc - Not that lay aferall — Ed Heal, Feb 23 '19 at 07:41
@kamil your 5th doesn't seem to be working it seems, :43f47e18ccd122612bdc402fa0ebf092 has numbers mirrored, — Muhammad Umar, Feb 23 '19 at 07:51
@muhammadumar Och right, endianess. You could just call a conversion function, like from [bswap.h](http://man7.org/linux/man-pages/man3/bswap.3.html) on unix, or just type-pune to char pointer and make one long `sprintf` call. — KamilCuk, Feb 23 '19 at 07:54

Character pointer adding random values while attaching string characters

1 Answers1