Updated webrev at http://cr.openjdk.java.net/~weijun/6722928/webrev.08/
@build-dev guys, I realize I've never included you in this code review. Please take a look. @Valerie, All comments accepted except for one (see below). In fact, I think I found a bug in gss_release_context that it might release a cred handle passed in, so I add a isLocalCred flag. However, I test it on my local Windows and it seems the same handle can be FreeCredentialsHandle and then used and then freed again. > On Jun 7, 2019, at 10:45 AM, Valerie Peng <valerie.p...@oracle.com> wrote: > > Hi, Max, > > <gssapi.h> > > - line 424: the "(used to be const)" comment can now be removed. > > <sspi.cpp> > > - line 396-403: on line 338, there is no need to go to err as no memory has > been allocated. What happens when jumping to err but the variables, i.e. > value and name, have not been declared? Line 400-401 seems not used as there > is no more goto err after line 391. > > - line 528: the size of buffer here is 4*len + 1, but then when calling > WideCharToMultiByte, the 6th argument is len. Seems inconsistent? line 534: > shouldn't we free "buffer" here? > > - line 596: free cred allocated on line 588? line 610 and 617: free cred and > cred->phCredK? line 638 and 644, 648 and 653: free cred, cred->phCredK and > cred->phCredS? > > - line 829: free the context handle allocated on line 807? line 879: free > newCred? line 901: no memory de-allocation before returning error? line 921: > seems redundant given line 918. > > - line 1071: based on gss api doc, context_handle should be set to > GSS_C_NO_CONTEXT after deletion. > > - line 1333: what about secBuff[1].pvBuffer? According to the DecryptMessage spec, "The encrypted message is decrypted in place, overwriting the original contents of its buffer". I've printed out both secBuff[0].pvBuffer and secBuff[1].pvBuffer after the decryption and the latter is indeed inside the former. > > - line 1390, 1393, 1397: call gss_release_oid_set before returning failure? > > - line 1471: should we return an error code here when FormatMessage() call > failed? > > Rest looks fine. > > Thanks, > > Valerie Thanks, Max [1] https://docs.microsoft.com/en-us/windows/desktop/api/sspi/nf-sspi-decryptmessage > > On 6/4/2019 2:52 AM, Weijun Wang wrote: >> I uploaded an updated webrev in place. The only changes are: >> >> 1. s/SSPI_TRACE/SSPI_BRIDGE_TRACE/ in sspi.cpp >> 2. Several copyright year updates. >> 3. Remove one unused import. >> >> Thanks, >> Max >> >>> On May 30, 2019, at 11:18 AM, Weijun Wang <weijun.w...@oracle.com> wrote: >>> >>> Here is the latest webrev >>> >>> http://cr.openjdk.java.net/~weijun/6722928/webrev.07/
