Re: [RFR]: JDK-8203182 - Release session if initialization of SunPKCS11 Signature fails

[Valerie Peng]

Hi Martin,

Thanks for covering additional scenarios.

The updated webrev looks good to me. I will sponsor your patch.

Thanks,
Valerie

On 5/24/2018 12:13 PM, Martin Balao wrote:

Hi Valerie,

Thanks for your review.

These are the cases I identified:

 * engineSign and engineVerify include a try-finally block that releases the session under any circumstances.

 * engineUpdate may leak. Session release added if failure occurs.

 * cancelOperation may leak. If session has no objects, it's killed. If it has, a "cancel operation by finishing it" code is executed. This code can fail on sign/verify operations, and a session may be leaked. It's not clear to me why this "cancel operation by finishing it" code is needed, though. Added a try-finally block anyways.

Webrev 01:

 * http://cr.openjdk.java.net/~mbalao/webrevs/8203182/8203182.webrev.01/ <http://cr.openjdk.java.net/%7Embalao/webrevs/8203182/8203182.webrev.01/>  * http://cr.openjdk.java.net/~mbalao/webrevs/8203182/8203182.webrev.01.zip <http://cr.openjdk.java.net/%7Embalao/webrevs/8203182/8203182.webrev.01.zip>

Look forward to your answer.

Kind regards,
Martin.-

On Tue, May 15, 2018 at 7:30 PM, Valerie Peng <valerie.p...@oracle.com <mailto:valerie.p...@oracle.com>> wrote:

    Hi Martin,

    Your fix only addresses the initialization case. Have you
    considered fixing the same problem under difference calls?

    Regards,
    Valerie

    On 5/14/2018 3:25 PM, Martin Balao wrote:

        Hi,

        Can I have a review for "JDK-8203182 - Release session if
        initialization of SunPKCS11 Signature fails" [1]?

         *
        http://cr.openjdk.java.net/~mbalao/webrevs/8203182/8203182.webrev.00/
        
<http://cr.openjdk.java.net/%7Embalao/webrevs/8203182/8203182.webrev.00/>
        <http://cr.openjdk.java.net/%7Embalao/webrevs/8203182/8203182.webrev.00/
        
<http://cr.openjdk.java.net/%7Embalao/webrevs/8203182/8203182.webrev.00/>>
         *
        http://cr.openjdk.java.net/~mbalao/webrevs/8203182/8203182.webrev.00.zip
        
<http://cr.openjdk.java.net/%7Embalao/webrevs/8203182/8203182.webrev.00.zip>
        
<http://cr.openjdk.java.net/%7Embalao/webrevs/8203182/8203182.webrev.00.zip
        
<http://cr.openjdk.java.net/%7Embalao/webrevs/8203182/8203182.webrev.00.zip>>

        Thanks in advanced!

        Martin.-

        --
        [1] - https://bugs.openjdk.java.net/browse/JDK-8203182
        <https://bugs.openjdk.java.net/browse/JDK-8203182>