Don't do operations on a closed ZipFile

[blaztinn]

When zip file was opened inside the loop and it did not contain the lib
inside of it, the zipFile stayed non-null for all next iterations. This
can cause next iterations that fail to open zip file to access
previously already closed zipFile because zipFile == null will never
be true.

Fixed by moving the zipFile declaration inside of the loop.

[benjamin-bader]

When zip file was opened inside the loop and it did not contain the lib
inside of it, the zipFile stayed non-null for all next iterations. This
can cause next iterations that fail to open zip file to access
previously already closed zipFile because zipFile == null will never
be true.

This doesn't make sense to me. As is, the variable is fresh on each loop iteration - subsequent iterations cannot see values assigned in previous iterations, because it's effectively a new variable.

With your proposed fix, it will definitely become possible for values to "bleed through" between iterations, which seems logically incorrect.

Finally, I'm not sure what actual problem is being solved here. Is it possible to provide some concrete problem, like a crash report or a reproduction?

[blaztinn]

Currently the variable bleeds through iterations and my patch fixes this, not the other way around. But let me try to describe the problem more precisely.

This is the crash that we get (relinker 1.3.1):

java.lang.IllegalStateException: zip file closed at java.util.zip.ZipFile.ensureOpen(ZipFile.java:706) at java.util.zip.ZipFile.getEntry(ZipFile.java:335) at
com.getkeepsafe.relinker.ApkLibraryInstaller.findAPKWithLibrary(ApkLibraryInstaller.java:92) at
com.getkeepsafe.relinker.ApkLibraryInstaller.installLibrary(ApkLibraryInstaller.java:125) at
com.getkeepsafe.relinker.ReLinkerInstance.loadLibraryInternal(ReLinkerInstance.java:180) at
com.getkeepsafe.relinker.ReLinkerInstance.loadLibrary(ReLinkerInstance.java:136) at
org.libsdl.app.SDL.loadLibrary(SDL.java:58) at
org.libsdl.app.SDLActivity.loadLibraries(SDLActivity.java:328) at
org.libsdl.app.SDLActivity.onCreate(SDLActivity.java:379) at
com.outfit7.bridge.EngineActivity.onCreate(EngineActivity.java:21) at
com.outfit7.engine.EngineHelper.onCreate(EngineHelper.java:245) at
com.outfit7.mytalkingtom2.MyTalkingTom2NativeActivity.onCreate(MyTalkingTom2NativeActivity.java:40) at android.app.Activity.performCreate(Activity.java:7032) at
android.app.Activity.performCreate(Activity.java:7023) at
android.app.Instrumentation.callActivityOnCreate(Instrumentation.java:1236) at
android.app.ActivityThread.performLaunchActivity(ActivityThread.java:2836) at
android.app.ActivityThread.handleLaunchActivity(ActivityThread.java:2965) at
android.app.ActivityThread.-wrap11(Unknown Source:0) at
android.app.ActivityThread$H.handleMessage(ActivityThread.java:1645) at
android.os.Handler.dispatchMessage(Handler.java:106) at
android.os.Looper.loop(Looper.java:164) at
android.app.ActivityThread.main(ActivityThread.java:6648) at
java.lang.reflect.Method.invoke(Native Method) at
com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438) at
com.android.internal.os.ZygoteInit.main(ZygoteInit.java:811)

We don't have a repro but I believe the problem is triggered this way:

1. One of the iterations of the loop for (String sourceDir : sourceDirectories(context) opens the zip file with zipFile = new ZipFile(new File(sourceDir), ZipFile.OPEN_READ);
2. If we don't find the lib in that iteration, the zipFile is closed at end of the iteration with zipFile.close();. But it is not nulled!
3. On next iteration zipFile = new ZipFile(new File(sourceDir), ZipFile.OPEN_READ); can fail, resulting in zipFile keeping the value from previous iteration.
4. if (zipFile == null) check does not skip this iteration, because zipFile is not null.
5. We crash at libraryEntry = zipFile.getEntry(jniNameInApk); because we are doing operations on an already closed zipFile from previous iteration.

My proposed fix moves the zipFile inside the loop because I don't see a need to keep the value of it over multiple iterations. I could be missing something though.

[benjamin-bader]

I don't know how I managed to misread this simple PR, but I did - of course, you're correct. Moving the variable into the loop fixes that problem. I must have mixed up the red and green lines in the diff.

This now looks fine to me. Thanks!

[blaztinn]

I though something was off with you first comment, it took me a while to see that you probably mixed up the diff :)

Thanks for the review!

[benjamin-bader]

@emarc-m would you please review/merge this as well? It's a legit bugfix.

[emarc-m]

@benjamin-bader Thanks for alerting me. I'll review as well.

[emarc-m]

@blaztinn Thanks for fixing. I'll include this on the next release and update once I do.