Bug 52665

Summary: An incomplete fix for the NPE bug in ZipFileZipEntrySource.java
Product: POI Reporter: lianggt08
Component: POI OverallAssignee: POI Developers List <dev>
Status: RESOLVED FIXED    
Severity: critical CC: lianggt08
Priority: P2    
Version: unspecified   
Target Milestone: ---   
Hardware: PC   
OS: All   

Description lianggt08 2012-02-14 10:18:23 UTC 
The fix revision 1179452 was aimed to remove an NPE bug on the  "zipArchive" in the method "close" of the file "/poi/trunk/src/ooxml/java/org/apache/poi/openxml4j/util/ZipFileZipEntrySource.java" , but it is incomplete. 
Since the "zipArchive" could be null during the run-time execution, its value should also be null-checked before being dereferenced in other methods. 

The buggy code locations the same fix needs to be applied at are as bellows: 
 
Line 44 of the method "getEntries()" : 
    public Enumeration<? extends ZipEntry> getEntries() {
[Line 44]      return zipArchive.entries();
   }

Line  48 of the method "getInputStream()" :
  public InputStream getInputStream(ZipEntry entry) throws IOException {
[Line 48]      return zipArchive.getInputStream(entry);
   }
Comment 1 Nick Burch 2012-02-14 12:59:12 UTC 
I'm not sure that zipArchive should ever be null - how are you triggering this?
Comment 2 lianggt08 2012-02-15 06:03:36 UTC 
If the method "close" is called, zipArchive will be null. After that, if the methods "getEntries()" and "getInputStream()" are called, NPE will happen at the code line 44 and 48.  

Please check the bug issue 51949 at https://issues.apache.org/bugzilla/show_bug.cgi?id=51949  for the method "close": 


    public void close() throws IOException {
        zipArchive.close();
        zipArchive = null;
    }



Thanks.
Comment 3 Yegor Kozlov 2012-02-15 08:16:08 UTC 
What do you expect from  "getEntries()" and "getInputStream()": return null or throw a more sensible exception? 

I would say that instead of the NPE they should throw IllegalStateException("zip file closed"), the way java.util.ZipFile does. Does it seem the right fix for you? 

Yegor

(In reply to comment #2)
> If the method "close" is called, zipArchive will be null. After that, if the
> methods "getEntries()" and "getInputStream()" are called, NPE will happen at
> the code line 44 and 48.  
> 
> Please check the bug issue 51949 at
> https://issues.apache.org/bugzilla/show_bug.cgi?id=51949  for the method
> "close": 
> 
> 
>     public void close() throws IOException {
>         zipArchive.close();
>         zipArchive = null;
>     }
> 
> 
> 
> Thanks.
Comment 4 lianggt08 2012-02-15 11:41:37 UTC 
OK.
Throwing an IllegalStateException("zip file closed") will be much better. 
Thanks.
Comment 5 Nick Burch 2012-02-15 11:51:25 UTC 
As of r1244450, we now through an IllegalArgumentException for this case