Mailing List Archive

Hi Alan,

> 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.

I'd open a JIRA issue, I can fix this. It only affects Lucene 9.0.

> understand it it’s looking for a NAME static field on the class in question, which
> should always be public. I’m in the process of upgrading elasticsearch to use a
> lucene 9 snapshot, and this change means that I need to wrap SPI reloading
> code in doPrivileged() blocks, which is a bit of a pain.

Thansk for doing this. Is Elasticsearch now using the Analysis Factory framework instead of their own factories?

> Thanks, Alan

No problem,
Uwe



---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

Thanks Uwe!

> On 6 Sep 2021, at 13:11, Uwe Schindler <uwe@thetaphi.de> wrote:
>
> Hi Alan,
>
>> 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.
>
> I'd open a JIRA issue, I can fix this. It only affects Lucene 9.0.
>
>> understand it it’s looking for a NAME static field on the class in question, which
>> should always be public. I’m in the process of upgrading elasticsearch to use a
>> lucene 9 snapshot, and this change means that I need to wrap SPI reloading
>> code in doPrivileged() blocks, which is a bit of a pain.
>
> Thansk for doing this. Is Elasticsearch now using the Analysis Factory framework instead of their own factories?
>
>> Thanks, Alan
>
> No problem,
> Uwe
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

Hi Alan,

Would you open issue, I will take it!?

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.

Uwe

-----
Uwe Schindler
Achterdiek 19, D-28357 Bremen
https://www.thetaphi.de
eMail: uwe@thetaphi.de

> -----Original Message-----
> From: Alan Woodward <romseygeek@gmail.com>
> Sent: Monday, September 6, 2021 3:48 PM
> To: dev@lucene.apache.org
> Subject: Re: getField vs getDeclaredField in analysis SPI
>
> Thanks Uwe!
>
> > On 6 Sep 2021, at 13:11, Uwe Schindler <uwe@thetaphi.de> wrote:
> >
> > Hi Alan,
> >
> >> 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-
> 6a65d91199a18bc4ee2d00a1e9dc283aedc4134846e0d7aafdc484f8263e250bR
> 159-R162)
> > - Wrap with doPrivileged in Lucene code. As far as I remember Lucene needs
> the permission anyways. With doPrivileged you would delegate responsibility.
> >
> > I'd open a JIRA issue, I can fix this. It only affects Lucene 9.0.
> >
> >> understand it it’s looking for a NAME static field on the class in question,
> which
> >> should always be public. I’m in the process of upgrading elasticsearch to use
> a
> >> lucene 9 snapshot, and this change means that I need to wrap SPI reloading
> >> code in doPrivileged() blocks, which is a bit of a pain.
> >
> > Thansk for doing this. Is Elasticsearch now using the Analysis Factory
> framework instead of their own factories?
> >
> >> Thanks, Alan
> >
> > No problem,
> > Uwe
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> > For additional commands, e-mail: dev-help@lucene.apache.org
> >
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

Hi Alan,

I will open an issue about this!

Uwe

-----
Uwe Schindler
Achterdiek 19, D-28357 Bremen
https://www.thetaphi.de
eMail: uwe@thetaphi.de

