InitSysContext function might lead to memory leakage

[edonyzpc]

        if( rval == TSS2_RC_SUCCESS )
            return sysContext;
        else
            return 0;

Source code above in InitSysContext function in file TPM2.0-TSS/common/syscontext.c lead to memory leakage for not free the malloc memory of variable sysContext.

/****************************/
/* function InitSysContext */
/****************************/
TSS2_SYS_CONTEXT *InitSysContext(
    UINT16 maxCommandSize,
    TSS2_TCTI_CONTEXT *tctiContext,
    TSS2_ABI_VERSION *abiVersion
 )
{
    UINT32 contextSize;
    TSS2_RC rval;
    TSS2_SYS_CONTEXT *sysContext;

    // Get the size needed for system context structure.
    contextSize = Tss2_Sys_GetContextSize( maxCommandSize );

    // Allocate the space for the system context structure.
    sysContext = malloc( contextSize );

    if( sysContext != 0 )
    {
        // Initialized the system context structure.
        rval = Tss2_Sys_Initialize( sysContext, contextSize, tctiContext, abiVersion );

        if( rval == TSS2_RC_SUCCESS )
            return sysContext;
        else
            return 0;
    }
    else
    {
        return 0;
    }
}

P.S. Also in file TPM2.0-TSS/resourcemgr/resourcemgr.c, there is the same problem in SockServer function and the source codes are as follows. In this while loops, resourcemgr will apply for the memory unlimited if failed to connect to the socket or failed to detach the thread.

do
    {
        cmdServerStruct = (*rmMalloc)( sizeof( SERVER_STRUCT ) );
        if( cmdServerStruct == 0 )
        {
            continue;
        }

        cmdServerStruct->connectSock = accept( serverStruct->connectSock, 0, 0 );

        if( cmdServerStruct->connectSock == INVALID_SOCKET )
        {
            printf( "Accept failed.  Error is 0x%x\n", WSAGetLastError() );
            continue;
        }

        printf( "Accept socket:  0x%x\n", cmdServerStruct->connectSock );

        cmdServerStruct->serverFn = serverStruct->serverFn;
        cmdServerStruct->serverName = serverStruct->serverName;

        printf( "Resource Manager %s Server accepted client\n", serverStruct->serverName );

#ifdef  _WIN32
        cmdServerStruct->threadHandle = CreateThread( NULL, 0,
                (LPTHREAD_START_ROUTINE)serverStruct->serverFn,
                (LPVOID)cmdServerStruct, 0, NULL );
        if( cmdServerStruct->threadHandle == NULL )
        {
            closesocket( cmdServerStruct->connectSock );
            (*rmFree)( cmdServerStruct );
            printf( "Resource Mgr failed to create OTHER command server thread.  Exiting...\n" );
            continue;
        }
#elif __linux || __unix
        rval = pthread_create( &cmdServerStruct->threadHandle, 0, (void *)serverStruct->serverFn, cmdServerStruct );
        if( rval != 0 )
        {
            closesocket( cmdServerStruct->connectSock );
            (*rmFree)( cmdServerStruct );
            printf( "Resource Mgr failed to create OTHER command server thread, error #%d.  Exiting...\n", rval );
            continue;
        }
        rval = pthread_detach( cmdServerStruct->threadHandle );
        if( rval != 0 )
        {
            printf( "Resource Mgr failed to detach a server thread, error #%d.  Warning and continue...\n", rval );
        }
#else
#error Unsupported OS--need to add OS-specific support for threading here.
#endif
    }
    while( continueServer );

[flihp]

Looks like a bug to me. Not in the library though, this would only effect the test code and the resource manager (which already has a lot of existing problems).

[edonyzpc]

@flihp Thank you for confirmation. And this function is not in the library ?

TSS2_SYS_CONTEXT *InitSysContext(
    UINT16 maxCommandSize,
    TSS2_TCTI_CONTEXT *tctiContext,
    TSS2_ABI_VERSION *abiVersion
 )

[flihp]

I misspoke there. Clarification: the InitSysContext is a convenience function used in the test programs and the resorucemgr. It's not part of the SAPI spec. We shouldn't be packaging / exposing it through the libraries we distribute. Likely we are though. I made an issue a long time back to cleanup symbols that we shouldn't be exposing in the libraries but that needs to be decomposed to actionable issues.

Either way this code needs to be fixed. The symbol stuff will be cleaned up in the next release.

[flihp]

@edonyM I threw together a PR over lunch. Wanna take a look for me and ACK if it's what you had in mind? https://github.com/01org/TPM2.0-TSS/pull/333

[edonyzpc]

@flihp I have checked you PR, it seems good to fix this bug. But I think there is another one you might miss, in function UINT32 WINAPI SockServer( LPVOID servStruct ) [LINE 2708 - LINE 2712]

        if( cmdServerStruct->connectSock == INVALID_SOCKET )
        {
            printf( "Accept failed.  Error is 0x%x\n", WSAGetLastError() );
            continue;
        }

This judgement means if fail to connect with the socket, connect it again. In the do{...}while(...), you are applying for memory and do not release it before you re-connect.

[flihp]

Good catch. The logic in this function is still pretty ugly but I think this takes care of the possible memory leak.

[edonyzpc]

I have checked your PR again and that's it. Personally, they were fixed. Of course, look forward to the improvement of resourcemgr.

Thank you for your time. Close the issue.