DHCP.c 0831- attempt to close socket but socket not open

Kernel
1

glenenglish wrote on Monday, September 05, 2016:

Found this one when I patched in the new +TCP 0831
and associated DHCP changes. an easy one to find.

in vDHCPProcess state == eWaitingSendFirstDiscover

If xApplicationDHCPHook returns at eDHCPPhase==eDHCPPhasePreDiscover
which sets xGivingUp==true

Down the bottom of vDHCPProcess

xGivingUp is checked and if true, then the socket is closed.

BUT !!!
prvInitialiseDHCP(); was never called, because of the initial eDHCPUseDefault return from the hook
so the socket never gets born.

Until I have a good look at the best way to deal with this (maybe change code path )

really needs an assert if there is an attept to close a open socket.

I have put in
/* hack by GLEN /
/ only close socket if it was opened… */
if (xDHCPData.xDHCPSocket!=NULL) {
vSocketClose( xDHCPData.xDHCPSocket );
xDHCPData.xDHCPSocket = NULL;
}

-glen

.

2

kerub wrote on Tuesday, September 06, 2016:

I think the problem is somewhere else.

The function vSocketClose do nothing if the socket is equal NULL

if( ( xSocket == NULL ) || ( xSocket == FREERTOS_INVALID_SOCKET ) )
{
	xResult = 0;
}.

The socket xDHCPData.xDHCPSocket is set first to NULL after first attempt to close the socket.

The problem is the stucture xDHCPData has no defined values at the start of the system.

static DHCPData_t xDHCPData;

So it does’t guarantiee that xDHCPData.xDHCPSocket is NULL before first use.

In a static analysis of the code it would be reported as an error.

Andrzej

3

richard_damon wrote on Tuesday, September 06, 2016:

Point of information. xDHCPData above is declared as a static variable, so the C language defines that it will be zero initialized at startup, so pointer members should be NULL. (If it was declared in block scope with auto duration, then yes, it doesn’t get initialized, but file/global scoped variables are supposed to be zero initialized if not otherwise initiaized).

There are some embeded enviroments where this doesn’t happen, but those are non-conforming to the standard.

4

heinbali01 wrote on Thursday, September 08, 2016:

Glen, thanks, you are totally right. We will change it.

Andrzej points out that it doesn’t matter because NULL may be closed. Indeed the public API FreeRTOS_closesocket() has this check:

	if( ( xSocket == NULL ) || ( xSocket == FREERTOS_INVALID_SOCKET ) )
	{
		xResult = 0;
	}

However, we recently made a change in FreeRTOS_DHCP.c to use vSocketClose(). That’s the internal version of FreeRTOS_closesocket(). The internal version does not check for NULL or FREERTOS_INVALID_SOCKET (~0U).

I agree with Richard Damon that the xDHCPData will be zero’d after start-up, but if FreeRTOS_socket() fails, the socket pointer could become FREERTOS_INVALID_SOCKET.

The function prvCreateDHCPSocket() still has an assert:

configASSERT( ( xDHCPData.xDHCPSocket != FREERTOS_INVALID_SOCKET ) );

that we better clean up now.

Please find attached a new version of FreeRTOS_dhcp.c without the configASSERT().

5

heinbali01 wrote on Friday, September 09, 2016:

A note about the new FreeRTOS_DHCP.c in the previous post: you need the latest Labs release to be able to compile it. In fact the current 160908 release already contains the above FreeRTOS_DHCP.c.

The naming of the DHCP-hook and some enums have changed since the earlier 160112 release. The essence is still the same though.

See here for a detailed description.

The DHCP-hook define has changed:

s/ipconfigDHCP_USES_USER_HOOK/ipconfigUSE_DHCP_HOOK/

When the old define is still in seen, an #error will be issued.

Some types and values have changed:

s/eDHCPCallbackQuestion_t/eDHCPCallbackPhase_t/
s/eDHCPCallbackAnswer_t/eDHCPCallbackAnswer_t/
s/eDHCPOffer/eDHCPPhasePreDiscover/
s/eDHCPRequest/eDHCPPhasePreRequest/
s/eQuestion/ePhase/

And finally the hook has changed:

BaseType_t xApplicationDHCPUserHook( eDHCPCallbackQuestion_t eQuestion,
    uint32_t ulIPAddress, uint32_t ulNetMask );
eDHCPCallbackAnswer_t xApplicationDHCPHook( eDHCPCallbackPhase_t eDHCPPhase,
    uint32_t ulIPAddress );

The parameter ulNetMask was dropped. It can be retrieved by calling FreeRTOS_GetAddressConfiguration().

Sorry for this inconvenience. In the 160112 release, the DHCP hook was still a new experimental feature. It worked OK but the naming was not very clear.
We think that it will be more clear now to new users what the hook does and how it works.

6

glenenglish wrote on Saturday, September 10, 2016:

Hi Hein , Andrzej and Richard

thanks for all the responses and work you have done.

I have not found any other problems with 0831 to date. I will download 0908 tomorrow and patch that in. ( I just copy over and do an SVN DIFF) . 0831 is now in a live system…running abt 900 pps and 80% utilization of the F4 in DSP…

I have some fixes for the TFTP server from 0831 latest so I will post that in the forum as some input that tomorrow. I have the tftp server working, loading data into external flash,when booting my xilinx fpga…The tricky bit was that my spi flash shared the spi with the ethernet :slight_smile: …solid as a rock.

best regards