> -----Original Message-----
> From: Uwe Schindler <uwe@thetaphi.de>
> Sent: Monday, September 6, 2021 4:57 PM
> To: dev@lucene.apache.org
> Subject: RE: getField vs getDeclaredField in analysis SPI
>
> Hi Alan,
>
> Would you open issue, I will take it!?
>
> 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.
>
> Uwe
>
> -----
> Uwe Schindler
> Achterdiek 19, D-28357 Bremen
> https://www.thetaphi.de
> eMail: uwe@thetaphi.de
>
> > -----Original Message-----
> > From: Alan Woodward <romseygeek@gmail.com>
> > Sent: Monday, September 6, 2021 3:48 PM
> > To: dev@lucene.apache.org
> > Subject: Re: getField vs getDeclaredField in analysis SPI
> >
> > Thanks Uwe!
> >
> > > On 6 Sep 2021, at 13:11, Uwe Schindler <uwe@thetaphi.de> wrote:
> > >
> > > Hi Alan,
> > >
> > >> 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-
> >
> 6a65d91199a18bc4ee2d00a1e9dc283aedc4134846e0d7aafdc484f8263e250bR
> > 159-R162)
> > > - Wrap with doPrivileged in Lucene code. As far as I remember Lucene needs
> > the permission anyways. With doPrivileged you would delegate responsibility.
> > >
> > > I'd open a JIRA issue, I can fix this. It only affects Lucene 9.0.
> > >
> > >> understand it it’s looking for a NAME static field on the class in question,
> > which
> > >> should always be public. I’m in the process of upgrading elasticsearch to
> use
> > a
> > >> lucene 9 snapshot, and this change means that I need to wrap SPI
> reloading
> > >> code in doPrivileged() blocks, which is a bit of a pain.
> > >
> > > Thansk for doing this. Is Elasticsearch now using the Analysis Factory
> > framework instead of their own factories?
> > >
> > >> Thanks, Alan
> > >
> > > No problem,
> > > Uwe
> > >
> > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> > > For additional commands, e-mail: dev-help@lucene.apache.org
> > >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> > For additional commands, e-mail: dev-help@lucene.apache.org
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

See https://issues.apache.org/jira/browse/LUCENE-10101

-----
Uwe Schindler
Achterdiek 19, D-28357 Bremen
https://www.thetaphi.de
eMail: uwe@thetaphi.de

> -----Original Message-----
> From: Uwe Schindler <uwe@thetaphi.de>
> Sent: Monday, September 13, 2021 9:59 AM
> To: dev@lucene.apache.org
> Subject: RE: getField vs getDeclaredField in analysis SPI
>
> Hi Alan,
>
> I will open an issue about this!
>
> Uwe
>
> -----
> Uwe Schindler
> Achterdiek 19, D-28357 Bremen
> https://www.thetaphi.de
> eMail: uwe@thetaphi.de
>
> > -----Original Message-----
> > From: Uwe Schindler <uwe@thetaphi.de>
> > Sent: Monday, September 6, 2021 4:57 PM
> > To: dev@lucene.apache.org
> > Subject: RE: getField vs getDeclaredField in analysis SPI
> >
> > Hi Alan,
> >
> > Would you open issue, I will take it!?
> >
> > 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.
> >
> > Uwe
> >
> > -----
> > Uwe Schindler
> > Achterdiek 19, D-28357 Bremen
> > https://www.thetaphi.de
> > eMail: uwe@thetaphi.de
> >
> > > -----Original Message-----
> > > From: Alan Woodward <romseygeek@gmail.com>
> > > Sent: Monday, September 6, 2021 3:48 PM
> > > To: dev@lucene.apache.org
> > > Subject: Re: getField vs getDeclaredField in analysis SPI
> > >
> > > Thanks Uwe!
> > >
> > > > On 6 Sep 2021, at 13:11, Uwe Schindler <uwe@thetaphi.de> wrote:
> > > >
> > > > Hi Alan,
> > > >
> > > >> 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-
> > >
> >
> 6a65d91199a18bc4ee2d00a1e9dc283aedc4134846e0d7aafdc484f8263e250bR
> > > 159-R162)
> > > > - Wrap with doPrivileged in Lucene code. As far as I remember Lucene
> needs
> > > the permission anyways. With doPrivileged you would delegate
> responsibility.
> > > >
> > > > I'd open a JIRA issue, I can fix this. It only affects Lucene 9.0.
> > > >
> > > >> understand it it’s looking for a NAME static field on the class in question,
> > > which
> > > >> should always be public. I’m in the process of upgrading elasticsearch to
> > use
> > > a
> > > >> lucene 9 snapshot, and this change means that I need to wrap SPI
> > reloading
> > > >> code in doPrivileged() blocks, which is a bit of a pain.
> > > >
> > > > Thansk for doing this. Is Elasticsearch now using the Analysis Factory
> > > framework instead of their own factories?
> > > >
> > > >> Thanks, Alan
> > > >
> > > > No problem,
> > > > Uwe
> > > >
> > > >
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> > > > For additional commands, e-mail: dev-help@lucene.apache.org
> > > >
> > >
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> > > For additional commands, e-mail: dev-help@lucene.apache.org
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> > For additional commands, e-mail: dev-help@lucene.apache.org
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org

