[LUCENE-10101] getField vs getDeclaredField in analysis SPI

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 9.0
• Fix Version/s: 9.0
• Component/s: core/other, modules/analysis
• Lucene Fields: New
• Assignee: Uwe Schindler
• Reporter: Uwe Schindler
• Created: 13/Sep/21 08:02
• Updated: 28/Aug/22 16:26
• Resolved: 14/Sep/21 08:32

Description

Discussion between uschindler and romseygeek:

> LUCENE-9281 moved the `lookupSPIName` method from
> AbstractAnalysisFactory to AnalysisSPILoader; the method is mostly the same,
> but one line has been changed from Class.getField() to Class.getDeclaredField().
> This can fall foul of the Security Manager, which wants a higher level of
> permission for getDeclaredField. Was this an intentional change? As I

This was intentional because the previous code wasn't fully correct, because I had some safety check in mind: The main reason for the getDeclaredField() is to lookup the field only in this class; while getField() also looks into superclasses. E.g. if the superclass has a NAME field because of a programming error it would pick that up, which would be wrong. When investigating other implementations using "named" lookups out there (even in JDK), they used getDeclaredField() when accessing a static member.

There are 2 solutions:

• Change to getField(), but in the if statement below check the actual class: (field.getDeclaringClass()==service) (see https://github.com/apache/lucene-solr/pull/1360/files#diff-6a65d91199a18bc4ee2d00a1e9dc283aedc4134846e0d7aafdc484f8263e250bR159-R162)
• Wrap with doPrivileged in Lucene code. As far as I remember Lucene needs the permission anyways. With doPrivileged you would delegate responsibility.

Maybe also post your opinion about think fix #1 or fix #2 is better. I tend to go for fix #1. getDeclaredField() should theoretically be faster, but that won't matter here: If it goes the slow path (going up to superclass) it will fail anyways and that's the exceptional case. A correct factory should have a NAME field and its lookup is fast and the additional check introduced for the class is cheap.

This is the issue to implement one of the solutions, preferably #1

Issue Links

• is broken by: LUCENE-9281 Retire SPIClassIterator from master because Java 9+ uses different mechanism to load services when module system is used
• links to: GitHub Pull Request #298