Bug 58098

Summary: NPE at NPOIFSFileSystem.<init>
Product: POI Reporter: Tiago Cavaleiro <tcavaleiro>
Component: POIFSAssignee: POI Developers List <dev>
Status: RESOLVED FIXED    
Severity: minor CC: tcavaleiro
Priority: P2    
Version: 3.12-FINAL   
Target Milestone: ---   
Hardware: PC   
OS: All   

Description Tiago Cavaleiro 2015-07-03 10:14:46 UTC
An NPE is being throw when opening a XLSX file that is currently opened by Excel (write mode).
On my opinion it should throw the java.io.FileNotFoundException instead.


Stacktrace:
Exception in thread "main" java.lang.NullPointerException
	at org.apache.poi.poifs.filesystem.NPOIFSFileSystem.<init>(NPOIFSFileSystem.java:235)
	at org.apache.poi.poifs.filesystem.NPOIFSFileSystem.<init>(NPOIFSFileSystem.java:165)
	at xxxxxx.CloseableWorkbook.create(CloseableWorkbook.java:271)
	at xxxxxx.ReadExcel.main(ReadExcel.java:29)


Issue story description:
NPOIFSFileSystem first tries to open the file and then gets an java.io.FileNotFoundException: (....).xlsx (The process cannot access the file because it is being used by another process) however since there is no null check on the channel it gets an NPE.

Class NPOIFSFileSystem should be modified to check for channel first...

private NPOIFSFileSystem(FileChannel channel, File srcFile, boolean readOnly, boolean closeChannelOnError)
         throws IOException
    {
       (...) // try 

       } catch(IOException e) {
          if(closeChannelOnError && channel != null) {
             channel.close();
          }
          throw e;
(...)


PS: Actually the null check if being done when handling RuntimeException.
Comment 1 Nick Burch 2015-07-06 22:29:06 UTC
Are you able to create a cross-platform unit test that is able to trigger this NPE, or is it uniquely "windows with file open" only?

(If we can add a universal unit test for this, that'd be great, but if not we won't bother waiting and can just add the null check)
Comment 2 Tiago Cavaleiro 2015-07-08 10:40:24 UTC
I did try to recreate a unit test using a FileLock, but accordingly with the API [1] is can work differently on different systems.

[1] http://docs.oracle.com/javase/8/docs/api/java/nio/channels/FileLock.html

"Platform dependencies

(...)
Whether or not a lock actually prevents another program from accessing the content of the locked region is system-dependent and therefore unspecified. The native file-locking facilities of some systems are merely advisory, meaning that programs must cooperatively observe a known locking protocol in order to guarantee data integrity. On other systems native file locks are mandatory, meaning that if one program locks a region of a file then other programs are actually prevented from accessing that region in a way that would violate the lock. On yet other systems, whether native file locks are advisory or mandatory is configurable on a per-file basis. To ensure consistent and correct behavior across platforms, it is strongly recommended that the locks provided by this API be used as if they were advisory locks.
(...)"


So I don't think it's worth a try. Also the file lock should be simulated from a different process than Java (on the original case from Excel), or else it can have different behaviour due to how Java handles file I/O, etc..
And last but not least, you're already doing the NPE check for RuntimeException as you can see on class NPOIFSFileSystem (line 243)

(...)
       } catch(IOException e) {
          if(closeChannelOnError) {
             channel.close();
          }
          throw e;
       } catch(RuntimeException e) {
          // Comes from Iterators etc.
          // TODO Decide if we can handle these better whilst
          //  still sticking to the iterator contract
          if(closeChannelOnError) {
              if (channel != null) {
                  channel.close();
                  channel = null;
              }
          }
          throw e;
       }
(...)
Comment 3 Nick Burch 2015-07-29 16:37:56 UTC
Hopefully fixed in r1693311, I've added the same logic for both catches