[jira] [Comment Edited] (TIKA-2667) Upgrade jmatio to 1.4

["Uwe Schindler (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/TIKA-2667?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16518490#comment-16518490 ] 

Uwe Schindler edited comment on TIKA-2667 at 6/20/18 7:00 PM:
--------------------------------------------------------------

It's OK because it wont fail, but I dont understand the need to catch Throwable and the reason to use AtomicReference. The doPrivileged part cannot throw any exception it will always succeed, all exceptions are handled internally! Do privileged is not risky as it does not do something like "sudo" (the name of method is misleading). It just executes the stuff inside the lambda with the privileges of the current code base (and that's documented to be always possible, because we call the method ourselves). Without the doPrivileged it would call the stuff with caller's privileges. The doPrivileged call is there to allow user of the JAR to configure the JVM that only our JAR file can do the privileged action. This improves security, because you don't need to give everyone the permission to call setAccesible() and access Unsafe.

So just copypaste the whole code from Lucene's MMAP directory - the static initializer (maybe do code a bit different with error reporting, Lucene uses no logging):

https://github.com/apache/lucene-solr/blob/master/lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java#L312-L336

The return value of the method reference to the private unmapper method  is Object to allow to pass a String or a MethodHandle through the return value of the privileged block: The code you added using the AtomicReference is not needed - exactly because the privileged code returns either a method handle OR it returns an error message (that's the trick).

The resourceDescription is used to make the exception more meaningful (in Lucene we use filename, so user get's an error about what file handle caused the issue). 

This code is obsolete:
https://github.com/tballison/jmatio/blob/master/src/main/java/com/jmatio/io/MatFileReader.java#L376-L395

The BufferCleaner interface just throws an IOException if unmapping goes wrong - with a meaningful error message. So I'd remove the try-catch block, it's legacy.

Maybe I should create a Pull Request? Unfortunately I have no time and no checkout of the matfile reader ready....



was (Author: thetaphi):
It's OK because it wont fail, but I dont understand the need to catch Throwable and the reason to use AtomicReference. The doPrivileged part cannot throw any exception it will always succeed, all exceptions are handled internally! Do privileged is not risky as it does not do something like "sudo" (the name of method is misleading). It just executes the stuff inside the lambda with the privileges of the current code base (ant that's always possible, because we call the method). Without the doPrivileged it would call the stuff with caller's privileges. The doPrivileged call is there to allow user of the JAR to configure the JVM that only our JAR file can do the privileged action. This improves security, because you don't need to give everyone the permission to call setAccesible() and access Unsafe.

So just copypaste the whole code from Lucene's MMAP directory - the static initializer (maybe do code a bit different with error reporting, Lucene uses no logging):

https://github.com/apache/lucene-solr/blob/master/lucene/core/src/java/org/apache/lucene/store/MMapDirectory.java#L312-L336

The return value of the method reference to the private unmapper method  is Object to allow to pass a String or a MethodHandle through the return value of the privileged block: The code you added using the AtomicReference is not needed - exactly because the privileged code returns either a method handle OR it returns an error message (that's the trick).

The resourceDescription is used to make the exception more meaningful (in Lucene we use filename, so user get's an error about what file handle caused the issue). 

This code is obsolete:
https://github.com/tballison/jmatio/blob/master/src/main/java/com/jmatio/io/MatFileReader.java#L376-L395

The BufferCleaner interface just throws an IOException if unmapping goes wrong - with a meaningful error message. So I'd remove the try-catch block, it's legacy.

Maybe I should create a Pull Request? Unfortunately I have no time and no checkout of the matfile reader ready....


> Upgrade jmatio to 1.4 
> ----------------------
>
>                 Key: TIKA-2667
>                 URL: https://issues.apache.org/jira/browse/TIKA-2667
>             Project: Tika
>          Issue Type: Task
>            Reporter: Tim Allison
>            Priority: Trivial
>
> jmatio 1.3 includes an upgrade to clean MappedByteBuffers in Java 8->11-ea, thanks to a copy/paste from Lucene.
> jmatio 1.4 will include one that actually works. Thank you, [~thetaphi]!



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)