Hi Mandy and Bob, Please review a new version of the webrev [1] that moves doPrivileged calls in jdk.internal.platform.cgroupv1.SubSystem to separate methods that throw IOException, as Mandy suggested.
Mach5 tests are still running. [1] Webrev: http://cr.openjdk.java.net/~dtitov/8226575/webrev.06/ [2] Jira issue: https://bugs.openjdk.java.net/browse/JDK-8226575 [3] CSR: https://bugs.openjdk.java.net/browse/JDK-8228428 Thank you, Daniil On 12/9/19, 1:55 PM, "Mandy Chung" <mandy.ch...@oracle.com> wrote: On 12/9/19 10:51 AM, Daniil Titov wrote: > Hi Mandy and Bob, > >> Why did you not change the exception caught in SubSystem.java:getStringValue to PrivilegedActionException from IOException >> so it’s consistent with the other get functions? > In this method both Files.newBufferedReader and return bufferedReader.readLine could throw IOException so for simplicity I just put > the whole code block in doPrivileged. On the other side I don't believe that BufferedReader.readline() requires FilePermission checks ( and tests proved that) > so we could change this implementation to the following: > > public static String getStringValue(SubSystem subsystem, String parm) { > if (subsystem == null) return null; > > try (BufferedReader bufferedReader = > AccessController.doPrivileged((PrivilegedExceptionAction<BufferedReader>) () -> { > return Files.newBufferedReader(Paths.get(subsystem.path(), parm)); > })) { > return bufferedReader.readLine(); > } catch (PrivilegedActionException | IOException e) { > return null; > } > } > > Could you please advise are you OK with it or you would like to proceed with the approach Mandy suggested to unwrap > PrivilegedActionException exception and throw the cause instead? > I think it's simpler to read and understand if the doPrivileged call is moved out as a separate method that will throw IOException as the expected functionality as suggested above. For SubSystem::getStringValue, one suggestion would be: diff --git a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java --- a/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java +++ b/src/java.base/linux/classes/jdk/internal/platform/cgroupv1/SubSystem.java @@ -29,7 +29,11 @@ import java.io.IOException; import java.math.BigInteger; import java.nio.file.Files; +import java.nio.file.Path; import java.nio.file.Paths; +import java.security.AccessController; +import java.security.PrivilegedActionException; +import java.security.PrivilegedExceptionAction; import java.util.ArrayList; import java.util.List; import java.util.Optional; @@ -90,9 +94,8 @@ public static String getStringValue(SubSystem subsystem, String parm) { if (subsystem == null) return null; - try(BufferedReader bufferedReader = Files.newBufferedReader(Paths.get(subsystem.path(), parm))) { - String line = bufferedReader.readLine(); - return line; + try { + return subsystem.readStringValue(parm); } catch (IOException e) { return null; @@ -100,6 +103,24 @@ } + private String readStringValue(String param) throws IOException { + PrivilegedExceptionAction<BufferedReader> pea = () -> Files.newBufferedReader(Paths.get(path(), param)); + try (BufferedReader bufferedReader = AccessController.doPrivileged(pea)) { + String line = bufferedReader.readLine(); + return line; + } catch (PrivilegedActionException e) { + Throwable x = e.getCause(); + if (x instanceof IOException) + throw (IOException)x; + if (x instanceof RuntimeException) + throw (RuntimeException)x; + if (x instanceof Error) + throw (Error)x; + + throw new InternalError(x); + } + } +
