The nmbd crashes right after starting up and receiving a packet from my machine, all the time everytime. This started after upgrading from 3.5.6 to 3.5.9. It seems that this crash was made possible by the addition of FD_SET related range checks to fix CVE-2011-0719. By preventing the code to operate on the invalid offsets, they're left uninitialized(?), triggering some other range-check bug, which was dormant until now. 0x08098708 in listen_for_packets (run_election=false) at nmbd/nmbd_packets.c:1958 1958 if (!FD_ISSET(sock_array[i],&r_fds)) { i = 8 listen_number = 12 (gdb) p *(int(*)[50])sock_array $13 = {8, -1, 12, 10, 9, -1, 13, 11, -401273741, 0 <repeats 41 times>}
Created attachment 6644 [details] backtrace for the nmbd 3.5.9 crash
Created attachment 6645 [details] samba-3.5.9-sock_array fix Attached is proposed bug fix. When we fill in sock_array[], we also return its size. However, consumer of the sock_array[] is interested in number of valid entries in the array, not the size of it. The number of actually filled entries could be less than size of the array as we could have filtered out some sockets. A fix is one-liner.
Patch seems to have fixed it - nmbd is still up, when before it crashed immediately after starting. Thank you for looking into this.
Comment on attachment 6645 [details] samba-3.5.9-sock_array fix I think this patch is wrong for 3.5. I'll upload a better one shortly.
Created attachment 6654 [details] A patch for v3-6-test As we should never remove a subnet record, I guess it's not critical for 3.6.0
Created attachment 6655 [details] Patch for v3-5-test Can you try and see if this patch fixes your problem too?
Created attachment 6656 [details] Patch for v3-4-test
Created attachment 6657 [details] Patch for v3-3-test Jeremy, as this fixes a bug introduced by a security release, I think we should apply this to the next 3.3 security release if there will be one.
Comment on attachment 6654 [details] A patch for v3-6-test Shouldn't this patch (the 3.6.0 one) also add the tests for subrec->nmb_sock != -1 and subrec->dgram_sock != -1 into the function create_listen_pollfds() in nmbd/nmbd_packets.c to prevent fd's set to -1 being added to the poll() set ? Jeremy.
Comment on attachment 6657 [details] Patch for v3-3-test LGTM for 3.3.next. Jeremy.
Comment on attachment 6656 [details] Patch for v3-4-test LGTM for 3.4.next. Jeremy.
Comment on attachment 6654 [details] A patch for v3-6-test For 3.6.x don't we also need git commit 1053a24a87f341fcd5578db56bc8b3962e63bb98 from master ? Metze please comment. Jeremy.
Comment on attachment 6655 [details] Patch for v3-5-test LGTM for 3.5.next. Jeremy.
(In reply to comment #12) > Comment on attachment 6654 [details] > A patch for v3-6-test > > For 3.6.x don't we also need git commit > 1053a24a87f341fcd5578db56bc8b3962e63bb98 from master ? > > Metze please comment. If you can explain me in what situation this is needed I'm fine with it, but I'm currently not seeing the need for this and it would confuse/mislead people which read this code. If it's not really needed I'd like to revert that one in master...
IMHO this makes the code clearer, as it prevents fd == -1 from ever being put into the poll array. Isn't it needed after close_subnet() has been called ? I disagree it would confuse/mislead people as I am confused mislead as to why this is *not* neccessary :-). Isn't this just standard defensive programming here ? Jeremy.
Ah - ok - I was missing the point of removal of the subrec struct from the linked list that the poll code looks at inside close_subnet(). Ok, I'm fine with you reverting this. Jeremy.
Comment on attachment 6654 [details] A patch for v3-6-test Ok, now I'm good with this as I understand it :-). This is the point of code review I guess, to make sure someone else understands the logic :-). Jeremy.
Re-assigning to Karolin for push to all branches. Karolin I'm ok with this going into 3.6.1 if this is too late for release. Jeremy.
Pushed to all branches. Closing out bug report. Thanks!