[PATCH] Fix for bug #9706 - Parameter is incorrect on Android

Jeremy Allison jra at samba.org
Mon Mar 18 15:44:26 MDT 2013


On Mon, Mar 18, 2013 at 08:38:01PM +0100, Stefan (metze) Metzmacher wrote:
> 
> (smb_size -4 + 12*2) seems to be wrong, which my modified version
> I trigger the 0xffffff smb_panic in create_outbuf().

Hmmm. Is that smb_panic check in create_outbuf() correct ?

The 0xFFFFFF restriction is on the bytes following the
4 byte 'type+length' header. We have type = 0, 3 bytes
of big endian length of packet following. So the max
length of the following packet, NOT INCLUDING THE 4
BYTE type+length field is 0xFFFFFF.

smb_size is the 35 byte SMB headers field + the 4 byte 'type+length' header == 39,
so when calculating max read length I think it really should be
max_read_length = 0xFFFFFF - (smb_size - 4 + num_words*2),
Substituting numerical values:

0xFFFFFF - (39 - 4 + 24) which is 16777156 == 0xFFFFC4 bytes.

Looks to me like the code in create_outbuf() is off by
the 4 byte type+length field and really should be:

        /*
         * Protect against integer wrap
         */
        if ((num_bytes > 0xffffff)
            || ((num_bytes + smb_size -4 + num_words*2) > 0xffffff)) {

I'll try that with your patches and do some more tests.

> calc_max_read_pdu() should only calculate the max_pdu size and not the
> 0x10000 windows limitation, and it should not depend on
> smb1.unix_info.client_cap_low nor on global_client_caps.

Ok, that's fair enough. We don't really have to look at
CAP_LARGE_READX on the server if we're using max_pdu.

> For the encrypted case we should use the max_send size from the client
> instead.
> The same applies to chained requests, which means we should not return a
> short read
> instead of an error for chained requests.

Good point - +1 on both of these changes.

> > From 69c288d13e76c7196c8c58dd375b2e1d6345f5fc Mon Sep 17 00:00:00 2001
> > From: Jeremy Allison <jra at samba.org>
> > Date: Wed, 13 Mar 2013 15:43:21 -0700
> > Subject: [PATCH 08/10] Add new LARGE_READX test to investigate large SMBreadX
> > behavior.
> 
> calc_expected_return() should check for CIFS_UNIX_LARGE_READ_CAP instead of
> any value in cli->server_posix_capabilities, we also need to
> fill call cli_unix_extensions_version() if the server returns CAP_UNIX.

That code is only checking for 'any value in cli->server_posix_capabilities'
due to the capabilities manipulation which removes the CIFS_UNIX_LARGE_READ_CAP
in server_posix_capabilitiesI added earlier to get around the
restrictions in cli_read_max_bufsize() that are triggered by the
cli->server_posix_capabilities & CIFS_UNIX_LARGE_READ_CAP check inside
that function.

Now you're proposed removing cli_read_max_bufsize() then I can
correctly check for cli->server_posix_capabilities & CIFS_UNIX_LARGE_READ_CAP
as I originally intended :-).

> I've attached my current patches, but they're not complete
> and fail make test. I'll try to fix this tomorrow.

I'll also update your patches and re-run tests.

Jeremy.


More information about the samba-technical mailing list