On Fri, 7 Nov 2025 16:31:24 GMT, Sean Mullan <[email protected]> wrote:
>> Francisco Ferrari Bihurriet has updated the pull request incrementally with
>> one additional commit since the last revision:
>>
>> Detect cyclic includes with Files::isSameFile
>>
>> checkCyclicInclude() is invoked after we successfully get an InputStream
>> for the path to avoid skipping the same IOException several times inside
>> checkCyclicInclude() if the path doesn't exist.
>>
>> Also, perform symlinks resolution only in the following cases:
>> • When we need to resolve a relative include
>> • For clarity to the user in logging messages
>> • For clarity to the user in exception messages
>>
>> In the first case, the resolution is a requirement, in the last two
>> cases it is a nice-to-have. But given the last two are exceptional
>> cases anyway, we let any resolution error bubble up.
>
> src/java.base/share/classes/java/security/Security.java line 286:
>
>> 284: if (Files.isSameFile(path, activePath)) {
>> 285: throw new InternalError(
>> 286: "Cyclic include of '" + resolve(path) +
>> "'");
>
> Why try to resolve the path for an exception message? If that causes an
> exception an `InternalError` will be thrown and this error message will be
> lost, making it harder to debug.
Was just for a nicer error message, but I agree it could make things even
harder to debug than an unresolved path.
I will be changing this, adjusting the test case and re-testing.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24465#discussion_r2519391452
