Segmentation fault when more than one l2cap connection on the same hci connection

[GoogleCodeExporter]

Hi. I have implemented a rfcomm server to use it as a serial port service using 
the example rfcomm client as the base. I tried to connect to my server from 
windows. (I used the SDP server for windows to find my device as a serial port 
and install a virtual com port) When I close the connection and wait for a 
while and then connect again there is no problem. But when I close and connect 
immediately my server stops with a segmentation fault. (It is an embedded 
system actually so I can say that a pointer stores an invalid address)

I searched and finally found that in function l2cap_signaling_handler_dispatch 
(l2cap.c) there is a for loop at the end:

// Find channel for this sig_id and connection handle    
for (it = (linked_item_t *) l2cap_channels; it ; it = it->next){
    l2cap_channel_t * channel = (l2cap_channel_t *) it;
    if (channel->handle == handle) {
        if (code & 1) {
            // match odd commands by previous signaling identifier 
            if (channel->sig_id == sig_id) {
                l2cap_signaling_handler_channel(channel, command);
            }
        } else {
            // match even commands by local channel id
            if (channel->local_cid == dest_cid) {
                l2cap_signaling_handler_channel(channel, command);
            }
        }
    }
}

Segmentation fault happens here. The problem is while looping, l2cap_channels 
link list can be changed (some channel can be removed) from the 
l2cap_signaling_handler_channel function. And in my situation (when there is 
more than one channel) it changes.

I tried to put "break;" after l2cap_signaling_handler_channel calls like this 
and it works now (although I didn't test very much):

// Find channel for this sig_id and connection handle    
for (it = (linked_item_t *) l2cap_channels; it ; it = it->next){
    l2cap_channel_t * channel = (l2cap_channel_t *) it;
    if (channel->handle == handle) {
        if (code & 1) {
            // match odd commands by previous signaling identifier 
            if (channel->sig_id == sig_id) {
                l2cap_signaling_handler_channel(channel, command);
                break;
            }
        } else {
            // match even commands by local channel id
            if (channel->local_cid == dest_cid) {
                l2cap_signaling_handler_channel(channel, command);
                break;
            }
        }
    }
}

I am looking forward to hear your thoughts about the fix.

Thanks

Original issue reported on code.google.com by murer...@gmail.com on 9 Sep 2010 at 9:48

[GoogleCodeExporter]

As a note when I connect to my device from windows first it does a sdp search 
and then connect to my rfcomm server. It does the sdp search everytime. I think 
because to find which rfcomm channel my service uses.

Original comment by murer...@gmail.com on 9 Sep 2010 at 9:53

[GoogleCodeExporter]

hi. thanks for reporting this. I recently had to rewrite multiple places where 
I iterate over a list and remove elements, but do some processing first. I 
guess you're right here: when an element is removed/freed, the it->next access 
will fail. I'll look into & fix this soon.

congrats on writing a rfcomm server. so far, my rfcomm implementation is the 
most complex so far, with about 1.5 k LOC, compared to 1k for L2CAP and 0.5 for 
HCI. And it still doesn't handle all cases, I'm afraid. OS X also does an SDP 
scan on every connection btw.

Original comment by matthias.ringwald@gmail.com on 10 Sep 2010 at 7:57

[GoogleCodeExporter]

This issue was closed by revision r934.

Original comment by matthias.ringwald@gmail.com on 10 Sep 2010 at 7:36

• Changed state: Fixed

[GoogleCodeExporter]

your fix is fine. there should only be a single channel addressed, so jumping 
out of the loop is both correct and fixes the problem

Original comment by matthias.ringwald@gmail.com on 10 Sep 2010 at 7:46

[GoogleCodeExporter]

Hi again, sorry for being late on this one. I found this problem in two other 
places. First one is here and this is my fix:

static void l2cap_handle_connection_failed_for_addr(bd_addr_t address, uint8_t 
status){
    linked_item_t *it;
    uint8_t loop_again;
    do {
    loop_again = 0;
        for (it = (linked_item_t *) l2cap_channels; it ; it = it->next){
            l2cap_channel_t * channel = (l2cap_channel_t *) it;
            if ( ! BD_ADDR_CMP( channel->address, address) ){
                if (channel->state == L2CAP_STATE_CLOSED) {
                    // failure, forward error code
                    l2cap_emit_channel_opened(channel, status);
                    // discard channel
                    linked_list_remove(&l2cap_channels, (linked_item_t *) channel);
                    free (channel);
                    loop_again = 1;
            break;
            }
        }
    }
    } while (loop_again);
}

Second one is here with my similar fix:

void l2cap_event_handler( uint8_t *packet, uint16_t size ){
    uint8_t loop_again;

    ...

    case HCI_EVENT_DISCONNECTION_COMPLETE:
        // send l2cap disconnect events for all channels on this handle
        handle = READ_BT_16(packet, 3);
        // only access next element to allows for removal

    do {
        loop_again = 0;
        for (it = (linked_item_t *) &l2cap_channels; it->next ; it = it->next){
            l2cap_channel_t * channel = (l2cap_channel_t *) it->next;
            if ( channel->handle == handle ){
                // update prev item before free'ing next element
                it->next = it->next->next;
                l2cap_finialize_channel_close(channel);
                loop_again = 1;
            break;
            }
        }
    } while (loop_again);

        break;

    ...
}

Thanks

Original comment by murer...@gmail.com on 13 Sep 2010 at 7:25

[GoogleCodeExporter]

hi. thanks for finding more. (After several tries...) I've settled on a while 
loop that handles iteration and deletion. I'll use that for the 2 places your 
found, too, to get it consistent. I'm close to writing a list iterator that 
calls a function for every element and handles deletions properly. .. .maybe 
later.

Original comment by matthias.ringwald@gmail.com on 13 Sep 2010 at 8:12

[GoogleCodeExporter]

hey! I've already fixed those two cases in revision r892: 
http://code.google.com/p/btstack/source/detail?r=892

Original comment by matthias.ringwald@gmail.com on 13 Sep 2010 at 8:33

[GoogleCodeExporter]

Sorry for that, I wish I had seen those fix before, so both me and you did not 
have to waste time. I had checked out before that revision and port that to arm 
and after that I looked to new revisions but missed that one. Sorry again and 
thank you.

Original comment by murer...@gmail.com on 14 Sep 2010 at 6:35

[GoogleCodeExporter]

hi. no problem. the other two bugs were real, and the SDP too. I just got my 
"iPhone as Bluetooth Keyboard" working with a PS3 after fixing SDP. :)

What are you working on? Is this commercial private? (you may answer in private 
if you like :)

Original comment by matthias.ringwald@gmail.com on 14 Sep 2010 at 7:41