Crashes on Android / Double-close issues in API?

[Malu007]

Hi there,

great piece of Software!
I am using your wrapper for an Android app. Everything works smoothly until I sporadically have a crash at about 300 to 600 calls. Unfortunately I don't see much via Logcat and the debugger. However, sometimes I see the following:

E/fdsan: attempted to close file descriptor 68, expected to be unowned, actually owned by FILE 0x74d3e14018*

My function:

bool zipAppend(const char* zipFileName, const char* fileName, int fileSize, const char* fileBuf)
{
    int compressionLevel = 0; // no compression

    struct zip_t* zip = zip_open(zipFileName, compressionLevel, 'a');
    if (!zip)
    {
        return false;
    }
    else
    {
        zip_entry_open(zip, fileName);
        {
            zip_entry_write(zip, fileBuf, fileSize);
        }
        zip_entry_close(zip);
        zip_close(zip);
    }

    return true;
}

[kuba--]

@Malu007 - I'll take a look. I think I found some possible scenario in miniz, but I'll have to double check it.

[kuba--]

Question - you don't use this function in multiple threads, do you?

[Malu007]

I use it in one Workerthread. Here I open a file, write to it and then close it. And this many times. In another issue I saw that a user ( @seanbrakefield, #190 ) used Android for testing. So it seems he didn't had the problems i have. I used a Honor8X but i don't think that this is the problem

[Malu007]

In the meantime I have tried a few things and can say that when write is used instead of append, everything works.

So if I use in my append function the following line:
struct zip_t* zip = zip_open(zipFileName, compressionLevel, 'a');
I replace it with this:
struct zip_t* zip = zip_open(zipFileName, compressionLevel, 'w');
the app runs without crashing.

Unfortunately, I don't know much about "file descriptor ownership". I only see that freopen is used in miniz.h and can only make guesses here.

Maybe as a help how to reproduce the error:
-no compression
-call zip_open with append mode
-append a lot of files... I use very large images from smartphone. At the latest after about 1200 append calls the app crashes

[kuba--]

Do you know if it crashes on write or on open?

[Malu007]

Unclear. Sometimes the app just hangs. Then i see fdsan errors in logcat. The last time my debugger could catch something it was on zip_entry_close. I looked up the stack and saw that in zip.c there is a null value in zip->entry.name when this line is executed:
entrylen = (mz_uint16)strlen(zip->entry.name);
strlen with a null was a chance for the debugger to catch that situation. But i think something is wrong before this happens and i guess it is undefined behaviour due to something arround that append mode when in miniz.h the file is being reopened.

[Malu007]

I would like to express my gratitude for the work that has gone into this library. I have since been able to tinker my way around the problem. In my particular case, it has resulted in no more crashes. Here is my diff:

index 1d1935a..5136c2b 100644
--- a/Android App/basic/src/main/cpp/zip/miniz.h	
+++ b/Android App/basic/src/main/cpp/zip/miniz.h	
@@ -5916,7 +5916,7 @@ mz_bool mz_zip_reader_init_file_v2(mz_zip_archive *pZip, const char *pFilename,
        (archive_size < MZ_ZIP_END_OF_CENTRAL_DIR_HEADER_SIZE)))
     return mz_zip_set_error(pZip, MZ_ZIP_INVALID_PARAMETER);
 
-  pFile = MZ_FOPEN(pFilename, "rb");
+  pFile = MZ_FOPEN(pFilename, "r+b");
   if (!pFile)
     return mz_zip_set_error(pZip, MZ_ZIP_FILE_OPEN_FAILED);
 
@@ -7936,15 +7936,15 @@ mz_bool mz_zip_writer_init_from_reader_v2(mz_zip_archive *pZip,
       if (!pFilename)
         return mz_zip_set_error(pZip, MZ_ZIP_INVALID_PARAMETER);
 
-      /* Archive is being read from stdio and was originally opened only for
-       * reading. Try to reopen as writable. */
-      if (NULL ==
-          (pState->m_pFile = MZ_FREOPEN(pFilename, "r+b", pState->m_pFile))) {
-        /* The mz_zip_archive is now in a bogus state because pState->m_pFile is
-         * NULL, so just close it. */
-        mz_zip_reader_end_internal(pZip, MZ_FALSE);
-        return mz_zip_set_error(pZip, MZ_ZIP_FILE_OPEN_FAILED);
-      }
+//      /* Archive is being read from stdio and was originally opened only for
+//       * reading. Try to reopen as writable. */
+//      if (NULL ==
+//          (pState->m_pFile = MZ_FREOPEN(pFilename, "r+b", pState->m_pFile))) {
+//        /* The mz_zip_archive is now in a bogus state because pState->m_pFile is
+//         * NULL, so just close it. */
+//        mz_zip_reader_end_internal(pZip, MZ_FALSE);
+//        return mz_zip_set_error(pZip, MZ_ZIP_FILE_OPEN_FAILED);
+//      }
     }
 
     pZip->m_pWrite = mz_zip_file_write_func;

To describe it briefly. I took the first time I opened the zip file immediately the mode r+b and not later with reopen. Since I don't know the exact procedure in miniz.h this solution should be used with some caution.

[kuba--]

I see your point. Generally you want to get rid of freopen. I'll take a closer look into this. In theory it makes sense, but I have to double check that it won't mess up any internal directory etc.

[kuba--]

@Malu007 - I analyzed this particular case, and I think getting rid of freopen and changing open mode to r+b is totally fine.
Just in case, to keep compatibility in miniz, I've created new internal functions for append.
PTAL, I hope it works for you: https://github.com/kuba--/zip/pull/223

It's already merged into the master branch.

[kuba--]

@Malu007, feel free to reopen the issue, if you can reproduce it on the master branch.