On Wed, 12 Nov 2025 19:13:38 GMT, Francisco Ferrari Bihurriet
<[email protected]> wrote:
>> src/java.base/share/classes/java/security/Security.java line 288:
>>
>>> 286: "Cyclic include of '" + resolve(path)
>>> + "'");
>>> 287: }
>>> 288: } catch (IOException ignore) {}
>>
>> Not sure you want to ignore this - seems better to let this propagate and be
>> thrown as an `InternalError`.
>
> We can make this an `InternalError`, the most common failure case is one of
> the two files nonexistence. So before proceeding I want to make sure you are
> aware that this would make the following filesystem race-condition noticeable:
>
> 1. File **A** is included, _OpenJDK_ starts reading it
> 2. File **A** is deleted by an administrator who is changing the settings
> * But _OpenJDK_ keeps it open, this is possible in _Linux_
> 3. File **B** is included, _OpenJDK_ wants to check for a circular inclusion
> 4. `Files.isSameFile(path, activePath)` throws `IOException` when `path` is
> file **B** and `activePath` is file **A** (now deleted)
> 5. `IOException` isn't ignored but wrapped in an `InternalError` and thrown
>
> Current code wouldn't fail in this scenario, although I recognize it's a
> corner case. I decided to ignore the exception under the assumption that
> `Files.isSameFile(x, y)` can be treated as `false` in this context for cases
> in which either `x` or `y` is nonexistent.
Addressed in 4e7206c082e4b0152d999b32eba0063a506eff67.
>> src/java.base/share/classes/java/security/Security.java line 295:
>>
>>> 293: throws IOException {
>>> 294: boolean isRegularFile = Files.isRegularFile(path);
>>> 295: if (!isRegularFile && Files.isDirectory(path)) {
>>
>> Can a directory ever be a regular file? If not, you don't need the
>> `!isRegularFile` check.
>
> A directory is not a regular file, but we need `isRegularFile` later here:
>
> https://github.com/openjdk/jdk/blob/a8d865c4985e6660655b27df28e76882855b2087/src/java.base/share/classes/java/security/Security.java#L302
>
> So `!isRegularFile` is part of the condition to save the posible
> `Files::isDirectory` I/O operations in the most likely case (we are including
> a regular file).
>
> Previous code was also saving this I/O:
>
> https://github.com/openjdk/jdk/blob/4b315111493ac65511890bc2127489ceee693915/src/java.base/share/classes/java/security/Security.java#L268-L272
>
> I would prefer not to, but I can remove the condition and inline
> `Files::isRegularFile` on line 302:
>
>
> currentPath = Files.isRegularFile(path) ? path : null;
Addressed in 4e7206c082e4b0152d999b32eba0063a506eff67.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24465#discussion_r2572286720
PR Review Comment: https://git.openjdk.org/jdk/pull/24465#discussion_r2572286632
