srdja / Collections-C

A library of generic data structures for the C language.
http://srdja.github.io/Collections-C
GNU Lesser General Public License v3.0
2.8k stars 328 forks source link

(deque.c): inline function upper_pow_two only valid for 32 bit size_t #146

Closed fhilgers closed 3 months ago

fhilgers commented 2 years ago

In the common.h header file, MAX_POW_TWO is defined like this:

#ifdef ARCH_64
#define MAX_POW_TWO (((size_t) 1) << 63)
#else

This macro is for example used in the function, static size_t upper_pow_two (size_t), of the deque.c file. It either returns MAX_POW_TWO or the given argument rounded up to the next higher power of two. As far as I am aware, those bit manipulations:

n--;
n |= n >> 1;
n |= n >> 2;
n |= n >> 4;
n |= n >> 8;
n |= n >> 16;
n++;

used to round the argument to the next power are only valid up to 32-bit, not 64-bit . Is this by design or by accident?

srdja commented 2 years ago

@flyxi121 It's definitely not by design. It should have been updated when ARCH_64 was added, but apparently it was overlooked. Thanks for spotting it!

fhilgers commented 2 years ago

After looking through the array.c file I notices another thing. When expanding the array, it is checked whether an overflow happens on increasing the capacity:

    size_t new_capacity = ar->capacity * ar->exp_factor;

    /* As long as the capacity is greater that the expansion factor
     * at the point of overflow, this is check is valid. */
    if (new_capacity <= ar->capacity)
        ar->capacity = CC_MAX_ELEMENTS;
    else
        ar->capacity = new_capacity;

But afterwards the variable new_capacity, which might contain an invalid value in case of an overflow, is used for the allocation of the new buffer:

    void **new_buff = ar->mem_alloc(new_capacity * sizeof(void*));
srdja commented 2 years ago

Yep, that's also a bug. It should be:

void **new_buff = ar->mem_alloc(ar->capacity * sizeof(void*));

Again, thanks for eyeballing the code. These things are near impossible to catch with tests. :)

fhilgers commented 3 months ago

Hi, I just looked through my open issues and came across this one. Should I open a quick PR to fix both things and to close the issue?

srdja commented 3 months ago

@fhilgers Sure, that would be awesome!