On Wed, 16 Oct 2024 23:18:42 GMT, John R Rose <jr...@openjdk.org> wrote:
>> Calvin Cheung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> @rose00 comment
>
> src/hotspot/share/classfile/modules.cpp line 697:
>
>> 695: st.print("%s%s", prefix, m);
>> 696: last_string = m;
>> 697: prefix = ",";
>
> You use comma as the separator. Folks will have various opinions, but I
> think you should use a character which is positively forbidden by the JVMS
> 4.2.3. That says, basically, that any character is fine (in the classfile
> format) EXCEPT control characters. The most visible control character is
> probably `'\n'` so I suggest using that instead of comma. Your key-strings
> will look like groups of sorted lines, one module name per line. That's
> plenty readable.
>
> If you keep comma, nothing will break, unless some smart guy figures out how
> to cook up a classfile that has a module name in it with a comma inside, that
> looks like two other legitimate modules names separated by that comma.
> Something odd could happen next. Better to avoid it from the start by using
> a truly illegal module name character.
>
> (If these were class names I'd be suggesting `';'` or `'.'` not `'\n'`
> because the rules for class names are different, in the JVMS. Again, none of
> these issues come up with inputs which are compiled from normal Java sources,
> but they can appear with craftily crafted classfiles.)
>
> https://docs.oracle.com/javase/specs/jvms/se23/html/jvms-4.html#jvms-4.2.3
Thanks for taking a look, John.
I've made the change based on your suggestion.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21553#discussion_r1803986162
