<div dir="ltr">Given that my fix is "obviously more correct" and this code is doing something pretty mysterious that only you guys understand, I'd prefer that you guys take ownership of this quick fix.  The call to checkRead is troublesome for us, but that's true even after my fix is applied.  (We would prefer the checkRead code be reverted)</div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Aug 23, 2013 at 9:48 PM, Bradford Wetmore <span dir="ltr"><<a href="mailto:bradford.wetmore@oracle.com" target="_blank">bradford.wetmore@oracle.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Martin,<br>
<br>
Your fix looks good to me, just need a test case to putback. Should be pretty straightforward to create a custom SecurityManager that throws a ACE instead of a SE during a checkRead(), and then link together.<br>
<br>
Brad<div class="im"><br>
<br>
<br>
On 8/21/2013 3:51 PM, Martin Buchholz wrote:<br>
</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
Adding Alexey Utkin, who appears to be the author of the lines I am<br>
proposing to modify.  Alexey, you are invited to take ownership of this fix.<br>
<br>
<br>
On Wed, Aug 21, 2013 at 3:43 PM, Martin Buchholz <<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a><br></div><div class="im">
<mailto:<a href="mailto:martinrb@google.com" target="_blank">martinrb@google.com</a>>> wrote:<br>
<br>
    Hi security team,<br>
<br>
    There's some code in ProcessBuilder.java to avoid leaking data in<br>
    case ProcessBuilder.start fails.<br>
    It appears to have an obvious bug, with an obvious fix.<br>
<br>
    <a href="http://cr.openjdk.java.net/~martin/webrevs/openjdk8/ProcessBuilder-checkRead/" target="_blank">http://cr.openjdk.java.net/~<u></u>martin/webrevs/openjdk8/<u></u>ProcessBuilder-checkRead/</a><br>
<br>
    checkRead is spec'ed to throw SecurityException, not<br>
    AccessControlException. If checkRead does throw SecurityException,<br>
    then start will throw the wrong exception.<br>
<br>
    Untested.<br>
<br>
    @@ -1033,9 +1033,9 @@<br>
                      // Can not disclose the fail reason for read-protected files.<br>
                      try {<br>
                          security.checkRead(prog);<br>
    -                } catch (AccessControlException ace) {<br>
    +                } catch (SecurityException e) {<br>
                          exceptionInfo = "";<br>
    -                    cause = ace;<br>
    +                    cause = e;<br>
                      }<br>
                  }<br>
                  // It's much easier for us to create a high-quality error<br>
<br>
<br>
</div></blockquote>
</blockquote></div><br></div>