Thanks for opening the issue Uwe! I agree that option #1 is the best long term fix (although for the moment I’ve wrapped the relevant ES code in a doPrivileged block which solves the immediate problem).

> Is Elasticsearch now using the Analysis Factory framework instead of their own factories?

No, we still have our own factory implementations. In fact, looking more closely at this, I don’t think we need to call these reload SPI methods at all and I can probably remove it entirely :) Still, it found a possible bug!


> On 13 Sep 2021, at 12:48, Uwe Schindler <uwe@thetaphi.de> wrote:
>
> See https://issues.apache.org/jira/browse/LUCENE-10101
>
> -----
> Uwe Schindler
> Achterdiek 19, D-28357 Bremen
> https://www.thetaphi.de
> eMail: uwe@thetaphi.de
>
>> -----Original Message-----
>> From: Uwe Schindler <uwe@thetaphi.de>
>> Sent: Monday, September 13, 2021 9:59 AM
>> To: dev@lucene.apache.org
>> Subject: RE: getField vs getDeclaredField in analysis SPI
>>
>> Hi Alan,
>>
>> I will open an issue about this!
>>
>> Uwe
>>
>> -----
>> Uwe Schindler
>> Achterdiek 19, D-28357 Bremen
>> https://www.thetaphi.de
>> eMail: uwe@thetaphi.de
>>
>>> -----Original Message-----
>>> From: Uwe Schindler <uwe@thetaphi.de>
>>> Sent: Monday, September 6, 2021 4:57 PM
>>> To: dev@lucene.apache.org
>>> Subject: RE: getField vs getDeclaredField in analysis SPI
>>>
>>> Hi Alan,
>>>
>>> Would you open issue, I will take it!?
>>>
>>> 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.
>>>
>>> Uwe
>>>
>>> -----
>>> Uwe Schindler
>>> Achterdiek 19, D-28357 Bremen
>>> https://www.thetaphi.de
>>> eMail: uwe@thetaphi.de
>>>
>>>> -----Original Message-----
>>>> From: Alan Woodward <romseygeek@gmail.com>
>>>> Sent: Monday, September 6, 2021 3:48 PM
>>>> To: dev@lucene.apache.org
>>>> Subject: Re: getField vs getDeclaredField in analysis SPI
>>>>
>>>> Thanks Uwe!
>>>>
>>>>> On 6 Sep 2021, at 13:11, Uwe Schindler <uwe@thetaphi.de> wrote:
>>>>>
>>>>> Hi Alan,
>>>>>
>>>>>> 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-
>>>>
>>>
>> 6a65d91199a18bc4ee2d00a1e9dc283aedc4134846e0d7aafdc484f8263e250bR
>>>> 159-R162)
>>>>> - Wrap with doPrivileged in Lucene code. As far as I remember Lucene
>> needs
>>>> the permission anyways. With doPrivileged you would delegate
>> responsibility.
>>>>>
>>>>> I'd open a JIRA issue, I can fix this. It only affects Lucene 9.0.
>>>>>
>>>>>> understand it it’s looking for a NAME static field on the class in question,
>>>> which
>>>>>> should always be public. I’m in the process of upgrading elasticsearch to
>>> use
>>>> a
>>>>>> lucene 9 snapshot, and this change means that I need to wrap SPI
>>> reloading
>>>>>> code in doPrivileged() blocks, which is a bit of a pain.
>>>>>
>>>>> Thansk for doing this. Is Elasticsearch now using the Analysis Factory
>>>> framework instead of their own factories?
>>>>>
>>>>>> Thanks, Alan
>>>>>
>>>>> No problem,
>>>>> Uwe
>>>>>
>>>>>
>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>>>>> For additional commands, e-mail: dev-help@lucene.apache.org
>>>>>
>>>>
>>>>
>>>> ---------------------------------------------------------------------
>>>> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>>>> For additional commands, e-mail: dev-help@lucene.apache.org
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>>> For additional commands, e-mail: dev-help@lucene.apache.org
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
>> For additional commands, e-mail: dev-help@lucene.apache.org
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org