2.2.5pre1: unlink design flaw
Simo Sorce
simo.sorce at xsec.it
Wed Jun 12 15:30:02 GMT 2002
This changes are not good for a normal samba installation.
First of all, Samba has been built to be run on posix file systems, and
is not guaranteed to work 100% ok with underlying non posix file
systems.
Your modifications make samba vulnerable to unacceptable race
conditions.
Under posix it is perfectly acceptable to delete a file while open, it
will be functional until kept open, when the last process that have it
open close it the system will delete the file, so that become very
similar to delete on close.
The race stay in the fact that you may rename the file and create a link
to an important file between the close and the second unlink, with very
bad results.
The same for directories. AFAIK we do keep files and directories open
and then unlink them just to avoid races.
And samba is not the only application that do this kind of operation,
the proper fix would be to make smbfs driver able to "hide" a file if it
is unlilnked but yet open by some process, and then silently unlink it
when the last process closes it.
It just involves a per open file counter and some kind of magic on
directory listing/file opening.
Making this thing at kernel level with proper atomicity will make
everything better (and more posix compliant).
Simo.
On Wed, 2002-06-12 at 23:50, jd at epcnet.de wrote:
> Hi,
>
> I recovered a design flaw in the unlink part of samba. If you try to re-export a smbfs-share with
> samba and if you create a file with the delete-on-close flag set the file remains on disk.
>
> Unlink over smbfs fails if the file is open.
>
> The samba way of closing a open delete-on-close flagged file is:
>
> 1. unlink file
> 2. close file
>
> But with a underlying smbfs unlink returns ETXTBUSY, so i changed it into:
>
> 1. unlink file
> 2. close file
> 3. if unlink returned ETXTBUSY -> try second time
>
> The same for an open directory which is set delete on close with kernel directory notifiys. First
> the standard samba way:
>
> 1. open dirA for notify
> 2. rmdir dirA
> 3. catch notify signal and close dirA
>
> But with a underlying smbfs rmdir fails, so i changed it into:
>
> 1. open dirA for notify
> 2. rmdir dirA
> 3. If rmdir fails force close of dirA notify handler und try rmdir dirA second time.
>
> I don't know if this also apply to NTFS/FAT filesystems or any other filesystem with a non-posix
> unlink and rmdir behaviour.
>
> Diff against close.c and notify_kernel.c attached.
>
> Greetings
>
> Jochen Dolze
>
> --- close.c.orig Mon Jun 10 13:07:45 2002
> +++ close.c Wed Jun 12 18:19:08 2002
> @@ -1,4 +1,4 @@
> -/*
> +/*
> Unix SMB/Netbios implementation.
> Version 1.9.
> file closing
> @@ -120,6 +120,7 @@
> share_mode_entry *share_entry = NULL;
> size_t share_entry_count = 0;
> BOOL delete_on_close = False;
> + BOOL delete_after_close = False;
> connection_struct *conn = fsp->conn;
> int err = 0;
> int err1 = 0;
> @@ -172,6 +173,8 @@
> DEBUG(5,("close_file: file %s. Delete on close was set - deleting file.\n",
> fsp->fsp_name));
> if(fsp->conn->vfs_ops.unlink(conn,dos_to_unix_static(fsp->fsp_name)) != 0) {
> + if (errno == ETXTBSY) delete_after_close = True;
> +
> /*
> * This call can potentially fail as another smbd may have
> * had the file open with delete on close set and deleted
> @@ -193,6 +196,23 @@
>
> err = fd_close(conn, fsp);
>
> + if (normal_close && delete_on_close && delete_after_close) {
> + DEBUG(5,("close_file: file %s. Delete after close was set - deleting file.\n",
> + fsp->fsp_name));
> + if(fsp->conn->vfs_ops.unlink(conn,dos_to_unix_static(fsp->fsp_name)) != 0) {
> +
> + /*
> + * This call can potentially fail as another smbd may have
> + * had the file open with delete on close set and deleted
> + * it when its last reference to this file went away. Hence
> + * we log this but not at debug level zero.
> + */
> +
> + DEBUG(5,("close_file: file %s. Delete after close was set and unlink failed \
> +with error %s\n", fsp->fsp_name, strerror(errno) ));
> + }
> + }
> +
> /* check for magic scripts */
> if (normal_close) {
> check_magic(fsp,conn);
> @@ -229,6 +249,9 @@
>
> static int close_directory(files_struct *fsp, BOOL normal_close)
> {
> + int delete_after_close = False;
> + BOOL ok;
> +
> remove_pending_change_notify_requests_by_fid(fsp);
>
> /*
> @@ -237,10 +260,15 @@
> */
>
> if (normal_close && fsp->directory_delete_on_close) {
> - BOOL ok = rmdir_internals(fsp->conn, fsp->fsp_name);
> + ok = rmdir_internals(fsp->conn, fsp->fsp_name);
> DEBUG(5,("close_directory: %s. Delete on close was set - deleting directory %s.\n",
> fsp->fsp_name, ok ? "succeeded" : "failed" ));
>
> + if (!ok) {
> + delete_after_close = True;
> + errno = 0; /* reset error */
> + }
> +
> /*
> * Ensure we remove any change notify requests that would
> * now fail as the directory has been deleted.
> @@ -250,10 +278,20 @@
> remove_pending_change_notify_requests_by_filename(fsp);
> }
>
> + if (normal_close && fsp->directory_delete_on_close && delete_after_close) {
> + remove_pending_change_notify_requests_by_filename(fsp);
> + }
> +
> /*
> * Do the code common to files and directories.
> */
> close_filestruct(fsp);
> +
> + if (normal_close && fsp->directory_delete_on_close && delete_after_close) {
> + ok = rmdir_internals(fsp->conn, fsp->fsp_name);
> + DEBUG(5,("close_directory: %s. Delete after close was set - deleting directory %s.\n",
> + fsp->fsp_name, ok ? "succeeded" : "failed" ));
> + }
>
> if (fsp->fsp_name)
> string_free(&fsp->fsp_name);
> --- notify_kernel.c.orig Sat Jun 8 14:55:58 2002
> +++ notify_kernel.c Wed Jun 12 21:25:30 2002
> @@ -128,6 +128,7 @@
> for (i = 0; i < signals_received; i++) {
> if (fd == (int)fd_pending_array[i]) {
> close(fd);
> + fd = -1;
> fd_pending_array[i] = (sig_atomic_t)-1;
> if (signals_received - i - 1) {
> memmove((void *)&fd_pending_array[i], (void *)&fd_pending_array[i+1],
> @@ -141,6 +142,8 @@
> BlockSignals(False, RT_SIGNAL_NOTIFY);
> }
> SAFE_FREE(data);
> + if (fd != -1)
> + close(fd);
> DEBUG(3,("kernel_remove_notify: fd=%d\n", fd));
> }
>
>
>
> ---
> EPCNet GmbH
> ISP & Web Design
> Bleichstrasse 24
> 89077 Ulm
>
> Tel. +49 731 1416 0
> Fax +49 731 1416 120
>
>
>
--
Simo Sorce - simo.sorce at xsec.it
Xsec s.r.l.
via Durando 10 Ed. G - 20158 - Milano
tel. +39 02 2399 7130 - fax: +39 02 700 442 399
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 232 bytes
Desc: This is a digitally signed message part
Url : http://lists.samba.org/archive/samba-technical/attachments/20020612/3c9dc27f/attachment.bin
More information about the samba-technical
mailing list