I'm refreshing my C skills (been decades) and ran across something I don't quite understand. I'm working on some code that involves a lot of bit shifting, masking, etc. I have one function that creates a string with the binary representation of a number. I've found it's a lot more complicated than it needs to be so I've adjusted it, but here is the original code below.
Note: The caller is responsible for freeing memory, so I've taken care of that. This is running on Linux x64 in Eclipse C/C++ IDE.
char* get_bit_str(int num, int num_bits) {
// XXX: This no worky worky...
// When having multiple malloc'd string it eventually
// get garbage. Not too sure why...
num_bits = num_bits > 0 ? num_bits : sizeof(int);
char* buf = (char*)malloc(num_bits + 1);
if (!buf) {
printf("\nMemory allocation failed !\n");
return "";
}
// Start at least significant bit.
for (int i = num_bits - 1; i >= 0; i--) {
char* bit = (num & (1 << i)) ? "1" : "0";
// strncat(buf, bit, 1);
strcat(buf, bit);
}
// strncat(buf, "\0", 1);
// strcat(buf, "\n");
return buf;
}
This works fine most of the time, but for one specific example, I get garbage. I've tried multiple approaches like overwriting the same char* or assigning a new one, but the result is always same. I also tried:
char* buf = (char*)malloc((num_bits + 1) * sizeof(char));
I've used strcpy and strncpy as well, but they produce the same issue. Both strcpy and strncpy append a null terminator, which might be an issue. Normally, the output is ok when I:
Call the function
Print the result
Free the
char*pointer
However, if I:
Call the function with two different values (e.g., 1 and then 2)
Print out both
char*pointersFree both
char*pointers
I get garbage for the second one using this test code (Like I say, I've tried to assign new variables but here I'm reusing the same "c" and "d"):
void test_get_bit_str() {
char* c = get_bit_str(0, 4);
printf("c (0) %s\n", c);
char* d = get_bit_str(1, 4);
printf("d (1) %s\n", d);
free(c);
free(d);
c = get_bit_str(1, 4);
printf("c (1) %s\n", c);
d = get_bit_str(2, 4);
printf("d (2) %s\n", d);
free(c);
free(d);
}
The output is:
c (0) 0000
d (1) 0001
c (1) ���D�U0001
d (2) 0010
You can see on the second c is garbage. I printed the pointer addresses, and it is reusing the same memory addresses after I free everything, which is fine, I think.
I really don't understand why this works when malloc and free is done for individual invocations but when I have two pointers defined (malloc twice, print out results, free both pointers), it's doing this.
Now, having stated all of that, I've made it a lot less complex with the following, which does work:
char* get_bit_str(int num, int num_bits) {
num_bits = num_bits > 0 ? num_bits : sizeof(int);
// Allocate enough memory for the number of bits including room for NULL.
char* buf = (char*)malloc(num_bits + 1);
if (!buf) {
printf("\nMemory allocation failed !\n");
return NULL;
}
*buf = 0;
for (int i = 0; i < num_bits; i++) {
char bit = (num & (1 << i)) ? '1' : '0';;
// Here we want to but the least significant bit at the other end of
// the array. This is effectively swapping the array positions.
buf[num_bits - 1 - i] = bit;
}
// Remember to append the NULL char.
buf[num_bits] = '\0';
return buf;
}
Any help understanding what's going on would be greatly appreciated.
valgrindand similar tools.s/malloc/calloc-strcatscans its left operand until it finds a null terminator, andmallocreturns uninitialized memory, so it can easily overrun the buffer. (and you certainly don't want to use strcat here, do not turn a simple O(N) operation into O(n^2))char* buf = (char*)malloc(num_bits + 1);add*buf=0;buf[num_bits - 1 - i] = bit;instead of justbuf[i] = bit;I also did what someone suggested and after the malloc deference it and set it to zero:*buf = 0;char* buf = ...and then*buf = 0;and thenif (!buf) { ...because you are assigning to the contents of buf and then testing if buf has a value. If it didn't have a value the assignment of the contents would cause problems. You need to assign the contents AFTER checking if buf has a value.