Bug in ProcessBuilder.

[Martin Buchholz]

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)


On Fri, Aug 23, 2013 at 9:48 PM, Bradford Wetmore <
bradford.wetmore at oracle.com> wrote:

> Martin,
>
> 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.
>
> Brad
>
>
>
> On 8/21/2013 3:51 PM, Martin Buchholz wrote:
>
>> Adding Alexey Utkin, who appears to be the author of the lines I am
>> proposing to modify.  Alexey, you are invited to take ownership of this
>> fix.
>>
>>
>> On Wed, Aug 21, 2013 at 3:43 PM, Martin Buchholz <martinrb at google.com
>> <mailto:martinrb at google.com>> wrote:
>>
>>     Hi security team,
>>
>>     There's some code in ProcessBuilder.java to avoid leaking data in
>>     case ProcessBuilder.start fails.
>>     It appears to have an obvious bug, with an obvious fix.
>>
>>     http://cr.openjdk.java.net/~**martin/webrevs/openjdk8/**
>> ProcessBuilder-checkRead/<http://cr.openjdk.java.net/~martin/webrevs/openjdk8/ProcessBuilder-checkRead/>
>>
>>     checkRead is spec'ed to throw SecurityException, not
>>     AccessControlException. If checkRead does throw SecurityException,
>>     then start will throw the wrong exception.
>>
>>     Untested.
>>
>>     @@ -1033,9 +1033,9 @@
>>                       // Can not disclose the fail reason for
>> read-protected files.
>>                       try {
>>                           security.checkRead(prog);
>>     -                } catch (AccessControlException ace) {
>>     +                } catch (SecurityException e) {
>>                           exceptionInfo = "";
>>     -                    cause = ace;
>>     +                    cause = e;
>>                       }
>>                   }
>>                   // It's much easier for us to create a high-quality
>> error
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20130826/7e5b4b39/attachment.htm>