TM4C129ENCPDT: NIMUPPP.C Code Issue

Part Number: TM4C129ENCPDT

I have come across a double freeing of memory in the example hdlcif.c code which calls the nimuppp.c timer function.

The timer function is called from the hdlcif

/*-------------------------------------------------------------------- */
/* pppTimer() */
/* Timer function called once per second */
/*-------------------------------------------------------------------- */
void pppTimer( HANDLE hPPP )
{
    PPP_SESSION *p = (PPP_SESSION *)hPPP;

    if( !p || p->Type != HTYPE_PPP )
    {
        DbgPrintf(DBG_ERROR,"pppTimer: Bad Handle %08x",p);
        return;
    }

    /* Add reference and execute */
    if( p->InUse < (INUSE_LOCKED-1) )
    {
        p->InUse++;

        lcpTimer( p );
        authTimer( p );
        ipcpTimer( p );

        /* Remove reference */
        if( p->InUse == INUSE_IDLE )
            pppFree( p );
        else
            p->InUse--;
    }
}

The lcptimer function can call back to the hdlc event function

//--------------------------------------------------------------------
// SI Control Function
//--------------------------------------------------------------------
static void hdlcSI( HANDLE hSI, uint Msg, UINT32 Aux, PBM_Handle hPkt )
{
	HDLC_INSTANCE       *pi = (HDLC_INSTANCE *)hSI;
	uint                Offset, Size;
	UINT8               *pBuf;

	switch( Msg )
	{
			case SI_MSG_CALLSTATUS:
					// Update Connection Status
					pi->Status = (uint)Aux;
					if ( pi->Status >= SI_CSTATUS_DISCONNECT )
					{
						// Close PPP - we clear the handle to make sure we
						// only call pppFree() once. (We may get multiple
						// disconnect messages - one from each protocol.)
						if ( pi->hPPP )
						{
							HANDLE hTmp = pi->hPPP;
							pi->hPPP = 0;
							pppFree( hTmp );
						}
					}
					break;

			case SI_MSG_PEERCMAP:
					// Update CMAP for Transmit
					pi->cmap_out = Aux;
					comHDLCPeerMap( pi->port, Aux );
					break;

			case SI_MSG_SENDPACKET:
					if( !hPkt )
					{
							DbgPrintf(DBG_ERROR,"hdlcSI: No packet");
							break;
					}

					Offset = PBM_getDataOffset(hPkt);
					Size = PBM_getValidLen(hPkt);

					// Make sure packet is valid, room for protocol, room for checksum
					if ( (Offset < 4) || ((Offset + Size + 2) > PBM_getBufferLen(hPkt)) )
					{
							DbgPrintf(DBG_ERROR, "hdlcSI: Bad packet");
							PBM_free( hPkt );
							break;
					}

					// Add in 2 byte Protocol and 2 byte header. Also add in size for
					// 2 byte checksum. Note that the outgoing checksum is corrected
					// (calculated) by the serial driver.
					Offset -= 4;
					Size += 6;
					PBM_setDataOffset(hPkt, Offset );
					PBM_setValidLen(hPkt, Size );
					pBuf = PBM_getDataBuffer(hPkt) + Offset;
					*pBuf++ = 0xFF;
					*pBuf++ = 0x03;
					*pBuf++ = (UINT8)(Aux / 256);							// Upper Byte
					*pBuf++ = (UINT8)(Aux % 256);							// Lower Byte

					// Send the buffer to the serial driver
					// If LCP and the command is <= 7, then make sure we
					// use the default CMAP
					if( Aux == 0xc021 && *pBuf <= 7 )
					{
							comHDLCPeerMap( pi->port, 0xFFFFFFFF );
							comHDLCSendPkt( pi->port, hPkt );
							comHDLCPeerMap( pi->port, pi->cmap_out );
					}
					else
					{
						comHDLCSendPkt( pi->port, hPkt );
					}
					break;
	}
}

This code can then free the PPP structure on a disconnect, however the ppptimer function continues to use the original pointer and then tries to free this pointer at the end giving a double free.

My work around for this was to simply add this code to the HDLC timer function

//--------------------------------------------------------------
// hdlcTimer()
// Dispatches timer ticks to the stack's PPP module
//--------------------------------------------------------------
static void hdlcTimer( HANDLE hHDLC )
{
	HDLC_INSTANCE *pi = (HDLC_INSTANCE *)hHDLC;

	if( pi && (pi->Type == HDLC_INST_CLIENT || pi->Type == HDLC_INST_SERVER) && pi->hPPP )
	{
		pppTimer( pi->hPPP );
		if ( pi->Status >= SI_CSTATUS_DISCONNECT )
		{
			// Close PPP - we clear the handle to make sure we
			// only call pppFree() once. (We may get multiple
			// disconnect messages - one from each protocol.)
			if ( pi->hPPP )
			{
				HANDLE hTmp = pi->hPPP;
				pi->hPPP = 0;
				pppFree( hTmp );
			}
		}
	}
}

As well as the stop and free functions:

//--------------------------------------------------------------
// hdlcFree()  USER FUNCTIION
// Close HDLC Client
//--------------------------------------------------------------
void hdlcFree( HANDLE hHDLC )
{
	HDLC_INSTANCE *pi = (HDLC_INSTANCE *)hHDLC;

	if (pi)
	{
		// Enter kernel mode
		llEnter();

		if( pi && (pi->Type==HDLC_INST_CLIENT || pi->Type==HDLC_INST_SERVER)  )
		{
			// This message will close PPP
			hdlcSI( hHDLC, SI_MSG_CALLSTATUS, SI_CSTATUS_DISCONNECT, 0 );

			if ( pi->Status >= SI_CSTATUS_DISCONNECT )
			{
				// Close PPP - we clear the handle to make sure we
				// only call pppFree() once. (We may get multiple
				// disconnect messages - one from each protocol.)
				if ( pi->hPPP )
				{
					HANDLE hTmp = pi->hPPP;
					pi->hPPP = 0;
					pppFree( hTmp );
				}
			}

			// Zap the type
			pi->Type = 0;

			// Close the serial driver
			comCloseHDLC( pi->port );

			// Free the instance
			free( pi );
		}

		// Exit kernel mode
		llExit();
	}
}

void hdlcStop(HANDLE hHDLC)
{
	HDLC_INSTANCE *pi = (HDLC_INSTANCE *)hHDLC;

	if (pi)
	{
		// Enter kernel mode
		llEnter();

		if( pi && (pi->Type == HDLC_INST_CLIENT || pi->Type == HDLC_INST_SERVER)  )
		{
			// This message will close PPP
			hdlcSI( hHDLC, SI_MSG_CALLSTATUS, SI_CSTATUS_DISCONNECT, 0 );
			if ( pi->Status >= SI_CSTATUS_DISCONNECT )
			{
				// Close PPP - we clear the handle to make sure we
				// only call pppFree() once. (We may get multiple
				// disconnect messages - one from each protocol.)
				if ( pi->hPPP )
				{
					HANDLE hTmp = pi->hPPP;
					pi->hPPP = 0;
					pppFree( hTmp );
				}
			}
		}

		// Exit kernel mode
		llExit();
	}
}

This ensures the PPP pointer isn't freed by the hdlc interface until it has control of the PPP pointer itself.

Just wanted to check if this is the best way to handle this, or if really the nimuppp.c file should do a check of the hdlc PPP pointer within each step of the timer function and exit if this has cleared.