Memory leak during nbns/llmnr hostname resolution

niconrok · March 23, 2021, 10:01am

Hello,

I was experiencing a memory leak during nbns/llmnr host name resolution of my device. I found out that the reply message to the resolve request is used with a parameter not to free the descriptor after sending it . Why should the memory for a dns reply not be freed after sending it?

Thats the function:
vReturnEthernetFrame( pxNetworkBuffer, pdFALSE );

Called in prvReplyDNSMessage(…) of the source file FreeRTOS_DNS.c.

To solve the issue I set pdFALSE to pdTRUE. So my output function will free the buffer descriptor.

htibosch · March 23, 2021, 3:34pm

@niconrok : thank you very much for reporting this!

I can not replicate the problem with LLMNR, but I do see a buffer leak when the application replies positively to an NBNS request.

I do use NBNS, but I never saw any leak. The reason is that I use mostly BufferAllocation_1.c.
Lesson learned: I also test with BufferAllocation_2.c!

Here you find PR #215: Repair buffer leak when replying to NBNS requests.

You will find the new repaired FreeRTOS_DNS.c here. Maybe you want to try it out? Thanks

niconrok · March 23, 2021, 4:16pm

Thank you! That solves also the problem. I will have a look on mDNS since LLMNR is not recommended for security reasons.

htibosch · March 23, 2021, 11:39pm

You wrote:

vReturnEthernetFrame( pxNetworkBuffer, pdFALSE );

To solve the issue I set pdFALSE to pdTRUE.

And also:

That solves also the problem

Your proposed change will not work properly in projects where BufferAllocation_1.c is used. Explanation:

BufferAllocation_1.c

When a LLMNR or NBNS request must be answered, the current network buffer is re-used to send a reply. Later on, the network buffer is released by the caller.

BufferAllocation_2.c

The reply is longer than the request, and the network buffer is just big enough to hold the request. In order to reply, a new network buffer must be created, which is a bit longer.
This new buffer is called pxNewBuffer, and must be release after use ( in two locations, LLMNR and NBNS ):

if( pxNewBuffer != NULL )
{
    vReleaseNetworkBufferAndDescriptor( pxNewBuffer );
}

niconrok · March 24, 2021, 6:07am

I understand, thanks a lot for your proper explantion. I will do the change accordingly to yours. Altough I don’t use BufferAllocation_1.c it is much better to keep it compatible to the other allocation styles, like your way.

htibosch · March 24, 2021, 7:45am

I would be grateful if you can report back if it works well for you ( while using BufferAllocation_2.c ). Always good to have multiple testers.

niconrok · March 24, 2021, 9:12am

Just checked it but pxNewBuffer is out of scope.

github.com

htibosch/FreeRTOS-Plus-TCP/blob/a1aaa347dd04fbc91f81d7223c771e3242aa1366/FreeRTOS_DNS.c#L1823


uint16_t usLength;
DNSMessage_t * pxMessage;
NBNSAnswer_t * pxAnswer;

/* Someone is looking for a device with ucNBNSName,
 * prepare a positive reply. */
pxNetworkBuffer = pxUDPPayloadBuffer_to_NetworkBuffer( pucUDPPayloadBuffer );

if( ( xBufferAllocFixedSize == pdFALSE ) && ( pxNetworkBuffer != NULL ) )
{
    NetworkBufferDescriptor_t * pxNewBuffer;

    /* The field xDataLength was set to the total length of the UDP packet,
     * i.e. the payload size plus sizeof( UDPPacket_t ). */
    pxNewBuffer = pxDuplicateNetworkBufferWithDescriptor( pxNetworkBuffer, pxNetworkBuffer->xDataLength + sizeof( NBNSAnswer_t ) );

    if( pxNewBuffer != NULL )
    {
        pucUDPPayloadBuffer = &( pxNewBuffer->pucEthernetBuffer[ sizeof( UDPPacket_t ) ] );
        pxNetworkBuffer = pxNewBuffer;
    }

github.com

htibosch/FreeRTOS-Plus-TCP/blob/a1aaa347dd04fbc91f81d7223c771e3242aa1366/FreeRTOS_DNS.c#L1875


                        vSetField16( pxAnswer, NBNSAnswer_t, usNbFlags, dnsNBNS_NAME_FLAGS );
                        vSetField32( pxAnswer, NBNSAnswer_t, ulIPAddress, FreeRTOS_ntohl( *ipLOCAL_IP_ADDRESS_POINTER ) );
                    #else
                        ( void ) pxAnswer;
                    #endif

                    usLength = ( uint16_t ) ( sizeof( NBNSAnswer_t ) + ( size_t ) offsetof( NBNSRequest_t, usType ) );

                    prvReplyDNSMessage( pxNetworkBuffer, ( BaseType_t ) usLength );

                    if( pxNewBuffer != NULL )
                    {
                        vReleaseNetworkBufferAndDescriptor( pxNewBuffer );
                    }
                }
            }
        }
    }

#endif /* ipconfigUSE_NBNS */
/*-----------------------------------------------------------*/

To get it into the same scope I would declare pxNewBuffer at line 1816. Tested it and it works.

kanherea · April 1, 2021, 7:50pm

The PR that was created by Hein was merged in the FreeRTOS+TCP repository.

Thanks for pointing this out @niconrok.

Hermiston · April 16, 2021, 3:31am

I am facing the same problem. Please give some solution related my issue.
Thanks in advance.

PaulB-AWS · April 28, 2021, 7:26am

@Hermiston Please take a look at the Pull-Request linked above: Repair buffer leak when replying to NBNS requests by htibosch · Pull Request #215 · FreeRTOS/FreeRTOS-Plus-TCP · GitHub. It has been merged into the main branch, so you can get the latest version from GitHub - FreeRTOS/FreeRTOS-Plus-TCP: FreeRTOS-Plus-TCP library repository. +TCP files only. Submoduled into https://github.com/FreeRTOS/FreeRTOS and various other repos..