Ggtkuk

Bug

As Carsten points out, you need to allocate $(text{length}cdot 2)+1$ bytes, rather than $(text{length}cdot2)$ to account for the null terminator sprintf() adds.

Formatting

Most C formatting guides do not include spaces around the arguments to function calls, nor the expressions within an if-statement. For an example of a C style most C programmers would find acceptable, see OpenBSD's style(9) manual.

I choose to associate * with the variable name, rather than floating between the type and name. This disambiguates the following example:

int *a, b;

Here, a is a pointer to an integer, but b is only an integer. By moving the asterisk next to the name, it makes this clearer.

int length = strlen ( str );

char * newStr = malloc (length * 2 );

if ( !newStr) shutDown ( "can't allocate memory" ) ;

Becomes:

int const len = strlen(str);

char *const new_str = malloc(1 + len * 2);



if (new_str == NULL) {

    shutDown("can't allocate memory");

}

Error checking

Rather than calling shutDown() and exit()ing the program, you should instead return an error value which can be checked by the caller of str_to_hex(). Because you return a pointer, you can return NULL to indicate an error occurred and the caller should check errno.

Likewise, on some systems your program can incorrectly exit when length == 0. If we look at the manual page for malloc(3):

Return Value

The malloc() and calloc() functions return a pointer to the allocated memory that is suitably aligned for any kind of variable. On error, these functions return NULL. NULL may also be returned by a successful call to malloc() with a size of zero, or by a successful call to calloc() with nmemb or size equal to zero.

So by returning NULL we account for the case where malloc(3) returns NULL on success.

if (new_str == NULL) {

    shutDown("can't alloc memory");

}

Becomes:

if (new_str == NULL) {

    return NULL;

}

If you choose, you can also check if str is NULL before calling strlen(). This is up to you, and it's not uncommon in C to ignore this case and leave it as user error.

Looping

Use the size_t type in your loop rather than int. size_t is guaranteed be wide enough to hold any array index, while int is not.

Using i rather than x is more common for looping variables.

The y variable isn't needed. You can simply use str[i] in its place.

In terms of performance there's likely a faster option than using sprintf(). You should look into strtol(3).

Conclusion

Here is the code I ended up with:

#include <stdio.h>

#include <stdlib.h>

#include <string.h>



char *

str_to_hex(char const *const str)

{

    size_t const len = strlen(str);



    char *const new_str = malloc(1 + len * 2);



    if (new_str == NULL) {

        return NULL;

    }



    for (size_t i = 0; i < len; ++i) {

        sprintf(new_str + i * 2, "%02X", str[i]);

    }



    return new_str;

}



int

main(void)

{

    char *str = "abz";

    char *hex = str_to_hex(str);



    if (hex == NULL && strlen(str) != 0) {

        /* error ... */

    }



    printf("%sn",hex);



    free(hex);

}

Hope this helps!

In my opinion, the most severe problem is "Insufficient target memory".

int length = strlen ( str );

char * newStr = malloc( length  * 2 );

You are allocating twice the length of str, which is enough for all the hex characters (two hex chars per input byte).

But sprintf works different: "A terminating null character is automatically appended after the content" (see here).

So the last call to sprintf will write a terminating zero byte right after newStr, into unallocated memory. This might provoke all kinds of unintended behaviour, including (but not limited to) crashes.

Just one addition: like asprintf vs snprintf. One can effectively predict the size, so I would think it natural to have a string buffer and the size passed in instead of creating it dynamically.

#include <stdlib.h> /* strtol */

#include <string.h> /* strlen */

#include <stdio.h>  /* printf */

#include <assert.h> /* assert */



/** Converts {str} to the underlying bit representation in hex, stored in

 {hex}. It may fail to compute the entire string due to {hex_size}, in which

 case the return will be less then the {str} length.

 str: A valid null-terminated string.

 hex: The output string.

 hex_size: The output string's size.

 return: The number of characters from the original that it processed. */

static size_t strToHex(const char *str, char *hex, size_t hex_size)

{

    static const char digits[0x0F] = { '0', '1', '2', '3', '4', '5',

        '6', '7', '8', '9', 'A', 'B', 'C', 'E', 'F' };

    const size_t str_len = strlen(str), hex_len = hex_size - 1;

    const size_t length = str_len < hex_len / 2 ? str_len : hex_len / 2;

    const char *s = str;

    char *h = hex;

    size_t x;

    assert(str && hex);

    if(!hex_size) return 0;

    for(x = 0; x < length; x++)

        *h++ = digits[(*s & 0xF0) >> 4], *h++ = digits[*s++ & 0x0F];

    *h = '';

    return s - str;

}



int main(void)

{

    const char *str = "abcdefghijklmnopqrstuvwxyz", *str2 = "æôƌԹظⓐa";

    char hex[80];

    size_t ret;

    ret = strToHex(str, hex, sizeof hex);

    printf(""%s" -> "%s" (%lu.)n", str, hex, (unsigned long)ret);

    ret = strToHex(str, hex, sizeof hex / 2);

    printf(""%s" -> "%s" (%lu.)n", str, hex, (unsigned long)ret);

    ret = strToHex(str, hex, 0);

    printf(""%s" -> "%s" (%lu.)n", str, hex, (unsigned long)ret);

    ret = strToHex(str2, hex, sizeof hex);

    printf(""%s" -> "%s" (%lu.)n", str2, hex, (unsigned long)ret);

    return EXIT_SUCCESS;

}

It cannot really fail if given the proper input, so this simplifies error checking a lot, especially in C. malloc and sprintf are pretty slow functions, comparatively, so I expect this to be faster and more robust.

I did more tests on the function today and found another Bug (shame on me), and AFAIK on code review I can't change the original code in the question since it got reviews.

if there are bytes have values more than 127 it will be all displayed as FF by the function. To reproduce

char str = {127,0};

char * hex = strToHex(str);

printf("%sn",hex); //pritnts 7F (NORMAL)



//now try with this

char str = {128,0};

char * hex = strToHex(str);

printf("%sn",hex); //pritnts FF (BUG)

It appears if the function is used with non English characters because they are stored with the most significant bit is set 1 in UTF-8

The Fix

To Fix it, replace this line

sprintf ( newStr + x * 2, "%02X", y );

with this

sprintf ( newStr + x * 2, "%02hhX", y ); // added hh

This is because y is of type char or signed char and the X specifier expects the argument to be unsigned int if no length is provided, so we provided length hh to tell the function that X is unsigned char . Check the length table of printf.

If we didn't provided hh, the sprintf function is going to promote Y from signed char to unsigned int and this promotion will go like this

when we defined the str as char and assigned the value 128 to it, it's represented as

1000 0000

The compiler thought it is -128 because it's type is signed char, now function sprintf wants to promote it to unsigned int, so to represent -128 in size of int, it will be like

1111 1111  1111 1111  1111 1111  1000 0000

^^^^ ^^^^  ^^^^ ^^^^

and because we chose to show only 2 digits then we see the last 2 bytes FF.

more info are here , and here