Setting a bit after using realloc

Question

I am trying to fix my function where I set the Nth bit of an already allocated space. If the size of N is larger than the size of space malloc'd then reallocate more space to be able to set the Nth bit.

My issue is that whenever I set a bit higher than the space allocated it sets two bits. I have been playing with it for a bit now and I feel stumped. I feel like the issue lies with using realloc incorrectly?

An example is when I try to set the 52nd bit it results with this output:

0001 0000 0000 0000 0000 0000 0000 0000 0001 0000 0000 0000 0000 0000

Both the 52nd bit and 20th bit are set

I made a repl.it of my whole program here: https://repl.it/@dholton/Broken

More specifically here is my function:

Status bit_flags_set_flag( BIT_FLAGS hBit_flags, int flag_position ) {

  bits *phBit_flags = ( bits * ) hBit_flags ;
  int *new_data = NULL ;

  if ( flag_position < phBit_flags -> capacity ) {

    // Check to see if the bit request to set is lower than the memory allocated.
    *phBit_flags -> data |= ( 1 << flag_position ) ;

  } else if ( flag_position >= phBit_flags -> capacity ) {

    // The bit requested is larger so realloc new data to reach the length of the requested bit.
    new_data = ( int * ) realloc( phBit_flags -> data, ( flag_position / 8 ) + 1 ) ;

    if ( new_data == NULL ) {

      free( new_data ) ;
      return FAILURE ;

    }

    free( phBit_flags -> data ) ;

    phBit_flags -> data = new_data ;
    phBit_flags -> size = flag_position ;
    phBit_flags -> capacity = ( flag_position / 8 + 1 ) * 8 ;
    // capacity is number of bits

    *phBit_flags -> data |= ( 1 << flag_position ) ;
    // Set nth bit

    return SUCCESS ;

  }

  return FAILURE ;

}

Answer 1

There are multiple problems:

• the type used for storing the bits, int, is inappropriate: you should aither use unsigned int or simply unsigned char.

• the method used to set the bit is incorrect, see the correction below.

• the array reallocated with a larger size is not initialized to 0 beyond the original size. You must perform this initialization yourself, after a successful call to realloc.

Here is a corrected version:

typedef struct Bits {
    size_t size;
    size_t capacity;
    unsigned char *data;
} bits;

Status bit_flags_set_flag(BIT_FLAGS hBit_flags, int flag_position) {
    bits *phBit_flags = hBit_flags;

    if (flag_position >= phBit_flags->capacity) {
        // The bit requested is larger so realloc new data to reach the length of the requested bit.
        size_t cur_size = phBit_flags->capacity / 8;
        size_t new_size = flag_position / 8 + 1;
        unsigned char *new_data = realloc(phBit_flags->data, new_size);
        if (new_data == NULL) {
            return FAILURE;
        }
        memset(new_data + cur_size, 0, new_size - cur_size);
        phBit_flags->data = new_data;
        phBit_flags->size = flag_position + 1;
        // capacity is number of bits
        phBit_flags->capacity = new_size * 8;
    }
    // Set nth bit
    phBit_flags->data[flag_position / 8] |= 1 << (flag_position & 7);

    return SUCCESS;
}

Answer 2

This whole block is wrong

new_data = ( int * ) realloc( phBit_flags -> data, ( flag_position / 8 ) + 1 ) ;

if ( new_data == NULL ) {

  free( new_data ) ;
  return FAILURE ;

}

free( phBit_flags -> data ) ;

phBit_flags -> data = new_data ;
phBit_flags -> size = flag_position ;
phBit_flags -> capacity = ( flag_position / 8 + 1 ) * 8 ;

realloc might return NULL, you check for that, but free(new_data) is essentially doing free(NULL). You should free the original pointer and set it to NULL.

if(new_data == NULL)
{
    free(phBit_flags->data);
    phBit_flags->data = NULL;
    return FAILURE;
}

If realloc does return a pointer, it might be a new pointer. If it's a new pointer, realloc already freed the old memory and you don't have to do it.

So remove the free( phBit_flags -> data ) ; before the assigning phBit_flags->data = new_data.

I'm not sure the allocation size is correct. Usually you allocate by multiplying the required new size with sizeof(int) or sizeof *var. But ( flag_position / 8 ) + 1 doesn't do that, in fact it isn't even a multiple or 4 (assuming the size of an int equals to 4), so you might be allocating less space than you need.

*phBit_flags -> data |= ( 1 << flag_position ) ;

this is going to work only if flag_position is not greater than 31 (again, asumming 4 for the size of int). So you would have to calculate in which byte you would need to do the shifting.

If you want to have more bits than an int can hold, then you can allocate an array of ints and use for example little endian in your array, meaning the least significant bits should by in data[0].

I don't understand you flag_position / 8 calculation, where does the 8 come from? If flag_position is 35 for example, then 35 / 8*sizeof(int) would give 1, and 35 % 8*sizeof(int) would give you 3, so the 35th bit would be in the you would have to set the 3th bit of data[1].

So the realloc should be

new_data = realloc( phBit_flags -> data,
        ((flag_position / 8*sizeof(phBit_flags->data) + 1) * sizeof *phBit_flags->data);

(flag_position / 8*sizeof(phBit_flags->data) + 1 would give you how many ints you will need, and sizeof *phBit_flags->data); gives you the size of an int.

So the setting of the bit:

size_t idx = flag_position / (8 * sizeof *phBit_flags->data);
size_t bit = flag_position % (8 * sizeof *phBit_flags->data);

phBit_flags->data[idx] |= (1 << bit);

Answer 3

*phBit_flags -> data |= ( 1 << flag_position ) ;

should be something like

size_t pos = flag_position / (8 * sizeof phBit_flags->data[0]);
size_t bit = flag_position % (8 * sizeof phBit_flags->data[0]);

phBit_flags -> data[pos] |= ((typeof(phBit_flags->data[0]))(1) << bit);