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

Factor our realloc handling to a separate function #344

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

krnowak
Copy link
Collaborator

@krnowak krnowak commented Dec 20, 2022

While working on source/reference split, I added another growable array that held the references, which meant another piece of code doing overflow checks and invoking realloc, so I decided to factor that out to a single place. While at it, I changed one size member to use zip_uint64_t.

This also contains a small behavior change in one place, please see the comment in the code.

Copy link
Member

@dillof dillof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's an excellent idea, which should make code simpler and easier to understand.

I have some suggestions to make it fit better with the conventions used by libzip, see below.

(I guess we should start a style guide.)

#include "zipint.h"

int
(_zip_realloc)(void **inout_memory, zip_uint64_t *inout_alloced, zip_uint64_t element_size, zip_uint64_t additional_elements) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error propagation in libzip is done via a zip_error_t *error argument, usually the last argument of the function. The function fills in error via zip_error_set() and signals that an error occurred via a special return value (often -1 or false).

Also, please put the return type and function prototype on the same line. (This is our new style; we haven't converted all of libzip to this new style yet.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error propagation in libzip is done via a zip_error_t *error argument, usually the last argument of the function. The function fills in error via zip_error_set() and signals that an error occurred via a special return value (often -1 or false).

This made the calling code even simpler, thanks.

Also, please put the return type and function prototype on the same line. (This is our new style; we haven't converted all of libzip to this new style yet.)

Alright, done.

void *new_memory;

if (additional_elements == 0)
additional_elements = 1;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add {/} around single line blocks.

Why do you special case 0 here? If no additional items are required, I would expect this function to do nothing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly? I don't remember. :) I wrote this function back in July or August, when working on that streams branch. Just borrowed that code from there. I probably should have documented this case. Anyway, will add the early return here instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

if (additional_elements == 0)
additional_elements = 1;

if (old_alloced > ZIP_UINT64_MAX - additional_elements)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually do overflow checks for the addition of two unsigned values as if (old_allocated + additional_elements < old_allocated), which does not require knowing the type of the elements.

I would prefer uniformity in our overflow checks so they are easy to recognise as such.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I kept the multiplication overflow check as is, though.

lib/zipint.h Outdated
@@ -487,9 +487,12 @@ typedef struct _zip_pkware_keys zip_pkware_keys_t;
#endif
#endif

#define _zip_realloc(inout_memory, inout_alloced, element_size, additional_elements) (_zip_realloc)((void **)inout_memory, inout_alloced, element_size, additional_elements)
int (_zip_realloc)(void **inout_memory, zip_uint64_t *inout_alloced, zip_uint64_t element_size, zip_uint64_t additional_elements);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a different name for macro and function (like zip_realloc_implementation for the function).

Also, since some C compilers warn about symbols beginning with _ as being reserved, please don't use that prefix for new internal symbols.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

lib/zip_source_buffer.c Show resolved Hide resolved
@@ -486,6 +486,8 @@ typedef struct _zip_pkware_keys zip_pkware_keys_t;
#endif
#endif

#define zip_realloc(inout_memory, inout_alloced, element_size, additional_elements, error) zip_realloc_implementation((void **)inout_memory, inout_alloced, element_size, additional_elements, error)
Copy link
Collaborator Author

@krnowak krnowak Dec 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about dropping the element_size parameter from the macro and make the macro to pass (sizeof(**(inout_memory))) to zip_realloc_implementation instead, but this is probably too much magic.

Comment on lines 57 to 59
/* Pick a smaller max. Not doing it here through ZIP_MIN to
minimize dealing with the wacky integer promotion rules. This
also covers the not-possible-at-the-time-of-writing-this case
of SIZE_MAX being greater than ZIP_UINT64_MAX. */
#if ZIP_UINT64_MAX > SIZE_MAX
max = SIZE_MAX;
#else
max = ZIP_UINT64_MAX;
#endif
if (new_alloced > max / element_size) {
zip_error_set(error, ZIP_ER_MEMORY, 0);
return false;
}
Copy link
Collaborator Author

@krnowak krnowak Dec 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this fiddling with max is necessary, please let me know what you think. In general switching between size_t and uint64 seems to be icky, especially on Windows platforms. Waiting for Appveyor to finish its work to see if msvc has some complaints about the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can always use SIZE_MAX here: new_allocated * element_size must fit in a size_t.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@krnowak krnowak requested a review from dillof December 21, 2022 13:42
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

Successfully merging this pull request may close these issues.

2 participants