On Tue, 15 Oct 2024 16:34:06 GMT, David M. Lloyd <d...@openjdk.org> wrote:
>> While making `LogManager.checkAccess` be a no-op might be more convenient, >> it could unconditionally >> permit operations that formerly required a permission check: clearly a bad >> idea. Always throwing a `SecurityException` is the safest option. > >> While making `LogManager.checkAccess` be a no-op might be more convenient, >> it could unconditionally permit operations that formerly required a >> permission check: clearly a bad idea. Always throwing a `SecurityException` >> is the safest option. > > It's not about convenience _or_ safety; this part of the change has a > provably flawed logical basis. > > These methods would no longer called from within the JDK after this change. > All three of these methods were already previously defined to be a no-op when > no security manager was installed (specifically when > `System.getSecurityManager() == null`). Since no security manager may be > installed after this change, this method will always return `null`. Thus, a > no-op is still the most correct behavior and does not permit any operation > that previously required a permission check (since it was already a no-op any > time no security manager was installed, which will now be the only possible > scenario). Therefore it is provably no safer to throw `SecurityException` > here, since this will only prompt library developers to introduce the > workaround I posted above when their tests fail, yielding the exact same > result (except with a minor inconvenience to library developers). > > Either way is fine (as I said, the workaround is trivial), but IMO it's best > to be conscious of the correct reasoning lest flawed assumptions _do_ end up > enabling the introduction of unsafe changes elsewhere in the code. We don't > have to make any assumptions about safety or previous behavior because it's > all statically provable. I see your point now. We have strived to preserve compatibility with libraries that follow well known code patterns as described in the JEP, so I will discuss this with others who are more familiar with the compatibility risk of making this change. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/21498#discussion_r1801595566
