Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
fhilgers opened this issue Oct 25, 2021 · 5 comments
Open

Comments

@fhilgers
Copy link

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
Copy link
Owner

srdja commented Oct 26, 2021

@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
Copy link
Author

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
Copy link
Owner

srdja commented Oct 26, 2021

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
Copy link
Author

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
Copy link
Owner

srdja commented May 24, 2024

@fhilgers
Sure, that would be awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants