Thread: Escaping non-printable characters in strings

Code:

strncat(result, &tempval, sizeof(tempval));

That's very wrong.

strncat is for strings. You should not take the address of a mundane character and expect it to work because the source should be zero terminated.

If value[i] is printable and part of the result, you should do

Code:

*result++ = tempval;

Unless you want to make tempval a zero terminated string. If you do make tempval a string, then the last argument to strncat will be the length of the appended string, just as it always is. sizeof is almost never the right thing, it definitely isn't here, and if it happens to be right some other occasion, it is a coincidence.

As for the other time, when tempval is not printable I have to wonder why you need to do this so often:

Code:

sz = snprintf(NULL, 0, "#%02X", tempval);
buf = malloc(sizeof(tempval) * 4);

You should do this exactly once, not once per every instance of non-printable things. And do it right:

Code:

sz = strlen(value);
buf = malloc(sz * 4 + 1);

Assuming malloc succeeds in returning buf, then you have a string 4 times the size of the value argument, plus 1 for the zero character.

Code:

char *result = (char*)malloc(sizeof(char) * strlen(value));

There is not enough space here to hold your string. First, if you were just copying a string, you'd need strlen(value)+1 bytes; the 1 is required because strings must be terminated by a null character, which strlen() does not count. Next, you're expanding the string. \x01 is a single byte, but you're replacing it with #01, which is three bytes. Your target buffer, worst case, needs to be three times the size of the original buffer (strlen()*3 + 1, to be exact).

Code:

strncat(result, &tempval, sizeof(tempval));

strncat() requires a string as the first argument. A string means there must be a null character. This is required because, otherwise, strncat() would not know where to start appending the new string. When you call malloc(), the contents of the memory are unspecified; they need not be zeroes, which means you can't use it as the target of a strncat(). You can fix this easily enough by setting the first byte of “result” to zero.

Code:

sz = snprintf(NULL, 0, "#%02X", tempval); 
buf = malloc(sizeof(tempval) * 4);
snprintf(buf, sz+1, "#%02X", tempval);

If you're going to use snprintf() to determine how much space is required to store the resulting string, you ought to pass that value to malloc().
As for why you're getting FFFFFFF: %X expects an unsigned int. \xfe, if chars are signed, is very likely going to have the value -2. When this is converted to unsigned int, you get a very large number. You should cast tempval to an unsigned char, forcing it into the range 0 to 255 (assuming an 8-bit char). This result, in all reality, should probably then be converted to an unsigned int, but in practice it will work fine as-is.

Code:

strncat(result, buf, strlen(buf)-1);

Because strlen() doesn't count the null character, you're chopping off the last (non-null) byte of your string. Don't subtract 1 here. There's no need for strncat(), either. Just use strcat().

Code:

char *buffer = (char*)malloc(sizeof(char) * strlen(result));
strncat(buffer, result, strlen(result));
free(result);
printf("Final buffer contents: %s\n", buffer);
return buffer;

Again, strncat() requires the target to be a string. But you don't want strncat() here. You want strcpy().

However, I'm not sure why you're copying the string into a new buffer. You could just return “result”; or if you're trying to save space by returning a buffer of exactly the proper size, you can just use realloc() to re-size “result”.

Code:

assert(escape_string(testvalue) == testescape);

You cannot compare strings with ==. Use strcmp().