
btstack - issue #76
Segmentation fault when more than one l2cap connection on the same hci connection
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
Comment #1
Posted on Sep 9, 2010 by Quick RabbitAs 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.
Comment #2
Posted on Sep 10, 2010 by Swift Oxhi. 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.
Comment #3
Posted on Sep 10, 2010 by Swift OxThis issue was closed by revision r934.
Comment #4
Posted on Sep 10, 2010 by Swift Oxyour fix is fine. there should only be a single channel addressed, so jumping out of the loop is both correct and fixes the problem
Comment #5
Posted on Sep 13, 2010 by Quick RabbitHi 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
Comment #6
Posted on Sep 13, 2010 by Swift Oxhi. 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.
Comment #7
Posted on Sep 13, 2010 by Swift Oxhey! I've already fixed those two cases in revision r892: http://code.google.com/p/btstack/source/detail?r=892
Comment #8
Posted on Sep 14, 2010 by Quick RabbitSorry 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.
Comment #9
Posted on Sep 14, 2010 by Swift Oxhi. 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 :)
Status: Fixed
Labels:
Type-Defect
Priority-Medium