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

Fix zip_delete_entries bug #320

Closed
wants to merge 1 commit into from

Conversation

howish
Copy link

@howish howish commented Nov 19, 2023

Purpose

Fix bugs in zip_delete_entires which crashed the central directory states.

Elaborate

I discover these bug when trying to use this API along with other entry writing APIs in same zip_open/zip_close section, for example:

    struct zip_t* obj = zip_open("foo.zip", ZIP_DEFAULT_COMPRESSION_LEVEL, 'a');
    {
        const char* entname = "bar.txt";
        // Delete the old entries with same names
        zip_delete_entires(obj, &entname, 1);
        // Add the new entry
        zip_entry_open(obj, entname);
        zip_entry_write(obj, "foobar", sizeof("foobar"));
        zip_entry_close(obj);
    }
    zip_close(obj);

@kuba--
Copy link
Owner

kuba-- commented Nov 19, 2023

Purpose

Fix bugs in zip_delete_entires which crashed the central directory states.

Elaborate

I discover these bug when trying to use this API along with other entry writing APIs in same zip_open/zip_close section, for example:

    struct zip_t* obj = zip_open("foo.zip", ZIP_DEFAULT_COMPRESSION_LEVEL, 'a');
    {
        const char* entname = "bar.txt";
        // Delete the old entries with same names
        zip_delete_entires(obj, &entname, 1);
        // Add the new entry
        zip_entry_open(obj, entname);
        zip_entry_write(obj, "foobar", sizeof("foobar"));
        zip_entry_close(obj);
    }
    zip_close(obj);

This is not expected use case. You cannot delete and append new elements on the same opened archive. You have to close archive first (to refine central directory), and then you can reopen and append stuff.

@@ -666,7 +666,6 @@ static int zip_central_dir_move(mz_zip_internal_state *pState, int begin,

if (next && l_size == 0) {
memmove(pState->m_central_dir.m_p, next, r_size);
pState->m_central_dir.m_p = MZ_REALLOC(pState->m_central_dir.m_p, r_size);
Copy link
Owner

Choose a reason for hiding this comment

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

can you explain why?
we should free memory if not needed

@@ -735,8 +734,7 @@ static int zip_central_dir_delete(mz_zip_internal_state *pState,
d_num += end - begin;
}

pState->m_central_dir_offsets.m_size =
Copy link
Owner

Choose a reason for hiding this comment

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

can you explain this change?
I would understand if we replace mz_uint32 by m_element_size

@kuba--
Copy link
Owner

kuba-- commented Nov 19, 2023

@jinfeihan57 - can you also take a look into this PR, becaue you mainly implemented delete mode, thanks!

@howish howish force-pushed the fix-delete-entries-api branch 2 times, most recently from 486d40e to fbaf7b8 Compare November 29, 2023 10:47
@kuba-- kuba-- marked this pull request as draft November 30, 2023 12:09
@kuba--
Copy link
Owner

kuba-- commented Dec 17, 2023

I'm closing right now. If you find time to elaborate more about the fix, do not hesitate to re-open the issue.
Keep in mind - either you open a zip for reading, writing, appending or deleting. Do not mix operations.

@kuba-- kuba-- closed this Dec 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants