Function is returning a different value every time?

Question

I'm trying to convert a hexadecimal INT to a char so I could convert it into a binary to count the number of ones in it. Here's my function to convert it into char:

#include <stdio.h>
#include <stdlib.h>
#define shift(a) a=a<<5
#define parity_even(a) a = a+0x11
#define add_msb(a) a = a + 8000

void count_ones(int hex){
    char *s = malloc(2);    
    sprintf(s, "0x%x", hex);
    free(s);
    printf("%x", s);
};

int main() {
    int a = 0x01B9;
    shift(a);
    parity_even(a);
    count_ones(a);
    return 0;
}

Every time I run this, i always get different outputs but the first three hex number are always the same. Example of outputs:

8c0ba2a0
fc3b92a0
4500a2a0
d27e82a0
c15d62a0

What exactly is happening here? I allocated 2 bytes for the char since my hex int is 2 bytes.

Does this answer your question? [Strings without a '\0' char?](https://stackoverflow.com/questions/5290823/strings-without-a-0-char) — Brian61354270, Apr 27 '22 at 03:27
You haven't allocated enough space with `malloc(2)`. You need 2 characters for `0x`, 4 characters for the number, and a trailing null byte. So it should be at least `malloc(7)`. So you're causing undefined behavior. — Barmar, Apr 27 '22 at 03:27
It's also not a good idea to perform bit shifting on a signed integer. Bit operations should almost always be on unsigned. — Barmar, Apr 27 '22 at 03:29
Your malloc is too small, you're using the wrong format to printf, you're using something after freeing it, and in any case there's *actually no reason to call sprintf* to do what you want to do. — hobbs, Apr 27 '22 at 03:30
What happens when you `free(s);` before you `printf("%x", s);` -- *Undefined Behavior* (on top of the other problems) — David C. Rankin, Apr 27 '22 at 03:32
and macros without projection with `()` is a bad idea. You don't even need any macros for these, and in case it's supposed to be reusable then an inline function is better than macros — phuclv, Apr 27 '22 at 03:35
just use the value directly, no need to convert to a string first: https://stackoverflow.com/questions/34338780/program-to-count-the-number-of-bits-set-in-c — yano, Apr 27 '22 at 03:42
AFAICT there's no reason to use `malloc()` and `free()` in this code at all; a simple `char s[32];` would be much simpler, more efficient, and less error-prone. — Jeremy Friesner, Apr 27 '22 at 04:37
Does this answer your question? [How to count the number of set bits in a 32-bit integer?](https://stackoverflow.com/questions/109023/how-to-count-the-number-of-set-bits-in-a-32-bit-integer) — Allan Wind, Apr 27 '22 at 04:45
@Barmar this is noted. The value of my hex is actually unsigned, sorry for the mistake, ill edit the question. — , Apr 27 '22 at 04:47
A serious question: Why would you even think to _first_ `free(s)` and _after_ that print `s`? What is the reason your wrote that code? Just a mistake/accident? — hyde, Apr 27 '22 at 05:40
@hyde A person from another forum told me I should do it that way. I researched what free() does and it made sense to me so I used it. I'm new to c and didn't know any better. — , Apr 27 '22 at 12:16
@Duts Right. Well, always take any advice from a random internet post or blog or video with a grain of salt :-) — hyde, Apr 27 '22 at 12:41

Allan Wind · Accepted Answer · 2022-04-27T07:45:41.097

It's too long to write a comment so here goes:

I'm trying to convert a hexadecimal INT

int are stored as a group of value, padding (possible empty) and sign bits, so is there no such thing as a hexadecimal INT but you can represent (print) a given number in the hexadecimal format.

convert a ... INT to a char

That would be lossy conversion as an int might have 4 bytes of data that you are trying to cram into a 1 byte. char specifically may be signed or unsigned. You probably mean string (generic term) or char [] (standard way to represent a string in C).

binary to count the number of ones

That's the real issue you are trying to solve and this is a duplicate of:

How to count the number of set bits in a 32-bit integer?

count number of ones in a given integer using only << >> + | & ^ ~ ! =

To address the question you ask:

Need to allocate more than 2 bytes. Specifically ceil(log16(hex)) + 2 (for 0x) + 1 (for trailing '\0').

One way to get the size is to just ask snprintf(s, 0, ...) then allocate a suitable array via malloc (see first implementation below) or use stack allocated variable length array (VLA).

You can use INT_MAX instead of hex to get an upper bound. log16(INT_MAX) <= CHAR_BIT * sizeof(int) / 4 and the latter is a compile time constant. This means you can allocate your string on stack (see 2nd implementation below).
It's undefined behavior to use a variable after it's deallocated. Move free() to after the last use.

Here is one of the dynamic versions mentioned above:

void count_ones(unsigned hex) {
    char *s = NULL;
    size_t n = snprintf(s, 0, "0x%x", hex) + 1;
    s = malloc(n);
    if(!s) return; // memory could not be allocated
    snprintf(s, n, "0x%x", hex);
    printf("%s (size = %zu)", s, n);
    free(s);
};

Note, I initialized s to NULL which would cause the first call to snprintf() to return an undefined value on SUSv2 (legacy). It's well defined on c99 and later. The output is:

0x3731 (size = 7)

And the compile-time version using a fixed upper bound:

#include <limits.h>

// compile-time
void count_ones(unsigned hex) {
    char s[BIT_CHAR * sizeof(int) / 4 + 3];
    sprintf(s, "0x%x", hex);
    printf("%s (size = %zu)", s, n);
};

and the output is:

0x3731 (size = 11)

score 0 · Answer 2 · answered Apr 27 '22 at 04:32

Your biggest problem is that malloc isn't allocating enough. As Barmar said, you need at least 7 bytes to store it or you could calculate the amount needed. Another problem is that you are freeing it and then using it. It is only one line after the free that you use it again, which shouldn't have anything bad happen like 99.9% of the time, but you should always free after you know you are done using it.

Function is returning a different value every time?

2 Answers2