Bug 10546 - Samba chokes on trans2 response if max_send is 0
Samba chokes on trans2 response if max_send is 0
Status: RESOLVED DUPLICATE of bug 10422
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
All All
: P5 normal
: ---
Assigned To: Samba QA Contact
Samba QA Contact
Depends on:
  Show dependency treegraph
Reported: 2014-04-11 01:33 UTC by Brian Campbell
Modified: 2014-04-11 21:25 UTC (History)
0 users

See Also:

Log of Samba dying (496.40 KB, text/plain)
2014-04-11 01:33 UTC, Brian Campbell
no flags Details
Packet capture of negotiation of a max_buf of 0 and connection dying (15.40 KB, application/vnd.tcpdump.pcap)
2014-04-11 01:34 UTC, Brian Campbell
no flags Details
Capture of negotiation succeeding after dropping "max xmit" to 65535 (12.80 KB, application/vnd.tcpdump.pcap)
2014-04-11 01:35 UTC, Brian Campbell
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Brian Campbell 2014-04-11 01:33:24 UTC
Created attachment 9841 [details]
Log of Samba dying

I was testing the Thursby DAVE Mac OS X SMB Client (I reproduced it with DAVE 9.0.2) with Samba 4.1.3, and ran into a problem where I just couldn't mount a particular share, as smbd would crash every time we tried to connect.

After taking a look at some Samba logs and packet captures of the issue, attached, it looks like the problem is because I was setting "max xmit = 65536". In the "Negotiate Protocol Resonse (0x72)" message that the server sends, it was sending this max buffer size to the client; 0x0100. DAVE was then sending a "Session Setup AndX Request (0x73)" that included a Max Buffer of 0, as this message only has a 16 bit field for the max buffer size (why it's a 32 bit field in the "negotiate protocol" response but a 16 bit field in the "Session Setup AndX" is beyond me). It looks like Samba accepted that value of 0, then died in this snippet of trans2.c after receiving a later trans2 request, as it calculated a useable_space of -60 for a max_send size of 0.

    /* When sending params and data ensure that both are nicely aligned */
    /* Only do this alignment when there is also data to send - else
        can cause NT redirector problems. */

    if (((params_to_send % 4) != 0) && (data_to_send != 0))
        data_alignment_offset = 4 - (params_to_send % 4);

    /* Space is bufsize minus Netbios over TCP header minus SMB header */
    /* The alignment_offset is to align the param bytes on an even byte
        boundary. NT 4.0 Beta needs this to work correctly. */

    useable_space = max_send - (smb_size
                    + 2 * 10 /* wct */
                    + alignment_offset
                    + data_alignment_offset);

    if (useable_space < 0) {
        DEBUG(0, ("send_trans2_replies failed sanity useable_space "
              "= %d!!!", useable_space));
        exit_server_cleanly("send_trans2_replies: Not enough space");

Now, this is clearly a but in Thursby's DAVE, it shouldn't send a Max Buffer of 0, so I've reported it there. And it's also my bug, that I'm setting "max xmit" to a value greater than what the protocol supports. And furthermore, it seems like a pretty bad protocol bug, that the max buffer size is 32 bits in one message and 16 bits in another, but I have no idea where to file this bug in the protocol.

But it's also pretty bad that smbd just gives up and quits on this, rather that trying to do some kind of renegotiation or using some sane default value; I'm wondering if this can be fixed in Samba as well, by picking a sane default if it gets 0, capping max xmit to 65535, or something of the sort.
Comment 1 Brian Campbell 2014-04-11 01:34:59 UTC
Created attachment 9842 [details]
Packet capture of negotiation of a max_buf of 0 and connection dying
Comment 2 Brian Campbell 2014-04-11 01:35:28 UTC
Created attachment 9843 [details]
Capture of negotiation succeeding after dropping "max xmit" to 65535
Comment 3 Jeremy Allison 2014-04-11 03:48:43 UTC
Isn't this a duplicate of bug 10422 - max xmit > 64kb leads in segmentation fault ?

I think it might be...

Can you try the latest v4-1-test git tree, I think this might be fixed already.


Comment 4 Brian Campbell 2014-04-11 04:05:07 UTC
Hmm, looks like you might be right; looking through those patches, it looks like they do exactly what I suggest (adding sane default minimum and maximum values, and capping the parameter based on that). I'll try out v4-1-test tomorrow.
Comment 5 Jeremy Allison 2014-04-11 21:25:08 UTC

*** This bug has been marked as a duplicate of bug 10422 ***