Potential memory corruption in fileio.c:write_file() since 3.0.20 with write cache size

[Jean-Francois Panisset]

I applied the patch you send against fileio.c from 3.0.23c (that's  
the version I can easily test against), and it seems to work fine:  
valgrind no longer reports any memory corruption when Maya writes out  
the file which was previously causing problems.

I would suggest a slight modification on the ASCII art at the  
beginning of that block of code:

                         /*

                 End of file ---->|

                  +---------------+-------------------------------+
                  | Cached data   | Cache buffer                  |
                  +---------------+-------------------------------+

                                  |<-------------- allocated size  
--------------->|

                                                          +--------+
                                                          | 1 Byte |
                                                          +--------+

                         MS-Office seems to do this a lot to  
determine if there's enough
                         space on the filesystem to write a new file.

                         Change to :

                 End of file ---->|
                                  +-----------------------+-------- 
+--------------+
                                  | Zeroed Cached data    | 1 Byte |  
Cache buffer |
                                  +-----------------------+-------- 
+--------------+
                         */


(hopefully this doesn't get too mangled in email). The idea is to  
show that "allocated size" corresponds to the total size of the write  
cache buffer, i.e. the current amount of cached data plus unused  
write cache buffer, and that after the "one byte write", the write  
cache buffer remains the same size, and that the one new byte of data  
may not be at the end of the write cache buffer.

JF Panisset
A52/Rock Paper Scissors


On Aug 3, 2007, at 9:48 AM, Jeremy Allison wrote:

> On Fri, Aug 03, 2007 at 08:06:46AM -0700, Jean-Francois Panisset  
> wrote:
>> server: Samba 3.0.23c on CentOS5 x86_64
>> client: Autodesk Maya 8.5 (a 3D modeling and animation application)
>> on Windows XP SP2 32 bit
>>
>> After upgrading from Fedora Core 4/Samba 3.0.23a, I started
>> experiencing mysterious hangs with Samba, which seemed to be
>> triggered when writing large scene files from Maya 8.5 in Maya Binary
>> format to a Samba share.
>>
>> After some digging, I came to the following conclusion:
>>
>> The share in question had:
>>
>> write cache size = 32768
>>
>> set in smb.conf. Turns out that when writing out largish files
>> (around 50MB), in some cases Maya likes to write a single byte 49152
>> bytes past the current end of file. This would trigger the following
>> optimization in write_file() in smbd/fileio.c around line 510:
>>
>> } else if ( (pos >= wcp->file_size) &&
>>   (n == 1) &&
>>   (pos < wcp->offset + 2*wcp->alloc_size) &&
>>   (wcp->file_size == wcp->offset + wcp->data_size)) {
>>
>>   SMB_BIG_UINT new_start = wcp->offset + wcp->data_size;
>>
>>   flush_write_cache(fsp, WRITE_FLUSH);
>>   wcp->offset = new_start;
>>   wcp->data_size = pos - new_start + 1;
>>   memset(wcp->data, '\0', wcp->data_size);
>>   memcpy(wcp->data + wcp->data_size-1, data, 1);
>>
>> an optimization which appeared in 3.0.20, and claims to help with
>> Microsoft Office behavior. With "write cache size" set to 32768, this
>> means that wcp->alloc_size is 32768. If the application tries to
>> write 1 byte 49152 bytes past the end of the file, the condition will
>> trigger, and the call to memset() will write past the end of the
>> 32768 byte long piece of memory allocated for the write cache
>> bufferby setup_write_cache(), causing heap corruption. valgrind
>> confirms that this is happening. In theory, I should have been seeing
>> this with 3.0.23a on FC4 as well, but I guess I just got lucky.
>
> Ok, I've looked carefully at this and I think this is the
> correct patch. It's not quite correct to just change the
> 2*wcp->alloc_size to wcp->alloc_size, the actual test should
> be pos < wcp->file_size + wcp->alloc_size I think.
>
> I've attached the patch I plan to commit. Could you test
> this in your setup please ? I think it will leave the
> optimization correctly in place.
>
> Thanks,
>
> 	Jeremy.
> <look>