[En-Nut-Discussion] Potential BugFix: tcpsock.c, incorrect pointer management with NutTcpDestroySocket

[Michael Jones]

Hi,

Sorry to say this, but the original code works fine. Yet, I see where your
problem comes from as I've stumbled over these constructs myself through out
nut/OS. The important thing is that spp is a pointer to a pointer, here used
as the pointer to the pointer linking to the currently checked object. Keep
that in mind and step through the code again and it will make sense.

Regards,
Michael

-----Original Message-----
From: en-nut-discussion-bounces at egnite.de
[mailto:en-nut-discussion-bounces at egnite.de] On Behalf Of Brett Abbott
Sent: Monday, October 02, 2006 4:09 AM
To: Ethernut User Chat (English)
Subject: [En-Nut-Discussion] Potential BugFix: tcpsock.c,incorrect pointer
management with NutTcpDestroySocket

Hi

Whilst tracking down a reliability problem, I came across what looks 
like a problem with NutTcpDestroySocket.  It doesnt appear to have been 
managing the linked list correctly.

Could I please ask those more familiar with the OS than I to review the 
original code and my proposed change to see if it is indeed a problem 
and that my proposed fix looks solid.  Of minor note, I have also 
changed the volatile pointer spp back to non volatile as it doesnt 
appear to be needed and may be a hangover from event code where this 
code may have been sourced - again please advise if this is correct.

tcpsock.c appears to have issue in current code levels as well as older 
versions (3.9.5)

Code snippet below.

Many Thanks in advance
Brett

void NutTcpDestroySocket(TCPSOCKET * sock)
{

// Bug Fix Brett Abbott 29Sep2006
//--- Old Code:
//
//    TCPSOCKET *sp;
//    TCPSOCKET *volatile *spp;
//   
//    //@@@printf ("[%04X] Calling destroy.\n", (u_short) sock);
//
//    /*
//     * Remove socket from the list.
//     */
//
//    sp = tcpSocketList;
//    spp = &tcpSocketList;
//    while (sp) {
//        if (sp == sock) {
//            *spp = sp->so_next;
//            break;
//        }
//        spp = &sp->so_next;
//        sp = sp->so_next;
//    }
//
//--- New Code:

    TCPSOCKET *sp;
    TCPSOCKET *spp; // no need to be volatile

    //@@@printf ("[%04X] Calling destroy.\n", (u_short) sock);
    printf ("[%04X] Calling destroy.\n", (u_short) sock);

    /*
     * Remove socket from the list.
     */

    sp = tcpSocketList;
    if(sp==sock) { // first record, special case, only move pointer to 
list rather than altering records

        tcpSocketList=sp->so_next;
   
    } else {

    if (sp) { // Cant assume that there is anything in the list at all, 
avoid linking to null pointer

            spp = tcpSocketList; // Set previous record to the top of 
the list

            sp = sp->so_next; // Set selection pointer to the 2nd in the 
list, if it exists.
            while (sp) {

                if (sp == sock) {

                    spp->so_next = sp->so_next;
                    break;
                }

                spp = sp;
                sp = sp->so_next;
            }
        }
    }   


//--End of Change 29Sep2006

    /*
     * Free all memory occupied by the socket.
     */
    if (sp) {

-- 
-----------------------------------------------------------------
Brett Abbott, Managing Director, Digital Telemetry Limited
Email: Brett.Abbott at digital-telemetry.com
PO Box 24 036 Manners Street, Wellington, New Zealand
Phone +64 (4) 5666-860  Mobile +64 (21) 656-144
------------------- Commercial in confidence --------------------


_______________________________________________
En-Nut-Discussion mailing list
En-Nut-Discussion at egnite.de
http://www.egnite.de/mailman/listinfo.cgi/en-nut-discussion