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

[Jeremy Allison]

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.
-------------- next part --------------
Index: smbd/fileio.c
===================================================================
--- smbd/fileio.c	(revision 24162)
+++ smbd/fileio.c	(working copy)
@@ -509,29 +509,39 @@
 
 			write_path = 3;
 
-                } else if ( (pos >= wcp->file_size) && 
+                } else if ( (pos >= wcp->file_size) &&
 			    (n == 1) &&
-			    (pos < wcp->offset + 2*wcp->alloc_size) &&
-			    (wcp->file_size == wcp->offset + wcp->data_size)) {
+			    (wcp->file_size == wcp->offset + wcp->data_size) &&
+			    (pos < wcp->file_size + wcp->alloc_size)) {
 
                         /*
-                        +---------------+
-                        | Cached data   |
-                        +---------------+
 
+                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 |
+                                 +-----------------------+--------+
                         */
 
-			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;
+			wcp->offset = wcp->file_size;
+			wcp->data_size = pos - wcp->file_size + 1;
 			memset(wcp->data, '\0', wcp->data_size);
 			memcpy(wcp->data + wcp->data_size-1, data, 1);