Re: RFR: 8282077: PKCS11 provider C_sign() impl should handle CKR_BUFFER_TOO_SMALL error [v2]
On Sat, 19 Feb 2022 00:07:34 GMT, Valerie Peng wrote: >> Could someone please help review this trivial change? This is to add an >> error handling for the potential CKR_BUFFER_TOO_SMALL error when calling >> C_Sign(). Since none of the supported signature algorithms trigger this >> error as the default buffer size is large enough, this is more for >> consistency sake. No new regression test for this and thus the @noreg-hard >> label. >> >> Thanks, >> Valerie > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > free allocated 'ckpData' upon OOM. Thanks for the review! Will proceed with integration once the checks completed... - PR: https://git.openjdk.java.net/jdk/pull/7540
Re: RFR: 8282077: PKCS11 provider C_sign() impl should handle CKR_BUFFER_TOO_SMALL error [v2]
On Sat, 19 Feb 2022 00:07:34 GMT, Valerie Peng wrote: >> Could someone please help review this trivial change? This is to add an >> error handling for the potential CKR_BUFFER_TOO_SMALL error when calling >> C_Sign(). Since none of the supported signature algorithms trigger this >> error as the default buffer size is large enough, this is more for >> consistency sake. No new regression test for this and thus the @noreg-hard >> label. >> >> Thanks, >> Valerie > > Valerie Peng has updated the pull request incrementally with one additional > commit since the last revision: > > free allocated 'ckpData' upon OOM. Marked as reviewed by mikael (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7540
Re: RFR: 8282077: PKCS11 provider C_sign() impl should handle CKR_BUFFER_TOO_SMALL error [v2]
> Could someone please help review this trivial change? This is to add an error > handling for the potential CKR_BUFFER_TOO_SMALL error when calling C_Sign(). > Since none of the supported signature algorithms trigger this error as the > default buffer size is large enough, this is more for consistency sake. No > new regression test for this and thus the @noreg-hard label. > > Thanks, > Valerie Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: free allocated 'ckpData' upon OOM. - Changes: - all: https://git.openjdk.java.net/jdk/pull/7540/files - new: https://git.openjdk.java.net/jdk/pull/7540/files/19d1451a..67d70e9d Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7540&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7540&range=00-01 Stats: 2 lines in 1 file changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7540.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7540/head:pull/7540 PR: https://git.openjdk.java.net/jdk/pull/7540
Re: RFR: 8282077: PKCS11 provider C_sign() impl should handle CKR_BUFFER_TOO_SMALL error
On Fri, 18 Feb 2022 22:10:17 GMT, Mikael Vidstedt wrote:
>> Could someone please help review this trivial change? This is to add an
>> error handling for the potential CKR_BUFFER_TOO_SMALL error when calling
>> C_Sign(). Since none of the supported signature algorithms trigger this
>> error as the default buffer size is large enough, this is more for
>> consistency sake. No new regression test for this and thus the @noreg-hard
>> label.
>>
>> Thanks,
>> Valerie
>
> src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_sign.c line 151:
>
>> 149: if (bufP == NULL) {
>> 150: throwOutOfMemoryError(env, 0);
>> 151: return NULL;
>
> Does ckpData need to be freed here?
Yes, good catch. I will change it with a cleanup label and jump to the cleanup
label as in other methods.
-
PR: https://git.openjdk.java.net/jdk/pull/7540
Re: RFR: 8282077: PKCS11 provider C_sign() impl should handle CKR_BUFFER_TOO_SMALL error
On Fri, 18 Feb 2022 21:52:59 GMT, Valerie Peng wrote:
> Could someone please help review this trivial change? This is to add an error
> handling for the potential CKR_BUFFER_TOO_SMALL error when calling C_Sign().
> Since none of the supported signature algorithms trigger this error as the
> default buffer size is large enough, this is more for consistency sake. No
> new regression test for this and thus the @noreg-hard label.
>
> Thanks,
> Valerie
src/jdk.crypto.cryptoki/share/native/libj2pkcs11/p11_sign.c line 151:
> 149: if (bufP == NULL) {
> 150: throwOutOfMemoryError(env, 0);
> 151: return NULL;
Does ckpData need to be freed here?
-
PR: https://git.openjdk.java.net/jdk/pull/7540
RFR: 8282077: PKCS11 provider C_sign() impl should handle CKR_BUFFER_TOO_SMALL error
Could someone please help review this trivial change? This is to add an error handling for the potential CKR_BUFFER_TOO_SMALL error when calling C_Sign(). Since none of the supported signature algorithms trigger this error as the default buffer size is large enough, this is more for consistency sake. No new regression test for this and thus the @noreg-hard label. Thanks, Valerie - Commit messages: - 8282077: PKCS11 provider C_sign() impl should handle CKR_BUFFER_TOO_SMALL error Changes: https://git.openjdk.java.net/jdk/pull/7540/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7540&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282077 Stats: 11 lines in 1 file changed: 10 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7540.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7540/head:pull/7540 PR: https://git.openjdk.java.net/jdk/pull/7540
