On Dec 12, 2017, at 1:55 PM, David Holmes <david.hol...@oracle.com> wrote:
>
> Hi John,
>
> On 13/12/2017 7:22 AM, John Rose wrote:
>> On Dec 11, 2017, at 11:52 PM, David Holmes <david.hol...@oracle.com
>> <mailto:david.hol...@oracle.com>> wrote:
>>>
>>> I have one query with regards to the processing of the NestMembers
>>> attribute. Originally when I wrote the draft I copied the InnerClasses text
>>> and that included prohibiting duplicate entries in NestMembers. You dropped
>>> that restriction from the spec (though I haven't yet removed it from the
>>> VM). How does that interact with the Class.getNestMembers() API? Should it
>>> return the raw NestMembers contents (which may include duplicates -
>>> including additional references to "self") or should it return the set of
>>> members (ie no duplicates, including self)?
>> My take:
>> We have landed on laissez-faire for this kind of format check
>> at class load time. The access checking rules are cleverly
>> written so that garbage in the list is ignored. This is noted
>> by the non-normative language in 4.7.29:
>>> This array is consulted during access checking (5.4.4). It should consist
>>> of references to other classes and interfaces that belong to the same
>>> run-time package and have NestHost attributes referencing this class or
>>> interface. Items that do not meet this description are discouraged and will
>>> be ignored by access checking.
>> There are four kinds of garbage:
>> 1. [self] the nest host class itself (the one containing NestMs)
>> 2. [outsider] valid classes whose package prefix differs form that of the
>> nest host class
>> 3. [duplicate] duplicate entries (either the same CONSTANT_Class or a
>> homonym)
>> 4. [not-found] references to classes which will not exist when resolved
>> The spec. says these are *allowed* and *ignored*.
>> (A fifth kind of garbage, syntax errors in class names, is caught
>> by routine processing of CONSTANT_Class items. Maybe this is worth
>> a unit test. The other four cases are worth a unit test, for that matter.)
>
> A sixth kind of garbage, or an expansion of #2, are classes that don't list
> the current class as their nest host.
>
>> Each of these four cases poses a question for reflection: Do we hide
>> it or expose it as an anomalous entry in the reflected list?
>> In an earlier conversation, I think we agree that case 4 should just
>> throw an error when reflection is performed, following a similar
>> precedent with Class.getDefinedClasses.
>> I don't remember our resolution of what to do about 1/2/3. The overall
>
> I recall no discussion of #1 and #3. For #2, #4 and #6 - ie any listed member
> that is not validated as being a member - we throw exceptions.
Ah, yes; I remember that part now. That makes sense: 2/4/6 are
a more dangerous kind of garbage, potentially misleading the reader
of the reflected data, so it needs to be blocked, and we have agreed
to throw exceptions (LEs of various sorts). That's good.
> * <p>Each listed nest member must be validated by checking its own
> * declared {@linkplain #getNestHost() nest host}. Any exceptions that occur
> * as part of this process will be thrown.
>
> * @throws LinkageError if there is any problem loading or validating
> * a nest member or its nest host
>
> In practice these will be IncompatibleClassChangeError for #2 and #6, and
> NoClassDefFoundError for #4.
>
> For #5, depending on exactly what you mean by "syntax error", it could be
> rejected at classfile parsing time; or it simply forms the wrong name and
> will then come under #2, #4 or #6.
By #5 I mean stuff that's locally malformed at class file parsing time, such
as CONSTANT_Class["foo///bar"], or even CONSTANT_String["Foo"].
That's correctly checked by parse_classfile_nest_members_attribute.
> choice is to sanitize on reflection or just pass through the garbage.
>> Given the action on 4 (just let it hit the fan), I suggest that we should
>> also let self, outsider, and duplicate entries show through on reflection.
>> Counterargument: We could sanitize the reflected data, and also sort
>> or randomize its order for good measure, to prevent buggy reflection users
>> from accidentally depending on irrelevant specifics of classfile input.
>> Remember how we got into trouble with reflected method lists, which
>> when they were reordered caused some buggy reflectors to break.
>> See also iteration order of maps and ImmutableCollections.SALT.
>> My recommendation: Just let the bad data through reflection, and
>> rely on javac to avoid putting bad data there in the first place.
>> The javadoc could be adorned with weasel words saying that response
>> to bad data (of forms 1..4 above) and sequence order are undefined
>> and may change without notice.
>
> Recommendation noted. :)
>
> Personally I'd prefer it if the array elements did form a set, but I don't
> want every correct use of this API to pay the cost of ensuring that is the
> case. So some additional words will be needed I think. I'll file a RFE to
> track that change and propose it here.
Good.
— John
